From 1e2d6a04db92274d2e32b1e6e5f07b616786f484 Mon Sep 17 00:00:00 2001 From: Alexey Stukalov Date: Fri, 7 Oct 2016 12:28:16 +0200 Subject: [PATCH 1/5] 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 3713e08..c3f08f8 100644 --- a/src/reduce.jl +++ b/src/reduce.jl @@ -136,7 +136,7 @@ function Base.mapreduce(f, op, X::NullableArray; skipnull::Bool = false) end @doc """ -`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 1b75f2891ee07112896ad74720d193a3796869f3 Mon Sep 17 00:00:00 2001 From: Alexey Stukalov Date: Fri, 7 Oct 2016 08:29:59 +0200 Subject: [PATCH 2/5] 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 c3f08f8..5686eb2 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 1b25f714d65d4c116e343173f361aec43cde197e Mon Sep 17 00:00:00 2001 From: Alexey Stukalov Date: Wed, 5 Oct 2016 21:37:55 +0200 Subject: [PATCH 3/5] test reduce with vectors of length 2 exposes some corner cases that are not observed with N>2 --- test/reduce.jl | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/test/reduce.jl b/test/reduce.jl index cc13eae..a9737e4 100644 --- a/test/reduce.jl +++ b/test/reduce.jl @@ -7,15 +7,14 @@ module TestReduce f{T<:Number}(x::Nullable{T}) = ifelse(isnull(x), Nullable{typeof(5 * x.value)}(), Nullable(5 * x.value)) - for N in (10, 2050) + for N in (2, 10, 2050) A = rand(N) 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) - while j == i - j = rand(1:N) - end + j = rand(1:(N-1)) + (j == i) && (j += 1) M[j] = false X = NullableArray(A) Y = NullableArray(A, M) From c0f76aee58ebd5fcb4007297a482d52b920371d3 Mon Sep 17 00:00:00 2001 From: Alexey Stukalov Date: Fri, 7 Oct 2016 08:32:46 +0200 Subject: [PATCH 4/5] don't count missing data twice Avoid counting the zeros twice (for missingdata flag and for nnull) if skipnull=true. --- src/reduce.jl | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/src/reduce.jl b/src/reduce.jl index 5686eb2..7926a32 100644 --- a/src/reduce.jl +++ b/src/reduce.jl @@ -70,19 +70,19 @@ 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) - 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 Nullable(Base.mapreduce_impl(f, op, X.values, 1, n)) + (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) return mapreduce_impl_skipnull(f, op, X) end -function Base._mapreduce(f, op, X::NullableArray, missingdata) +function Base._mapreduce(f, op, X::NullableArray, missingdata::Bool) missingdata && return Base._mapreduce(f, op, X) Nullable(Base._mapreduce(f, op, X.values)) end @@ -90,11 +90,10 @@ end # to fix ambiguity warnings function Base.mapreduce(f, op::Union{typeof(@functorize(&)), typeof(@functorize(|))}, X::NullableArray, skipnull::Bool = false) - missingdata = anynull(X) if skipnull - return _mapreduce_skipnull(f, op, X, missingdata) + return _mapreduce_skipnull(f, op, X) else - return Base._mapreduce(f, op, X, missingdata) + return Base._mapreduce(f, op, X, anynull(X)) end end @@ -117,21 +116,18 @@ Note that, in general, mapreducing over a `NullableArray` will return a """ -> function Base.mapreduce(f, op::Function, X::NullableArray; skipnull::Bool = false) - missingdata = anynull(X) if skipnull - return _mapreduce_skipnull(f, specialized_binary(op), - X, missingdata) + return _mapreduce_skipnull(f, specialized_binary(op), X) else - return Base._mapreduce(f, specialized_binary(op), X, missingdata) + return Base._mapreduce(f, specialized_binary(op), X, anynull(X)) end end function Base.mapreduce(f, op, X::NullableArray; skipnull::Bool = false) - missingdata = anynull(X) if skipnull - return _mapreduce_skipnull(f, op, X, missingdata) + return _mapreduce_skipnull(f, op, X) else - return Base._mapreduce(f, op, X, missingdata) + return Base._mapreduce(f, op, X, anynull(X)) end end From 80f62d9d51318b15804b0d17e77d476469838385 Mon Sep 17 00:00:00 2001 From: Alexey Stukalov Date: Fri, 7 Oct 2016 10:04:02 +0200 Subject: [PATCH 5/5] 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 | 2 +- test/reduce.jl | 47 ++++++++++++++++++++++++++++++++--------------- 2 files changed, 33 insertions(+), 16 deletions(-) diff --git a/src/reduce.jl b/src/reduce.jl index 7926a32..fadbe5f 100644 --- a/src/reduce.jl +++ b/src/reduce.jl @@ -75,7 +75,7 @@ function _mapreduce_skipnull{T}(f, op, X::NullableArray{T}) nnull = countnz(X.isnull) (nnull == 0) && return Nullable(Base.mapreduce_impl(f, op, X.values, 1, n)) - (nnull == n) && return Base.mr_empty(f, op, T) + (nnull == n) && return Nullable(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) diff --git a/test/reduce.jl b/test/reduce.jl index a9737e4..5f202c1 100644 --- a/test/reduce.jl +++ b/test/reduce.jl @@ -1,21 +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 (2, 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) - # 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 + 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 X = NullableArray(A) Y = NullableArray(A, M) B = A[find(x->!x, M)] @@ -41,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_approx_eq v.value method(B) - @test !isnull(v) @test isequal(method(f, Y), Nullable{Float64}()) - v = method(f, Y, skipnull=true) - @test_approx_eq v.value method(f, B) - @test !isnull(v) + # test skipnull=true + if !allnull || method ∈ [sum, prod] + # reduce + v_r = method(Y, skipnull=true) + @test_approx_eq v_r.value method(B) + @test !isnull(v_r) + # mapreduce + v_mr = method(f, Y, skipnull=true) + @test_approx_eq 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 for method in (