Add skipmissings #111

pdeffebach · 2019-12-23T18:58:09Z

This PR adds functionality for multiskipmissings as discussed in Julia issue 30596 here.

The motivation for this PR was noticing that

cor(x, y)

has no missing value support.

julia> x = allowmissing(rand(10)); y = allowmissing(rand(10));

julia> x[1] = missing; y[2] = missing;

julia> cor(x, y)
missing

In order for this to work, you need to collect the non-missing matching values of x and y. To do that I implement the function multiskipmissing, which returns a Tuple of iterators.

julia> x = [1, 2, missing, 4]; y = [1, 2, 3, missing];
julia> tx, ty = multiskipmissing(x, y)
julia> for t in tx
       @show t
       end
t = 1
t = 2

In the above example, each tx and ty are MultiSkipMissing objects which have zip(x, y) as a field. I then implemented skipmissing from Base with some changed logic such that instead of checking if the array of interest has a missing element, I check if any elements of the ith element of zip(x, y) are missing.

I this PR is not complete. I still can't get collect to work, so the original goal is not yet met.

nalimilan

Thanks for pushing this! Can you add a few tests?

I wonder whether this couldn't be called skipmissings. That would be simpler and easier to discover. what do you think?

EDIT: could you run some benchmarks, e.g. comparing with iterating over the input vectors when there are no missing values?

nalimilan · 2019-12-24T13:26:19Z

src/Missings.jl

+    y = iterate(itr.c, ntuple(_ -> first(state), width(itr)))
+    y === nothing && return nothing
+    item, state = y
+    while any(ismissing, item)


I think you can avoid repeating the iterate stuff by doing while true with any(ismissing, item) || break.

Also I suspect that for performance any(x -> x ===missing, item) will be better due to inference limitations. And since that's what skipmissing does, better be consistent anyway.

src/Missings.jl

nalimilan · 2019-12-24T13:34:24Z

src/Missings.jl

+```
+"""
+function multiskipmissing(args...)
+    return ((MultiSkipMissing(args[i], zip(args...), i) for i in 1:length(args))...,)


Suggested change

return ((MultiSkipMissing(args[i], zip(args...), i) for i in 1:length(args))...,)

ntuple(i -> MultiSkipMissing(args[i], zip(args...), i), length(args))

Could also use the short function syntax since that's a one-liner..

nalimilan · 2019-12-24T14:15:56Z

src/Missings.jl

+# Examples
+```
+julia> x = [1, 2, missing, 4]; y = [1, 2, 3, missing];
+julia> tx, ty = multiskipmissing(x, y)


Need to copy the output, which is interesting for users BTW. Also add a line break before (basically, just copy/paste the real output).

nalimilan · 2019-12-24T14:16:41Z

src/Missings.jl

+```
+julia> x = [1, 2, missing, 4]; y = [1, 2, 3, missing];
+julia> tx, ty = multiskipmissing(x, y)
+julia> for t in tx


Probably simpler to show collect(t)?

pdeffebach · 2019-12-24T18:46:05Z

I responded to comments.

I also ported the mapreduce implementation for skipmissing over to this PR. But the implementation relies on all the objects involved in the SkipMissings object to be indexable, which is not known to the compiler given the way the object is currently constructed.

Ideally the type of SkipMissings is something like

struct SkipMissings{T, C} where T where C ::Base.Iterators.Zip{<:Tuple}
    c::C
    I::Int
end

Then we use itr.I to peer into itr.c when needed to get information about the array of interest etc.

And we can dispatch on the elements of the Tuple in the Zip, but I'm not sure how to do that.

nalimilan · 2019-12-25T10:44:38Z

Ideally the type of SkipMissings is something like
struct SkipMissings{T, C} where T where C ::Base.Iterators.Zip{<:Tuple}
    c::C
    I::Int
end
Then we use itr.I to peer into itr.c when needed to get information about the array of interest etc.

And we can dispatch on the elements of the Tuple in the Zip, but I'm not sure how to do that.

I'm not sure what prevents you from doing this. Can you show what you have tried?

pdeffebach · 2019-12-31T22:13:00Z

I have updated this PR with a new implementation. Now the SkipMissings object contains only two fields, a Zip and an Int. When getting values, I use some gymnastics based off of the type signature to get elements from the array of interest. This requires a few more accessor functions which complicate the code a bit so I'm not sure it's worth it.

Additionally, I have updated the type signatures for the optimized reduce implementations. These are complicated pieces of code so if you don't think they are useful, let me know and we can worry about them in a later PR.

src/Missings.jl

nalimilan · 2019-12-25T10:30:34Z

Project.toml

 DataAPI = "1.1"
+julia = "1"


Revert this.

nalimilan · 2019-12-25T10:44:00Z

src/Missings.jl

+
+# Returns nothing when the input contains only missing values, and Some(x) otherwise
+@noinline function Base.mapreduce_impl(f, op, itr::SkipMissings{<:AbstractArray},
+                                  ifirst::Integer, ilast::Integer, blksize::Int)


Fix indentation.

nalimilan · 2020-01-04T15:51:58Z

src/Missings.jl

+"""
+   skipmissings(args...)
+
+


Suggested change

nalimilan · 2020-01-04T15:54:19Z

src/Missings.jl

+    return ntuple(i -> SkipMissings{typeof(z), i}(z, i), length(args))
+end
+
+struct SkipMissings{C, I}


Suggested change

struct SkipMissings{C, I}

struct SkipMissings{C}

Maybe also note this Z?

nalimilan · 2020-01-04T16:10:09Z

src/Missings.jl

+    Base._mapreduce(f, op, Base.IndexStyle(_inner_itr(itr)), Base.eltype(itr.z) <: Tuple{Vararg{Union{Missing, Any}}} ? 
+                                            itr : _inner_itr(itr))
+
+function Base._mapreduce(f, op, ::IndexLinear, itr::SkipMissings{C, I}) where C <: ZipofArrays where I


Suggested change

function Base._mapreduce(f, op, ::IndexLinear, itr::SkipMissings{C, I}) where C <: ZipofArrays where I

function Base._mapreduce(f, op, ::IndexLinear, itr::SkipMissings{C}) where C <: ZipofArrays

nalimilan · 2020-01-04T16:10:34Z

src/Missings.jl

+    Base.mapreduce_impl(f, op, A, ifirst, ilast, Base.pairwise_blocksize(f, op))
+
+# Returns nothing when the input contains only missing values, and Some(x) otherwise
+@noinline function Base.mapreduce_impl(f, op, itr::SkipMissings{C, I},


Suggested change

@noinline function Base.mapreduce_impl(f, op, itr::SkipMissings{C, I},

@noinline function Base.mapreduce_impl(f, op, itr::SkipMissings{C},

nalimilan · 2020-01-04T16:10:43Z

src/Missings.jl

+
+# Returns nothing when the input contains only missing values, and Some(x) otherwise
+@noinline function Base.mapreduce_impl(f, op, itr::SkipMissings{C, I},
+                                       ifirst::Integer, ilast::Integer, blksize::Int) where C <: ZipofArrays where I


Suggested change

ifirst::Integer, ilast::Integer, blksize::Int) where C <: ZipofArrays where I

ifirst::Integer, ilast::Integer, blksize::Int) where C <: ZipofArrays

test/runtests.jl

nalimilan · 2020-01-04T16:13:47Z

test/runtests.jl

@@ -75,6 +75,20 @@ struct CubeRooter end
    @test collect(x) == [1, 2, 4]
    @test collect(x) isa Vector{Int}

+    x = [1, 2, missing, 4]
+    y = [1, 2, 3, missing]
+    z = [missing, missing, 3, 4]


Suggested change

z = [missing, missing, 3, 4]

z = [missing, missing, 3.1, 4.5]

And then test the different eltypes.

pdeffebach · 2020-01-05T20:16:51Z

Thanks for the feedback! Two issues:

First, I don't understand how we can get rid of the I in the type signature of SkipMissings. Otherwise we don't know where to look for eltype etc. when all we have is the type SkipMissings and not an instance of the type.

Second, we don't have inference when compared to skipmissing in Base.

julia> x = [1, 2, missing, 4];
julia> y = ["a", "b", "c", missing];
julia> sx = skipmissing(x);
julia> mx, my = skipmissings(x, y);
julia> @code_warntype sum(sx)
# Variables
#   #self#::Core.Compiler.Const(sum, false)
#   a::Base.SkipMissing{Array{Union{Missing, Int64},1}}
#
# Body::Union{Missing, Int64}
# 1 ─ %1 = Base.sum(Base.identity, a)::Union{Missing, Int64}
# └──      return %1

julia> @code_warntype sum(mx)
# Variables
#   #self#::Core.Compiler.Const(sum, false)
#   a::Missings.SkipMissings{Base.Iterators.Zip{Tuple{Array{Union{Missing, Int64},1},Array{Union{Missing, String},1}}},1}
# 
# Body::Any
# 1 ─ %1 = Base.sum(Base.identity, a)::Any
# └──      return %1

I'm not sure how to fix it. Is the issue with my const ZipofArrays alias? Sorry that I wasn't able to figure this out.

nalimilan · 2020-01-07T08:58:06Z

OK, I see. The problem is likely that k is just a value, not a type parameter, so it's not known at compile time. I think you either have to use I instead of k, or store the element type as a type parameter. The latter is probably better as it will avoid recompiling functions for each value of k, but probably better check the performance of both approaches. If that's not enough, you can add a type assertion to the first value returned by iterate (just like you did to getindex).

Something that needs to be checked is what happens when you iterate over all iterators returned by skipmissings (e.g. using for (x, y) in zip(skipmissings(args...)...) or cor(skipmissings(args...)...)): for performance it is essential that the compiler is able to avoid doing the iteration multiple times for each input. Maybe a special method for SkipMissings{Zip{Tuple{<:AbstractVector}}} with @inbounds will be needed for that.

pdeffebach · 2020-02-15T21:49:18Z

Thanks for the suggestions, and sorry for taking so long to get back to this.

i have added the index of the relevant vector as a type parameter and added lots of type asserts. Still no luck. Let me know if you have any new ideas.

Otherwise I suppose we can iterate the vector separately. But that might have it's own performance drawbacks.

I understand your other point and will work on it soon.

nalimilan · 2020-02-16T16:47:53Z

src/Missings.jl


-function Base.iterate(itr::SkipMissings, state = 1)
-    y = iterate(itr.z, ntuple(_ -> state, _width(itr)))
+function Base.iterate(itr::SkipMissings{Z, I, T}, state = 1)::Tuple{eltype(T), Union{Int, Nothing}} where {Z, I, T}


@code_warntype sum(mx) from your previous post seems to be OK if I remove this failing type assert:

Suggested change

function Base.iterate(itr::SkipMissings{Z, I, T}, state = 1)::Tuple{eltype(T), Union{Int, Nothing}} where {Z, I, T}

function Base.iterate(itr::SkipMissings{Z, I, T}, state = 1) where {Z, I, T}

or if I fix it:

Suggested change

function Base.iterate(itr::SkipMissings{Z, I, T}, state = 1)::Tuple{eltype(T), Union{Int, Nothing}} where {Z, I, T}

function Base.iterate(itr::SkipMissings{Z, I, T}, state = 1)::Union{Tuple{eltype(T), Int}, Nothing} where {Z, I, T}

pdeffebach · 2020-02-16T23:47:58Z

Still failing for me. I'm on 1.3 still. I will download the 1.5 and see if that's the issue.

pdeffebach · 2020-02-17T22:19:37Z

The code correctly infers on 1.4-rc1! Now I will work on the zip propblem.

I would assume we have a "less performant" release for 1.0-1.3 and then people in 1.4 just get the added benefit of having this code work better.

pdeffebach · 2020-02-17T22:54:26Z

Maybe a special method for SkipMissings{Zip{Tuple{<:AbstractVector}}} with @inbounds will be needed for that.

I understand the Zip argument and how that could be expensive. But can you describe what kind of improvement we can do for cor(mx, my) and similar functions? Looking at the code in Statistics, cor doesn't use zip at all.

To be fair, Skipmissing objects can't be used with cor because that just takes abstract vectors. Maybe we need some sort of collect function that works with many skipmissings objects?

nalimilan · 2020-02-18T09:26:46Z

The code correctly infers on 1.4-rc1! Now I will work on the zip propblem.

I would assume we have a "less performant" release for 1.0-1.3 and then people in 1.4 just get the added benefit of having this code work better.

OK, cool. Then yes, let's not worry about performance on older releases, at least for now. Please remove any type asserts that are not needed on Julia 1.4 (and anyway they are not enough on older releases).

I understand the Zip argument and how that could be expensive. But can you describe what kind of improvement we can do for cor(mx, my) and similar functions? Looking at the code in Statistics, cor doesn't use zip at all.

By Zip I didn't mean that we should optimize zip, just that we could/should optimize SkipMissings when it gets passed only AbstractArrays (the most common case). But actually I'm not sure what's needed, so better first benchmark the overhead of skipmissings, e.g. compared to passing vectors directly when they don't contain missing values at all. Maybe it's going to be fast already.

There are two different paths to optimize: iterate, and eachindex+getindex. For iterate, it would be possible to override the generic definition with a more specific one for AbstractArray arguments, which would do for i in eachindex(itr), and would then be able to use @inbounds when indexing the wrapped array. I'm not sure that would help, but it could be worth trying.

For the eachindex+getindex path, it will be a good idea anyway to put@inline before the getindex definition, and inside it check the bounds manually in a @boundscheck block, plus add @inbounds everywhere the index is used after that. eachindex already has @inbounds, but you must ensure that it's propagated to _anymissing, so add @inline to it too. See https://docs.julialang.org/en/latest/devdocs/boundscheck/ for details.

To be fair, Skipmissing objects can't be used with cor because that just takes abstract vectors. Maybe we need some sort of collect function that works with many skipmissings objects?

Ah, indeed. We can probably extend that since the two objects barely need to be iterable and have the same length. Anyway, for now you can just test performance with a custom function, like this:

function testperfzip(X, Y)
    r = zero(eltype(x)) * zero(eltype(x))
    for (x, y) in zip(skipmissings(X, Y)...)
        r += x * y
    end
    r
end

pdeffebach · 2020-03-05T15:03:49Z

I benchmarked this a bit yesterday and it turns out it's enormously slow. I will spend some time next week uncovering the source of the allocations. I think the reason has to do with iterate allocating and not something like eachindex and @inbounds type improvements.

pdeffebach · 2020-03-06T23:00:15Z

Good news! I think this is about on par with what I was expecting, after some re-factoring.

x = map(1:100) do _
    rand() < .2 ? missing : rand()
end

y = map(1:100) do _
    rand() < .2 ? missing : rand()
end

mx, my = skipmissings(x, y)
cx = collect(mx)
cy = collect(my)

function testperfzip(X, Y)
    r = zero(eltype(x)) * zero(eltype(x))
        for (x, y) in zip(X, Y)
            r += x * y
         end
         r
end

julia> @btime testperfzip($cx, $cy)
  1.692 μs (139 allocations: 2.17 KiB)
19.163942918070717

julia> @btime testperfzip($mx, $my)
  33.437 μs (579 allocations: 11.20 KiB)
19.163942918070717

nalimilan · 2020-03-07T16:50:23Z

Cool. At least it looks like the overhead is reasonable. But have you tried with a larger vector? It's not clear whether the number of allocations increases with the length (it shouldn't).

Also, it would be interesting to check with vectors which don't contain any missing values: there the overhead should be low or non-existent, and that's a first goal to reach. Can you push your latest changes?

pdeffebach · 2020-03-08T20:35:33Z

I think this is ready for another review.

My performance improvement was a shift from using Zip internally to just using a Tuple of all the other iterators and writing some helper functions which perform better when checking if anything is missing.

There is one big performance bottleneck and that is my use of

const SkipMissingsofArrays = SkipMissings{V, T} where {V <: AbstractArray, T <: Tuple{Vararg{AbstractArray}}}

Which I then use to do the type signature of functions that need arrays as inputs, in particular mapreduce. Julia isn't doing what I am expecting it to do here. I have

julia> mx isa Missings.SkipMissingsofArrays
julia> true

but mapreduce defaults to any, so something isn't working. Let me know if you have any thoughts.

nalimilan · 2020-03-08T21:44:02Z

Which I then use to do the type signature of functions that need arrays as inputs, in particular mapreduce. Julia isn't doing what I am expecting it to do here. I have
julia> mx isa Missings.SkipMissingsofArrays
julia> true
but mapreduce defaults to any, so something isn't working. Let me know if you have any thoughts.

Have you checked that Base.IndexStyle returns IndexLinear()?

pdeffebach · 2020-03-08T23:14:03Z

I added a method for Base.IndexStyle. It works now! I'm not 100% sure that's what made it work or there was some other error but it works now.

In fact, @code_warntype works better than skipmissing.

I think this PR is ready for a final review and then I would like to either

Overload zip for SkipMissings so that we don't have to perform a double check every time we iterate when two arguments are together, which is a common occurence.
Create a SkipMissingsZip that returns an iterator of tuples.

Both can be done in a separate PR.

pdeffebach · 2020-03-08T23:22:33Z

Nevermind. It was working and now it isn't. I'm not sure what changed and need to investigate further.

pdeffebach · 2020-03-09T13:58:30Z

@nalimilan I think I need you to take a look at it, or I should ask Discourse / Slack for help because I'm not understanding the lack of inference. I believe the issue is with Base.mapreduce_impl defined for SkipMissings.

The error has something to do with Some(something(v)) and Some is where the type inference breaks, I think.

pdeffebach · 2020-03-09T15:19:03Z

posted on discourse here

pdeffebach · 2020-03-11T13:56:41Z

Here is a gist for the new performance behavior. Performance still isn't great, I think. Since sum is good, iterate is working okay. But zip is slow, meaning there could be mileage out of a customized zip implementation.

===========
Proportion missing: 0.2
============

Sum:
==========
N = 100: skipmissings
    2.9539166667e-07 seconds

N = 100: vector
    1.5250752257e-08 seconds

N = 100: manual sum with view
    5.9066480447e-07 seconds

N = 100: manual sum with index
    4.5000000000e-07 seconds

N = 10_000: skipmissings
    2.9712000000e-05 seconds

N = 10_000: vector
    1.0934000000e-06 seconds

N = 10_000: manual sum with view
    2.7132000000e-05 seconds

N = 10_000: manual sum with index
    2.3014000000e-05 seconds


Zip
==========
N = 100: skipmissings
    6.8684000000e-07 seconds

N = 100: vector
    9.9984978541e-08 seconds

N = 100: manual skipping missings
    2.2794827586e-07 seconds

N = 10_000: skipmissings
    1.0735400000e-04 seconds

N = 10_000, vector
    1.1815000000e-05 seconds

N = 10_000: manual skipping missings
    4.7137000000e-05 seconds

============
Proportion missing: 0
============

Sum:
==========
N = 100: skipmissings
    1.7022041420e-07 seconds

N = 100: vector
    1.2069138277e-08 seconds

N = 100: manual sum with view
    4.9769072165e-07 seconds

N = 100: manual sum with index
    3.3501826484e-07 seconds

N = 10_000: skipmissings
    1.4783000000e-06 seconds

N = 10_000: vector
    1.0908000000e-06 seconds

N = 10_000: manual sum with view
    2.4616000000e-05 seconds

N = 10_000: manual sum with index
    1.7179000000e-05 seconds


Zip
==========
N = 100: skipmissings
    1.0692448759e-07 seconds

N = 100: vector
    9.9993576017e-08 seconds

N = 100: manual skipping missings
    1.0125751073e-07 seconds

N = 10_000: skipmissings
    1.1827000000e-05 seconds

N = 10_000, vector
    1.1820000000e-05 seconds

N = 10_000: manual skipping missings
    1.2560000000e-05 seconds

src/Missings.jl

pdeffebach · 2020-03-12T15:36:46Z

I have responded to all comments above.

With regards to branching and inference, I settled on:

ai === missing || @inbounds _anymissingindex(itr.others, i) || (v = op(v, f(ai)))

which is hacky but infers. On the other hand it isn't any faster than nested if statements so let me know if I should change it back. Nested if statements in general seem just as fast as the short-circuited expressions. sum is still 30x slower than a sum with a collected vector, which is frustrating.

I will need your help on show because I'm not sure how to get it to be compact enough.
I added @inbounds in front of every while and for loop and deleted the @inbounds inside those loops. Performance is the same so I think I did it right.

src/Missings.jl

nalimilan · 2020-03-19T09:15:58Z

src/Missings.jl

+    @inbounds x_item = itr.x[ind]
+    @inbounds while true
+        x_item === missing || _anymissingindex(itr.others, ind) || break
+        indnext = iterate(eix, indstate)


Why not call this inditr as above? That's the same thing.

(BTW, it's a bit inconsistent to use x_item but xitr here and in the previous method. Better use the same convention regarding underscores everywhere.)

nalimilan · 2020-03-19T09:19:13Z

src/Missings.jl

@@ -402,7 +406,7 @@ Base.mapreduce_impl(f, op, A::SkipMissingsofArrays, ifirst::Integer, ilast::Inte
        v = op(f(a1), f(a2))
        @simd for i = i:ilast
            @inbounds ai = A[i]
-            ai === missing || @inbounds !_anymissingindex(itr.others, i) || (v = op(v, f(ai)))
+            ai === missing || @inbounds _anymissingindex(itr.others, i) || (v = op(v, f(ai)))


So this was incorrect before and tests didn't spot it? Probably there was no test with an input large enough to use this branch (IIRC 16 elements is the threshold). Can you add one?

added a test for this.

src/Missings.jl

test/runtests.jl

src/Missings.jl

pdeffebach · 2020-03-31T17:10:19Z

I believe I successfully rebased! There were no conflicts.

pdeffebach · 2020-03-31T20:55:58Z

As you know, my rebasing skills are weak. Googling around it seems like a re-base is too complicated for this scenario because I have too many fixes to code I added during the course of this PR. This is an alternate method I got off slack.

It works when I try it on a test branch.

git checkout multiskipmissing # this branch
git reset --soft <commit I diverged from>
git stash
git checkout master
git checkout -b temp_branch
git stash pop # will have to resolve conflicts in two lines. It's nbd. 
git commit -am "Squash everything"
git checkout multiskipmissing 
git reset --hard temp_branch # scary!

pdeffebach · 2020-03-31T21:04:47Z

I propose to do this, but then push temp_branch to a new PR and merge it master in from there.

nalimilan · 2020-04-01T07:41:43Z

I think the rebase is OK (except for that mysterious @inline change). But you need to make the @inferred test conditional on VERSION to fix CI.

src/Missings.jl

pdeffebach · 2020-04-01T13:15:57Z

The rebase didn't work all the way, I still think it's because of all the re-factoring that I did mid-PR. You can tell because the passmissing test still fails but would have not failed had the rebase gone through. But I updated the tests and docs.

pdeffebach · 2020-04-01T13:42:16Z

I can confirm that git merge master works perfectly, though. with no conflicts.

nalimilan · 2020-04-01T15:45:18Z

But the passmissing test doesn't seem to fail (CI passes everywhere except Julia 1.0)?

pdeffebach · 2020-04-01T15:52:06Z

It doesn't seem to be testing 1.4 yet, so it passes. The PR to have it test 1.4 is not in here because the rebase didn't work. If I merge then it will see it.

nalimilan · 2020-04-01T16:16:13Z

But master doesn't appear to test 1.4. I've pulled locally and rebased against latest master, but indeed the branch seems to be really messy. I think the simplest solution would be to squash everything before rebasing, so that you only have to fix conflicts once.

test/runtests.jl

nalimilan · 2020-04-01T20:19:36Z

Argh, since the @inferred failure happens at parsing time, I think you need to wrap it in a @eval block.

pdeffebach · 2020-04-01T21:13:40Z

Tests pass!

nalimilan · 2020-04-02T08:59:07Z

test/runtests.jl

+    @test isapprox(mapreduce(cos, *, collect(mx)),  mapreduce(cos, *, mx))
+    if VERSION >= v"1.4.0-DEV"
+        t = quote 
+            @inferred Union{Float64, Missing} mapreduce(cos, *, $mx)


Actually, thinking about it again, shouldn't this be just Float64? The result cannot be missing by definition, right?

For the skipmissing in Base we have

julia> @code_warntype sum(sx) Variables #self#::Core.Compiler.Const(sum, false) a::Base.SkipMissing{Array{Union{Missing, Int64},1}} Body::Union{Missing, Int64} 1 ─ %1 = Base.sum(Base.identity, a)::Union{Missing, Int64} └── return %1

I don't know why the output works like that given eltype(sx) == Int but it's a yet-to-be improved aspect of mapreduce with skipmissing. So it's fair that skipmissings would work the same. You have a comment on some issue highlighting that it's not a high priority performance issue but I can't find it.

Thankfully small unions are fast.

OK, I need to investigate this. It's somewhat ironic that the function designed to skip missings isn't inferred as such. :-D

Actually I've realized @static if works here instead of @eval. I've committed that.

test/runtests.jl

Co-Authored-By: Milan Bouchet-Valat <nalimilan@club.fr>

nalimilan · 2020-04-03T12:13:22Z

Thanks!

pdeffebach mentioned this pull request Dec 23, 2019

zip-like version of skipmissing for multiple iterators? JuliaLang/julia#30596

Open

nalimilan reviewed Dec 24, 2019

View reviewed changes

nalimilan reviewed Jan 4, 2020

View reviewed changes

nalimilan reviewed Feb 16, 2020

View reviewed changes

pdeffebach mentioned this pull request Mar 9, 2020

Non-propogation, skipmissing-related improvements to Missing handling. JuliaLang/julia#35050

Open

nalimilan reviewed Mar 11, 2020

View reviewed changes

pdeffebach mentioned this pull request Mar 12, 2020

incomplete inference with && and || branches JuliaLang/julia#35090

Open

nalimilan reviewed Mar 19, 2020

View reviewed changes

src/Missings.jl Outdated Show resolved Hide resolved

nalimilan reviewed Apr 1, 2020

View reviewed changes

src/Missings.jl Outdated Show resolved Hide resolved

Squash everything

f235617

pdeffebach force-pushed the multiskipmissing branch from 1a1d12e to f235617 Compare April 1, 2020 16:31

small post-rebase fixes

f115e46

nalimilan reviewed Apr 1, 2020

View reviewed changes

test/runtests.jl Outdated Show resolved Hide resolved

Remove duplicate test

8c874e8

nalimilan closed this Apr 1, 2020

nalimilan reopened this Apr 1, 2020

pdeffebach added 2 commits April 1, 2020 16:48

eval in tests

beab796

interpolate so tests work

6edb38a

make 32 bit tests pass

2d3cb69

nalimilan reviewed Apr 2, 2020

View reviewed changes

pdeffebach and others added 2 commits April 2, 2020 09:43

Update test/runtests.jl

4b20194

Co-Authored-By: Milan Bouchet-Valat <nalimilan@club.fr>

Use @static

70637ea

nalimilan merged commit bb10b04 into JuliaData:master Apr 3, 2020

pdeffebach mentioned this pull request Apr 30, 2020

Make covariance and correlation work for iterators, skipmissing in particular. JuliaStats/Statistics.jl#34

Open

nalimilan changed the title ~~Add multiskipmissing~~ Add skipmissings Apr 30, 2020

pdeffebach mentioned this pull request Aug 26, 2020

Skipping missing values more easily JuliaData/DataFrames.jl#2314

Open

	return ((MultiSkipMissing(args[i], zip(args...), i) for i in 1:length(args))...,)
	ntuple(i -> MultiSkipMissing(args[i], zip(args...), i), length(args))

	function Base._mapreduce(f, op, ::IndexLinear, itr::SkipMissings{C, I}) where C <: ZipofArrays where I
	function Base._mapreduce(f, op, ::IndexLinear, itr::SkipMissings{C}) where C <: ZipofArrays

	@noinline function Base.mapreduce_impl(f, op, itr::SkipMissings{C, I},
	@noinline function Base.mapreduce_impl(f, op, itr::SkipMissings{C},

	ifirst::Integer, ilast::Integer, blksize::Int) where C <: ZipofArrays where I
	ifirst::Integer, ilast::Integer, blksize::Int) where C <: ZipofArrays

	z = [missing, missing, 3, 4]
	z = [missing, missing, 3.1, 4.5]

	function Base.iterate(itr::SkipMissings{Z, I, T}, state = 1)::Tuple{eltype(T), Union{Int, Nothing}} where {Z, I, T}
	function Base.iterate(itr::SkipMissings{Z, I, T}, state = 1) where {Z, I, T}

		"""
		skipmissings(args...)

Add skipmissings #111

Add skipmissings #111

Conversation

pdeffebach commented Dec 23, 2019

nalimilan left a comment • 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

pdeffebach commented Dec 24, 2019

nalimilan commented Dec 25, 2019

pdeffebach commented Dec 31, 2019

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

pdeffebach commented Jan 5, 2020

nalimilan commented Jan 7, 2020

pdeffebach commented Feb 15, 2020

Choose a reason for hiding this comment

pdeffebach commented Feb 16, 2020

pdeffebach commented Feb 17, 2020

pdeffebach commented Feb 17, 2020

nalimilan commented Feb 18, 2020 • edited Loading

pdeffebach commented Mar 5, 2020

pdeffebach commented Mar 6, 2020

nalimilan commented Mar 7, 2020

pdeffebach commented Mar 8, 2020

nalimilan commented Mar 8, 2020

pdeffebach commented Mar 8, 2020

pdeffebach commented Mar 8, 2020

pdeffebach commented Mar 9, 2020

pdeffebach commented Mar 9, 2020

pdeffebach commented Mar 11, 2020 • edited Loading

pdeffebach commented Mar 12, 2020

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

pdeffebach commented Mar 31, 2020

pdeffebach commented Mar 31, 2020

pdeffebach commented Mar 31, 2020

nalimilan commented Apr 1, 2020

pdeffebach commented Apr 1, 2020

pdeffebach commented Apr 1, 2020

nalimilan commented Apr 1, 2020

pdeffebach commented Apr 1, 2020

nalimilan commented Apr 1, 2020

nalimilan commented Apr 1, 2020

pdeffebach commented Apr 1, 2020

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

nalimilan commented Apr 3, 2020

nalimilan left a comment •

edited

Loading

nalimilan commented Feb 18, 2020 •

edited

Loading

pdeffebach commented Mar 11, 2020 •

edited

Loading