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

Reland "add more methods and tests for reductions over empty arrays" #52004

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Nov 2, 2023

Relands #29919

@vtjnash vtjnash marked this pull request as ready for review April 16, 2024 19:31
@vtjnash vtjnash force-pushed the revert-52003-revert-29919-sb/reduce-empty branch from b87fdeb to 6ca85cd Compare April 16, 2024 20:48
@vtjnash
Copy link
Member Author

vtjnash commented Apr 16, 2024

Looks like this still runs into this SparseArray problem:

SparseArrays/sparsematrix_ops                    (10) |         failed at 2024-04-16T20:47:52.441
Test Failed at /home/vtjnash/julia/usr/share/julia/stdlib/v1.12/SparseArrays/test/sparsematrix_ops.jl:248
  Expression: minimum(sparse(Int[]))
    Expected: errchecker
  No exception thrown

Test Failed at /home/vtjnash/julia/usr/share/julia/stdlib/v1.12/SparseArrays/test/sparsematrix_ops.jl:249
  Expression: maximum(sparse(Int[]))
    Expected: errchecker
  No exception thrown

@mbauman mbauman added the fold sum, maximum, reduce, foldl, etc. label Sep 3, 2024
@mbauman
Copy link
Member

mbauman commented Sep 4, 2024

OK, I have two good examples of why I don't like these behaviors. With this patch:

julia> minimum(Union{Missing,Int}[])
ERROR: MethodError: no method matching typemax(::Type{Union{Missing, Int64}})

julia> minimum(abs, Union{Missing,Int}[])
9223372036854775807

julia> maximum(Union{Missing,Int}[])
ERROR: MethodError: no method matching typemin(::Type{Union{Missing, Int64}})

julia> maximum(abs, Union{Missing,Int}[])
0

While I think it makes some sense that zero(Union{Missing, Int}) would be 0, I don't find it near as defensible that typemin or typemax should work — with or without an abs thrown into the mix. It seems like maximum(abs, Union{Missing,Int}[]) could be either missing or 0.

It also just doesn't seem like this is necessarily true in the abstract — for example:

julia> maximum(abs, Int8[])
0

julia> maximum(abs, Int8[-128]) # But there _does exist_ a lesser least element!
-128

That last example doesn't even require this patch — that's the current behavior of Julia. Still bad, though, and representative of what this patch wants to do more of.

@adienes
Copy link
Contributor

adienes commented Sep 4, 2024

julia> maximum(identity, [])
ERROR: MethodError: reducing over an empty collection is not allowed; consider supplying `init` to the reducer

julia> maximum(abs, [])
ERROR: MethodError: no method matching zero(::Type{Any})

julia> maximum(abs, Int8[])
0

😬 that's unfortunate.

maybe that can be fixed in your PR which is already slightly breaking? after all (slightly breaking ---> slightly more breaking) is a much smaller jump than (not breaking ---> slightly breaking)

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 this pull request may close these issues.

3 participants