From e1610bf47d7c1d10aa10f8d13475edab58fe9bac Mon Sep 17 00:00:00 2001 From: Alexey Stukalov Date: Fri, 7 Oct 2016 12:28:16 +0200 Subject: [PATCH 1/9] fix typo (mapreduce -> reduce) --- src/reduce.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/reduce.jl b/src/reduce.jl index 3afbc1d..966dfd5 100644 --- a/src/reduce.jl +++ b/src/reduce.jl @@ -136,7 +136,7 @@ function Base.mapreduce(f, op, X::NullableArray; skipnull::Bool = false) end """ - mapreduce(f, op::Function, X::NullableArray; [skipnull::Bool=false]) + reduce(op::Function, X::NullableArray; [skipnull::Bool=false]) Reduce `X`under the operation `op`. One can set the behavior of this method to skip the null entries of `X` by setting the keyword argument `skipnull` equal From 747a541f3e9ede892572801c909fdec3f312f4a4 Mon Sep 17 00:00:00 2001 From: Alexey Stukalov Date: Wed, 12 Jul 2017 17:40:59 +0200 Subject: [PATCH 2/9] fix whitespace --- src/indexing.jl | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/indexing.jl b/src/indexing.jl index 73891ad..8ffa122 100644 --- a/src/indexing.jl +++ b/src/indexing.jl @@ -109,17 +109,17 @@ else end function Base.checkbounds(::Type{Bool}, sz::Int, I::NullableVector{Bool}) - any(isnull, I) && throw(NullException()) + any(isnull, I) && throw(NullException()) length(I) == sz - end + end function Base.checkbounds{T<:Real}(::Type{Bool}, sz::Int, I::NullableArray{T}) inbounds = true - any(isnull, I) && throw(NullException()) - for i in 1:length(I) + any(isnull, I) && throw(NullException()) + for i in 1:length(I) @inbounds v = unsafe_getvalue_notnull(I, i) inbounds &= checkbounds(Bool, sz, v) - end + end return inbounds end end From 214386bbbb44539be645a8f57f59ec60621df2dd Mon Sep 17 00:00:00 2001 From: Alexey Stukalov Date: Fri, 7 Oct 2016 08:29:59 +0200 Subject: [PATCH 3/9] fix mapreduce_skipnull for single nonnull element --- src/reduce.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/reduce.jl b/src/reduce.jl index 966dfd5..cc642ab 100644 --- a/src/reduce.jl +++ b/src/reduce.jl @@ -76,7 +76,7 @@ function _mapreduce_skipnull{T}(f, op, X::NullableArray{T}, missingdata::Bool) nnull = countnz(X.isnull) nnull == n && return Base.mr_empty(f, op, T) - nnull == n - 1 && return Base.r_promote(op, f(X.values[findnext(x -> x == false), X, 1])) + nnull == n - 1 && return Nullable(Base.r_promote(op, f(X.values[findfirst(X.isnull, false)]))) # nnull == 0 && return Base.mapreduce_impl(f, op, X, 1, n) return mapreduce_impl_skipnull(f, op, X) From eb82854069aa21e1171b68152ee17c82005d51c8 Mon Sep 17 00:00:00 2001 From: Alexey Stukalov Date: Thu, 9 Mar 2017 15:58:36 +0100 Subject: [PATCH 4/9] fix mapreduce() if all values are null + tests * mapreduce() returns Nullable{T}(mr_empty) instead of just mr_empty * added tests for [map]reduce() over non-empty collection that contain no non-null --- src/reduce.jl | 6 +++--- test/reduce.jl | 46 +++++++++++++++++++++++++++++++--------------- 2 files changed, 34 insertions(+), 18 deletions(-) diff --git a/src/reduce.jl b/src/reduce.jl index cc642ab..73765a4 100644 --- a/src/reduce.jl +++ b/src/reduce.jl @@ -75,9 +75,9 @@ function _mapreduce_skipnull{T}(f, op, X::NullableArray{T}, missingdata::Bool) !missingdata && return Nullable(Base.mapreduce_impl(f, op, X.values, 1, n)) nnull = countnz(X.isnull) - nnull == n && return Base.mr_empty(f, op, T) - nnull == n - 1 && return Nullable(Base.r_promote(op, f(X.values[findfirst(X.isnull, false)]))) - # nnull == 0 && return Base.mapreduce_impl(f, op, X, 1, n) + nnull == n && return Nullable(Base.mr_empty(f, op, T)) + @inbounds (nnull == n - 1 && return Nullable(Base.r_promote(op, f(X.values[findfirst(X.isnull, false)])))) + #nnull == 0 && return Nullable(Base.mapreduce_impl(f, op, X.values, 1, n)) # there is missing data, so nnull>0 return mapreduce_impl_skipnull(f, op, X) end diff --git a/test/reduce.jl b/test/reduce.jl index 3b61c0f..cbb49bc 100644 --- a/test/reduce.jl +++ b/test/reduce.jl @@ -1,22 +1,29 @@ module TestReduce using NullableArrays using Base.Test + import Base.mr_empty - srand(1) f(x) = 5 * x f{T<:Number}(x::Nullable{T}) = ifelse(isnull(x), Nullable{typeof(5 * x.value)}(), Nullable(5 * x.value)) + # FIXME should Base/NullableArrays handle this automatically? + Base.mr_empty(::typeof(f), op::typeof(+), T) = Base.r_promote(op, zero(T)::T) + Base.mr_empty(::typeof(f), op::typeof(*), T) = Base.r_promote(op, one(T)::T) - for N in (10, 2050) + srand(1) + for (N, allnull) in ((2, true), (2, false), (10, false), (2050, false)) A = rand(N) - M = rand(Bool, N) - i = rand(1:N) - M[i] = true - j = rand(1:N) - while j == i - j = rand(1:N) + if allnull + M = fill(true, N) + else + M = rand(Bool, N) + i = rand(1:N) + # should have at least one null and at least one non-null + M[i] = true + j = rand(1:(N-1)) + (j == i) && (j += 1) + M[j] = false end - M[j] = false X = NullableArray(A) Y = NullableArray(A, M) B = A[find(x->!x, M)] @@ -42,13 +49,22 @@ module TestReduce @test isequal(method(X), Nullable(method(A))) @test isequal(method(f, X), Nullable(method(f, A))) @test isequal(method(Y), Nullable{Float64}()) - v = method(Y, skipnull=true) - @test v.value ≈ method(B) - @test !isnull(v) @test isequal(method(f, Y), Nullable{Float64}()) - v = method(f, Y, skipnull=true) - @test v.value ≈ method(f, B) - @test !isnull(v) + # test skipnull=true + if !allnull || method ∈ [sum, prod] + # reduce + v_r = method(Y, skipnull=true) + @test v_r.value ≈ method(B) + @test !isnull(v_r) + # mapreduce + v_mr = method(f, Y, skipnull=true) + @test v_mr.value ≈ method(f, B) + @test !isnull(v_mr) + else + # reduction over empty collection not defined for these methods + @test_throws ArgumentError method(Y, skipnull=true) + @test_throws ArgumentError method(f, Y, skipnull=true) + end end @test isequal(extrema(X), (Nullable(minimum(A)), Nullable(maximum(A)))) From 27775a260e3ca771f3072efaa39f5481de344c48 Mon Sep 17 00:00:00 2001 From: Alexey Stukalov Date: Tue, 4 Oct 2016 13:35:46 +0200 Subject: [PATCH 5/9] add comments to unsafe_getXXX_notnull() --- src/indexing.jl | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/indexing.jl b/src/indexing.jl index 8ffa122..28a00b6 100644 --- a/src/indexing.jl +++ b/src/indexing.jl @@ -76,10 +76,12 @@ to store `v` at `I`. return v end +# return the value of non-null X element wrapped in Nullable function unsafe_getindex_notnull(X::NullableArray, I::Int...) return Nullable(getindex(X.values, I...)) end +# return the value of non-null X element function unsafe_getvalue_notnull(X::NullableArray, I::Int...) return getindex(X.values, I...) end From 3f6685f61ecc37f050737c8460c17071cd96ebb9 Mon Sep 17 00:00:00 2001 From: Alexey Stukalov Date: Wed, 5 Oct 2016 00:05:43 +0200 Subject: [PATCH 6/9] add isnull(AbstractArray{Nullable}, i) fallback --- src/primitives.jl | 1 + 1 file changed, 1 insertion(+) diff --git a/src/primitives.jl b/src/primitives.jl index af2a9a9..b7b1e41 100644 --- a/src/primitives.jl +++ b/src/primitives.jl @@ -1,4 +1,5 @@ Base.isnull(X::NullableArray, I::Int...) = X.isnull[I...] +Base.isnull{T}(X::AbstractArray{Nullable{T}}, I::Int...) = isnull(X[I...]) # fallback method Base.values(X::NullableArray, I::Int...) = X.values[I...] """ From 3e56713e22159083dd88795acc9b54f39a0e764f Mon Sep 17 00:00:00 2001 From: Alexey Stukalov Date: Wed, 5 Oct 2016 00:12:00 +0200 Subject: [PATCH 7/9] unsafe_xxx_notnull(AbstrArr{Nullable{T}}) fallback --- src/indexing.jl | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/indexing.jl b/src/indexing.jl index 28a00b6..e5954d1 100644 --- a/src/indexing.jl +++ b/src/indexing.jl @@ -80,11 +80,17 @@ end function unsafe_getindex_notnull(X::NullableArray, I::Int...) return Nullable(getindex(X.values, I...)) end +function unsafe_getindex_notnull{T}(X::AbstractArray{Nullable{T}}, I::Int...) + return getindex(X, I...) +end # return the value of non-null X element function unsafe_getvalue_notnull(X::NullableArray, I::Int...) return getindex(X.values, I...) end +function unsafe_getvalue_notnull{T}(X::AbstractArray{Nullable{T}}, I::Int...) + return get(getindex(X, I...)) +end if VERSION >= v"0.5.0-dev+4697" function Base.checkindex(::Type{Bool}, inds::AbstractUnitRange, i::Nullable) From 9643fb21775119c00814c19bea07b180a5e7a242 Mon Sep 17 00:00:00 2001 From: Alexey Stukalov Date: Wed, 18 Jan 2017 18:03:50 +0100 Subject: [PATCH 8/9] annotate more indexing methods with @inline should have the same effect as @propagate_inbounds within @inbounds blocks --- src/indexing.jl | 10 +++++----- src/primitives.jl | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/indexing.jl b/src/indexing.jl index e5954d1..36cac83 100644 --- a/src/indexing.jl +++ b/src/indexing.jl @@ -77,18 +77,18 @@ to store `v` at `I`. end # return the value of non-null X element wrapped in Nullable -function unsafe_getindex_notnull(X::NullableArray, I::Int...) +@inline function unsafe_getindex_notnull(X::NullableArray, I::Int...) return Nullable(getindex(X.values, I...)) end -function unsafe_getindex_notnull{T}(X::AbstractArray{Nullable{T}}, I::Int...) +@inline function unsafe_getindex_notnull{T}(X::AbstractArray{Nullable{T}}, I::Int...) return getindex(X, I...) end # return the value of non-null X element -function unsafe_getvalue_notnull(X::NullableArray, I::Int...) +@inline function unsafe_getvalue_notnull(X::NullableArray, I::Int...) return getindex(X.values, I...) end -function unsafe_getvalue_notnull{T}(X::AbstractArray{Nullable{T}}, I::Int...) +@inline function unsafe_getvalue_notnull{T}(X::AbstractArray{Nullable{T}}, I::Int...) return get(getindex(X, I...)) end @@ -142,6 +142,6 @@ end This is a convenience method to set the entry of `X` at index `I` to be null """ -function nullify!(X::NullableArray, I...) +@inline function nullify!(X::NullableArray, I...) setindex!(X, Nullable{eltype(X)}(), I...) end diff --git a/src/primitives.jl b/src/primitives.jl index b7b1e41..83bbee2 100644 --- a/src/primitives.jl +++ b/src/primitives.jl @@ -1,6 +1,6 @@ -Base.isnull(X::NullableArray, I::Int...) = X.isnull[I...] -Base.isnull{T}(X::AbstractArray{Nullable{T}}, I::Int...) = isnull(X[I...]) # fallback method -Base.values(X::NullableArray, I::Int...) = X.values[I...] +@inline Base.isnull(X::NullableArray, I::Int...) = X.isnull[I...] +@inline Base.isnull{T}(X::AbstractArray{Nullable{T}}, I::Int...) = isnull(X[I...]) # fallback method +@inline Base.values(X::NullableArray, I::Int...) = X.values[I...] """ size(X::NullableArray, [d::Real]) From 3dab600ba6fd3603394241499acb060fb157a2af Mon Sep 17 00:00:00 2001 From: Alexey Stukalov Date: Thu, 9 Mar 2017 16:13:29 +0100 Subject: [PATCH 9/9] mapreduce(): don't scan for missing values twice since _mapreduce_skipnull() counts the number of nulls anyway, it doesn't need the `missingdata` parameter --- src/reduce.jl | 49 ++++++++++++++----------------------------------- 1 file changed, 14 insertions(+), 35 deletions(-) diff --git a/src/reduce.jl b/src/reduce.jl index 73765a4..b40af09 100644 --- a/src/reduce.jl +++ b/src/reduce.jl @@ -70,33 +70,24 @@ mapreduce_impl_skipnull(f, op::typeof(@functorize(+)), X::NullableArray) = # general mapreduce interface -function _mapreduce_skipnull{T}(f, op, X::NullableArray{T}, missingdata::Bool) +function _mapreduce_skipnull{T}(f, op, X::NullableArray{T}) n = length(X) - !missingdata && return Nullable(Base.mapreduce_impl(f, op, X.values, 1, n)) - nnull = countnz(X.isnull) + # handle specific corner cases (no nulls, all nulls, one non-null) + nnull == 0 && return Nullable(Base.mapreduce_impl(f, op, X.values, 1, n)) nnull == n && return Nullable(Base.mr_empty(f, op, T)) @inbounds (nnull == n - 1 && return Nullable(Base.r_promote(op, f(X.values[findfirst(X.isnull, false)])))) - #nnull == 0 && return Nullable(Base.mapreduce_impl(f, op, X.values, 1, n)) # there is missing data, so nnull>0 return mapreduce_impl_skipnull(f, op, X) end -function Base._mapreduce(f, op, X::NullableArray, missingdata) - missingdata && return Base._mapreduce(f, op, X) - Nullable(Base._mapreduce(f, op, X.values)) -end +Base._mapreduce(f, op, X::NullableArray, missingdata::Bool) = + missingdata ? Base._mapreduce(f, op, X) : Nullable(Base._mapreduce(f, op, X.values)) # to fix ambiguity warnings -function Base.mapreduce(f, op::Union{typeof(@functorize(&)), typeof(@functorize(|))}, - X::NullableArray, skipnull::Bool = false) - missingdata = any(isnull, X) - if skipnull - return _mapreduce_skipnull(f, op, X, missingdata) - else - return Base._mapreduce(f, op, X, missingdata) - end -end +Base.mapreduce(f, op::Union{typeof(@functorize(&)), typeof(@functorize(|))}, + X::NullableArray, skipnull::Bool = false) = + skipnull ? _mapreduce_skipnull(f, op, X) : Base._mapreduce(f, op, X, any(isnull, X)) if VERSION >= v"0.5.0-dev+3701" @@ -115,25 +106,13 @@ behavior is enabled, `f` will be automatically lifted over the elements of `X`. Note that, in general, mapreducing over a `NullableArray` will return a `Nullable` object regardless of whether `skipnull` is set to `true` or `false`. """ -function Base.mapreduce(f, op::Function, X::NullableArray; - skipnull::Bool = false) - missingdata = any(isnull, X) - if skipnull - return _mapreduce_skipnull(f, specialized_binary(op), - X, missingdata) - else - return Base._mapreduce(f, specialized_binary(op), X, missingdata) - end -end +Base.mapreduce(f, op::Function, X::NullableArray; skipnull::Bool = false) = + skipnull ? _mapreduce_skipnull(f, specialized_binary(op), X) : + Base._mapreduce(f, specialized_binary(op), X, any(isnull, X)) -function Base.mapreduce(f, op, X::NullableArray; skipnull::Bool = false) - missingdata = any(isnull, X) - if skipnull - return _mapreduce_skipnull(f, op, X, missingdata) - else - return Base._mapreduce(f, op, X, missingdata) - end -end +Base.mapreduce(f, op, X::NullableArray; skipnull::Bool = false) = + skipnull ? _mapreduce_skipnull(f, specialized_binary(op), X) : + Base._mapreduce(f, specialized_binary(op), X, any(isnull, X)) """ reduce(op::Function, X::NullableArray; [skipnull::Bool=false])