[RFC] Fix [map]reduce with no non-null and single non-null element

src/reduce.jl

@@ -70,31 +70,30 @@ 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 Base.r_promote(op, f(X.values[findnext(x -> x == false), X, 1]))
+
+    (nnull == 0) && return Nullable(Base.mapreduce_impl(f, op, X.values, 1, n))
+    (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)
 
     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
 
 # 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,26 +116,23 @@ 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
 
 @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

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_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 (