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

mapreduce over & on empty collection differs depending on type #31635

Closed
oxinabox opened this issue Apr 6, 2019 · 6 comments
Closed

mapreduce over & on empty collection differs depending on type #31635

oxinabox opened this issue Apr 6, 2019 · 6 comments
Labels
fold sum, maximum, reduce, foldl, etc.

Comments

@oxinabox
Copy link
Contributor

oxinabox commented Apr 6, 2019

I don't think it makes sense for a empty generator to throw an error,
and an empty Array{Bool} to not.
Even more so, an empty Array{Any} to throw an error,
Particularly when such typing can come about only due to type-inference failing

This weirdness related to EltypeUnknown being treated differently to having an eltype of Any.
Perhaps in an ideal world, when type inference fails, the resulting Vector would have EltypeUnknown.
But that is not a realistic solution.

Related: #28028

Return True:

Arrays that are typed Bool or successfully type inferred Bool

mapreduce(identity, &, Bool[])
mapreduce(identity, &, [false for ii in 1:0])

Generators

Base.mapreduce(identity, &, (false for i in 1:0))
mapreduce(identity, &, (rand()>2 ? false : 4 for ii in 1:0))
mapreduce(identity, &, ((Bool, Int)[Int(rand()<2)](0)  for ii in 1:0))

Give Argument error about reducing over empty collection

Arrays not typed Bool

mapreduce(identity, &, Any[])
mapreduce(identity, &, Any[false for ii in 1:0])
mapreduce(identity, &, Exception[])

Type inference failures

mapreduce(identity, &, [rand()>2 ? false : 4 for ii in 1:0])
mapreduce(identity, &, [(Bool, Int)[Int(rand()<2)](0)  for ii in 1:0])
@JeffBezanson
Copy link
Member

mapreduce(identity, &, [rand()>2 ? false : 4 for ii in 1:0])

I don't see what this could do differently. Yes, if we allowed reducing & over any empty collection then these would work, but that has nothing to do with type inference.

We did this deliberately, since without a value to pass to & there is no well-defined notion of what it should return. true only makes sense if you're operating on bools.

Particularly when such typing can come about only due to type-inference failing

This is not caused by type inference failing (which it isn't here). Rather it's caused by people's insistence on using return_type, leading to these unpredictable effects. This is a more fundamental issue that is not the fault of downstream functions like reduce.

@oxinabox
Copy link
Contributor Author

oxinabox commented Apr 8, 2019

insistence on using return_type

Just so we are clear, you mean the @default_eltype fallback, right?
Which always(?) occurs for generators?

Or something else?

@JeffBezanson
Copy link
Member

Just so we are clear, you mean the @default_eltype fallback, right?

Yes.

@simonbyrne
Copy link
Contributor

Now only Arrays that are typed Bool will return true.

@mbauman
Copy link
Member

mbauman commented Jun 23, 2020

To be pedantic, it's all AbstractArray{Bool} that allow zero-element & | reductions. (You had me worried for a sec!)

I think this is getting closer to a sensible situation, and it largely mirrors what we do for other arithmetic reductions. For example:

julia> sum(Int[])
0

julia> sum(Float64[])
0.0

julia> sum((0 for i in 1:0))
ERROR: ArgumentError: reducing over an empty collection is not allowed

Now, one place where this still differs is in unions and other types. For example:

julia> reduce(+, Union{Missing, Bool}[])
false

julia> reduce(&, Union{Missing, Bool}[])
ERROR: ArgumentError: reducing over an empty collection is not allowed

This is a fairly limited case; reduce(+, Union{Nothing, Bool}[]) fails with a MethodError: no method matching zero(::Type{Union{Nothing, Bool}}) (#34003).

Since & and | are also quite well defined for integers, I initially thought it'd make sense to support them, too, but the return values get odd here: reduce(&, Int[]) == typemin(Int), but reduce(&, UInt[]) == typemax(UInt). This also means that we cannot possibly support abstract super types like reduce(+, Integer[]) does.

All that to say: this seems effectively complete. I'd say the only thing that remains would be that Union{Missing, Bool}.

@mbauman mbauman added the fold sum, maximum, reduce, foldl, etc. label Jun 23, 2020
@vtjnash
Copy link
Member

vtjnash commented Nov 23, 2021

While regrettable that this can depend on type-inference in creating the empty case, the consistent alternative is to always error (requiring the init parameter), which is even less user-friendly. See also triage's discussion outcome of the issues related to generator vs array eltype in #29919 (comment)

@vtjnash vtjnash closed this as completed Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fold sum, maximum, reduce, foldl, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants