Skip to content
This repository was archived by the owner on May 4, 2019. It is now read-only.

Commit

Permalink
Fix mapreduce() in a corner case
Browse files Browse the repository at this point in the history
mapreduce_seq_impl() does not exist in Base for a long time, but this went
unnoticed because since that code path is only used in very special conditions.
This bug probably comes from Base, where the code called the seq variant when
the pairwise one should rather have called itself recursively: also change that
for the path where there are NAs.
Another consequence of this bug is that the return type of
mapreduce_pairwise_impl_skipna() was inferred as Any.
Add a test which would have triggered the failure.

Also rename a variable to fix a type instability due to using it for two
different things.
  • Loading branch information
nalimilan committed Aug 2, 2017
1 parent 2103a21 commit eca6161
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 4 deletions.
8 changes: 4 additions & 4 deletions src/reduce.jl
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ function mapreduce_seq_impl_skipna(f, op, T, A::DataArray, ifirst::Int, ilast::I

while i < ilast
i += 1
@inbounds na = Base.unsafe_bitgetindex(chunks, i)
na && continue
@inbounds na_el = Base.unsafe_bitgetindex(chunks, i)
na_el && continue
@inbounds d = data[i]
v = op(v, f(d))
end
Expand All @@ -36,8 +36,8 @@ function mapreduce_pairwise_impl_skipna{T}(f, op, A::DataArray{T}, bytefirst::In
ifirst = 64*(bytefirst-1)+1
ilast = min(64*bytelast, length(A))
# Fall back to Base implementation if no NAs in block
return ilast - ifirst + 1 == n_notna ? Base.mapreduce_seq_impl(f, op, A.data, ifirst, ilast) :
mapreduce_seq_impl_skipna(f, op, T, A, ifirst, ilast)
return ilast - ifirst + 1 == n_notna ? Base.mapreduce_impl(f, op, A.data, ifirst, ilast) :
mapreduce_seq_impl_skipna(f, op, A, ifirst, ilast)
end

# Find byte in the middle of range
Expand Down
6 changes: 6 additions & 0 deletions test/reduce.jl
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,12 @@ end
@test sum(da2; skipna=true) sum(dropna(da2))
end

# Check that mapreduce with skipna=true works when a full block has no NA
da = DataArray(1:2049)
da[1] = NA
@test isna(sum(da))
@test sum(da, skipna=true) === 2100224

## other reductions

_varuc(x; kw...) = var(x; corrected=false, kw...)
Expand Down

0 comments on commit eca6161

Please sign in to comment.