Add conditional and passmissing #89

bkamins · 2018-08-30T08:16:51Z

This is a proposal of implementation of JuliaLang/julia#26661. Actually the best name for conditional would be ifelse but unfortunately it cannot be extended.

nalimilan

Thanks. Is performance always good or are there allocations in some cases?

src/Missings.jl

ararslan · 2018-08-31T05:28:03Z

src/Missings.jl

+    P(xs...; kw...) ? X(xs...; kw...) : Y(xs...; kw...)
+
+_passmissing_predicate(xs...;kw...) =
+    any(ismissing.(xs)) || any(ismissing.(values(values(kw))))


It's more efficient to pass ismissing as a function argument to any rather than broadcasting, i.e.

any(ismissing, xs) || any(ismissing, values(values(kw)))

Right. In any case, this really needs to be unrolled, specializing on the number of arguments, or it will be too slow. EDIT: I mean that the compiler should hopefully be able to do that automatically.

nalimilan · 2018-09-08T12:03:26Z

src/Missings.jl

+  missing
+   2.0
+"""
+passmissing(f::Base.Callable) = PassMissing{f}()


Shouldn't this be typeof(f)?

BTW, have you done some benchmarking to check there are no allocations?

AFAIK if I wanted to use typeof{f} then I would have to store f in a struct which would be slower and would allocate more.

I will post benchmarks for the current implementation in the main thread so that they do not disappear.

I think in most practical uses, the struct would never allocate. It's the same idea w/ the new iteration protocol: every call to iterate potentially returns a Tuple{T...}, but most of the time those tuples and even intermediate allocated objects don't even get allocated.

nalimilan · 2018-09-08T12:16:46Z

src/Missings.jl

+
+# Examples
+```jldoctest
+julia> passmissing(sqrt).([missing, 4])


It would be useful to start with a simpler example with a single scalar (first non-missing, and then missing).

bkamins · 2018-09-08T13:02:33Z

Here are the benchmarks. In general, if I understand it correctly, splatting slows it down in more complex scenarios. Fortunately for single positional argument this is fast (and I would say this is a most common case). We could define functions like:

(::PassMissing{F})(x1, x2) where {F} =
    ismissing(x1) || ismissing(x2) ? missing : F(x1, x2)

to handle more complex cases faster. I do not know if we want to as, hopefully, Julia should be able to handle such splatting as used in this PR fast (not sure though).

Benchmark 1: simple scenario

julia> checkedsqrt(x) = x isa Missing ? missing : sqrt(x)
checkedsqrt (generic function with 1 method)

julia> @btime checkedsqrt(4.0)
  1.866 ns (0 allocations: 0 bytes)
2.0

julia> @btime checkedsqrt(missing)
  0.001 ns (0 allocations: 0 bytes)
missing

julia> checkedsqrt2(x) = passmissing(sqrt)(x)
checkedsqrt2 (generic function with 1 method)

julia> @btime checkedsqrt2(4.0)
  1.866 ns (0 allocations: 0 bytes)
2.0

julia> @btime checkedsqrt2(missing)
  0.001 ns (0 allocations: 0 bytes)
missing

Benchmark 2: complex scenario

julia> parsemissing(t, s, b) = t isa Missing || s isa Missing || b isa Missing ? missing : parse(t, s, base=b)
parsemissing (generic function with 1 method)

julia> @btime parsemissing(Int, "1234", 5)
  89.091 ns (0 allocations: 0 bytes)
194

julia> @btime parsemissing(Int, "1234", missing)
  0.001 ns (0 allocations: 0 bytes)
missing

julia> @btime parsemissing(missing, "1234", 5)
  0.001 ns (0 allocations: 0 bytes)
missing

julia> parsemissing2(t, s, b) = passmissing(parse)(t, s, base=b)
parsemissing2 (generic function with 1 method)

julia> @btime parsemissing2(Int, "1234", 5)
  752.883 ns (4 allocations: 96 bytes)
194

EDIT: removed wrong copy-paste

bkamins · 2018-09-08T23:35:42Z

Using @generated improves performance a bit.

bkamins · 2018-09-08T23:54:32Z

Of course some independent benchmarks are welcome (but at least looking at @code_warntype the generated version is type-inferred correctly as should be expected).

CC @pszufe

quinnj

I think the implementation could be a bit more idiomatic (I left some comments w/ suggestions).

quinnj · 2018-09-13T04:27:04Z

src/Missings.jl

@@ -1,7 +1,7 @@
 module Missings

 export allowmissing, disallowmissing, ismissing, missing, missings,
-       Missing, MissingException, levels, coalesce
+       Missing, MissingException, levels, coalesce, passmissing


I personally prefer Missings.propagate if we're still bikeshedding the name here.

I like propagate too, the only gripe is that it's quite long. But well...

But do we want to export it or not? If yes then propagate is ok, but Missings.propagate is a bit long.

Unfortunately propagate is quite general to claim it for this feature...

I know, but that is why I thought the idea was propagatemissing which tab-completes.

quinnj · 2018-09-13T04:29:19Z

src/Missings.jl

@@ -165,4 +165,30 @@ function levels(x)
    levs
 end

+struct PassMissing{F} <: Function end
+
+@generated (::PassMissing{F})(xs...;kw...) where {F} =


I don't think the @generated is doing anything here except ensuring a new function gets compiled w/ every call.

quinnj · 2018-09-13T04:29:56Z

src/Missings.jl

@@ -165,4 +165,30 @@ function levels(x)
    levs
 end

+struct PassMissing{F} <: Function end


I think we probably want:

struct PassMissing{F} <: Function f::F end

quinnj · 2018-09-13T04:32:23Z

src/Missings.jl

+  missing
+   2.0
+"""
+passmissing(f::Base.Callable) = PassMissing{f}()


I think in most practical uses, the struct would never allocate. It's the same idea w/ the new iteration protocol: every call to iterate potentially returns a Tuple{T...}, but most of the time those tuples and even intermediate allocated objects don't even get allocated.

quinnj · 2018-09-13T04:35:13Z

src/Missings.jl

+struct PassMissing{F} <: Function end
+
+@generated (::PassMissing{F})(xs...;kw...) where {F} =
+    :(any(ismissing, xs) || any(ismissing, values(values(kw))) ? missing : F(xs...; kw...))


Hmmm, the inclusion of keyword arguments here seems off to me; what if missing is a totally valid thing to pass as a keyword argument, but now it's interacting weird w/ PassMissing because it always makes my result missing?

I think my stab at this definition would be more along the lines of:

struct PassMissing{F} <: Function f::F end function (f::PassMissing{F})(x) where {F} if @generated return x === Missing ? missing : :(f.f(x)) else return x === missing ? missing : f.f(x) end end function (f::PassMissing{F})(xs...; kw...) where {F} if @generated for T in xs T === Missing && return missing end return :(f.f(xs...; kw...)) else return any(ismissing, xs) ? missing : f.f(xs...; kw...) end end

with specialize methods for 1 argument, 2 argument, maybe 3.

Interesting. Though with generated functions you don't need to specialize on the number of arguments.

Also why do you think keyword arguments should be excluded from the missingness check?

Thank you for the comments. I have done additional benchmarking and it looks you are right with the optimizations 👍 (I did some tests earlier that did allocate).

I do not understand how and why this if @generated part works.
In particular e.g. why is it different from just writing:

@generated function (f::PassMissing{F})(x) where {F} return x === Missing ? missing : :(f.f(x)) end

Could you please explain?

Regarding other issues:

function name: I am open to anything (not a native)

I was also unclear if we want to include keyword arguments as in general it is somewhat arbitrary what user sets as positional argument and what as keyword argument (especially as kwargs do not require to have default values).

The if @generated part is just a future-proof against a time when you're able to statically compile a julia program into an executable that can run without LLVM (i.e. w/o runtime compilation). The else block just provides a path that would be executed if the compiler encountered an already non-generated/compiled method for the given arguments at runtime.

For keyword arguments, it just feels more arbitrary. Like, it's hard to imagine a case where I'd be maping over some Vector{Union{Float64, Missing}} and Vector{Union{Int, Missing}} and be doing something like map((x, y)->round(x; digits=y), zip(A, B)). i.e. when would I maybe pass a value vs. maybe pass a missing as a keyword argument? Keyword arguments also tend to use various "sentinel" values as signals or special values for the function to use, including missing; the danger there being that someone writes a function foo(x...; sentinel=nothing), but when the user tries to call Missings.propagate(foo)(x; sentinel=missing), the entire result comes back missing instead of passing missing on to foo as a valid sentinel value.

If anything, I'd say we leave keyword arguments out of the mix for now since they can always be added later.

My main issue with passmissing is that pass usually has a connotation of "ignore" or "do nothing" (see python's pass), whereas we're not really ignoring missing values, we're propagating them.

For keyword arguments, it just feels more arbitrary. Like, it's hard to imagine a case where I'd be maping over some Vector{Union{Float64, Missing}} and Vector{Union{Int, Missing}} and be doing something like map((x, y)->round(x; digits=y), zip(A, B)). i.e. when would I maybe pass a value vs. maybe pass a missing as a keyword argument? Keyword arguments also tend to use various "sentinel" values as signals or special values for the function to use, including missing; the danger there being that someone writes a function foo(x...; sentinel=nothing), but when the user tries to call Missings.propagate(foo)(x; sentinel=missing), the entire result comes back missing instead of passing missing on to foo as a valid sentinel value.

I don't think there are any examples of functions using missing as a sentinel currently, right? We rather want people to use nothing for that.

I see your point about keyword arguments generally being about options, while the data is passed as positional arguments, but I'd find it problematic to make this a rule. Overall it doesn't seem we have strong use cases for either behavior, and in such situations I tend to favor the simplest rule (i.e. "return missing if one of the arguments is missing").

If anything, I'd say we leave keyword arguments out of the mix for now since they can always be added later.

The problem is, changing that would be breaking. The only way to be able to choose any behavior later is to throw an error if a keyword argument is missing.

The only way to be able to choose any behavior later is to throw an error if a keyword argument is missing.

We could define lift for functions that do not take keyword arguments for the time being. It is easy enough to wrap a function requiring kwargs in an anonymous function.

tcovert · 2019-01-06T16:20:01Z

Did this ever make its way into master? Having a consistent way to "wrap" routines that might have to deal with missing values in DataFrames data cleaning would be pretty nice. Right now, the absence of missing-aware Date methods is kind of a pain...

bkamins · 2019-01-06T16:55:56Z

I was not clear what the best API would be so I left it hanging.

@tcovert do you have any opinion what would be best given the discussion above?

bkamins · 2019-01-08T15:25:59Z

Having looked at the options I recommend:

to use the design proposed by @quinnj
not to accept keyword arguments at all; we can add this support later (that change will be non-breaking) and there is no strong consensus what to do with them for now; I would say that most use cases will not involve keyword arguments anyway

@nalimilan I we accept this PR the side effect will be that DataFrames.jl will re-export this (which is actually nice I think) - I will add it to the documentation and tutorial.

codecov-io · 2019-01-08T15:41:18Z

Codecov Report

Merging #89 into master will decrease coverage by 11.53%.
The diff coverage is 80%.

@@             Coverage Diff             @@
##           master      #89       +/-   ##
===========================================
- Coverage     100%   88.46%   -11.54%     
===========================================
  Files           1        1               
  Lines          38       52       +14     
===========================================
+ Hits           38       46        +8     
- Misses          0        6        +6

Impacted Files	Coverage Δ
src/Missings.jl	`88.46% <80%> (-11.54%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f37ce9...8c0bd5d. Read the comment docs.

bkamins · 2019-01-08T15:47:53Z

codecov gave a strange result (and the else part of if @generated is not covered by tests)

nalimilan · 2019-01-09T17:44:18Z

Sounds good for now. Can you document (and possibly test) that keyword arguments are not supported?

bkamins · 2019-01-09T19:06:11Z

Added. Thanks for looking at it.

nalimilan

Thanks!

bkamins · 2019-01-17T14:24:27Z

Do we want to merge it or wait for more feedback?

nalimilan · 2019-01-17T15:18:45Z

There's been plenty of time for feedback, and the PR throws an error for keyword arguments, so we can discuss that later.

add conditional and passmissing

1516580

bkamins mentioned this pull request Aug 30, 2018

add lift function for working with missing values JuliaLang/julia#26661

Closed

nalimilan reviewed Aug 30, 2018

View reviewed changes

src/Missings.jl Outdated Show resolved Hide resolved

ararslan reviewed Aug 31, 2018

View reviewed changes

leave a minimal implementation of passmissing

3bf10c0

bkamins force-pushed the passmissing branch from da23dd0 to 3bf10c0 Compare August 31, 2018 19:18

Improved docstring

f947bcf

nalimilan reviewed Sep 8, 2018

View reviewed changes

use generated function

e775f2b

quinnj reviewed Sep 13, 2018

View reviewed changes

improved passmissing implementation

4cc1a5e

fix a typo

7f491bf

test keyword arguments

8c0bd5d

nalimilan approved these changes Jan 10, 2019

View reviewed changes

stevengj mentioned this pull request Jan 11, 2019

convert methods between String and Symbol JuliaLang/julia#30680

Closed

nalimilan merged commit 9cca49f into JuliaData:master Jan 17, 2019

bkamins deleted the passmissing branch January 17, 2019 15:22

Add conditional and passmissing #89

Add conditional and passmissing #89

Conversation

bkamins commented Aug 30, 2018

nalimilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan Aug 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkamins commented Sep 8, 2018 • edited Loading

Benchmark 1: simple scenario

Benchmark 2: complex scenario

bkamins commented Sep 8, 2018

bkamins commented Sep 8, 2018

quinnj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcovert commented Jan 6, 2019

bkamins commented Jan 6, 2019

bkamins commented Jan 8, 2019

codecov-io commented Jan 8, 2019 • edited Loading

Codecov Report

bkamins commented Jan 8, 2019

nalimilan commented Jan 9, 2019

bkamins commented Jan 9, 2019

nalimilan left a comment

Choose a reason for hiding this comment

bkamins commented Jan 17, 2019

nalimilan commented Jan 17, 2019

nalimilan Aug 31, 2018 •

edited

Loading

bkamins commented Sep 8, 2018 •

edited

Loading

codecov-io commented Jan 8, 2019 •

edited

Loading