Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reduce with &/| over empty arrays #31837

Closed
simonbyrne opened this issue Apr 25, 2019 · 5 comments
Closed

reduce with &/| over empty arrays #31837

simonbyrne opened this issue Apr 25, 2019 · 5 comments

Comments

@simonbyrne
Copy link
Contributor

As pointed out by @oxinabox, calling reduce(&, X) is inconsistent when X is an empty collection:

julia> reduce(&, (ii for ii in 1:-1))
true

julia> reduce(&, collect((ii for ii in 1:-1)))
ERROR: ArgumentError: reducing over an empty collection is not allowed
Stacktrace:
 [1] _empty_reduce_error() at ./reduce.jl:216
 [2] reduce_empty(::Function, ::Type) at ./reduce.jl:226
 [3] mapreduce_empty(::typeof(identity), ::Function, ::Type) at ./reduce.jl:251
 [4] _mapreduce(::typeof(identity), ::typeof(&), ::IndexLinear, ::Array{Int64,1}) at ./reduce.jl:305
 [5] _mapreduce_dim(::Function, ::Function, ::NamedTuple{(),Tuple{}}, ::Array{Int64,1}, ::Colon) at ./reducedim.jl:308
 [6] #mapreduce#548 at ./reducedim.jl:304 [inlined]
 [7] mapreduce at ./reducedim.jl:304 [inlined]
 [8] #reduce#549 at ./reducedim.jl:348 [inlined]
 [9] reduce(::Function, ::Array{Int64,1}) at ./reducedim.jl:348
 [10] top-level scope at none:0

this is caused by defining it differently depending on whether the element type is known or not:

julia/base/reduce.jl

Lines 262 to 265 in 93c9ae4

mapreduce_empty_iter(f, op, itr, ::HasEltype) = mapreduce_empty(f, op, eltype(itr))
mapreduce_empty_iter(f, op::typeof(&), itr, ::EltypeUnknown) = true
mapreduce_empty_iter(f, op::typeof(|), itr, ::EltypeUnknown) = false
mapreduce_empty_iter(f, op, itr, ::EltypeUnknown) = _empty_reduce_error()

Two possible solutions:

  1. Get rid of the mapreduce_empty_iter definitions for & and | (so both examples above would error), or
  2. Define reduce_empty(::typeof(&), T) = true and reduce_empty(::typeof(|), T) = false (so both examples above would return true). The main problem with this is that true isn't strictly the identity element under &, since
julia> true & 2
0

we could of course resolve this by redefining

(&)(b::Bool, x::Integer) = b ? x : zero(x)

but that might be even more complicated

@ararslan
Copy link
Member

+1 to making them both an error.

julia> reduce(&, (ii for ii in 1:-1))
true

feels pretty nonsensical to me.

@simonbyrne
Copy link
Contributor Author

simonbyrne commented Apr 25, 2019

we could of course resolve this by redefining

(&)(b::Bool, x::Integer) = b ? x : zero(x)

On second thoughts, that would be even weirder, since then we would break the distributive law:

julia> (true | 0) & 2
0

julia> (true & 2) | (0 & 2)
2

we could resolve that by defining

(|)(b::Bool, x::Integer) = b ? reinterpret(typeof(x), typemax(unsigned(x))) : x

(i.e. x | true gives the value with a bit pattern of all ones), but you couldn't make that work for BigInts (since no such value exists), and I'm sure would break other things.

@oxinabox
Copy link
Contributor

Same issue but for mapreduce: #31635

@KristofferC
Copy link
Sponsor Member

Seems fixed?

julia> reduce(&, (ii for ii in 1:-1))
ERROR: ArgumentError: reducing over an empty collection is not allowed

@simonbyrne can be closed?

@simonbyrne
Copy link
Contributor Author

Looks like it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants