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

Support init keyword in maximum/minimum #35839

Closed
wants to merge 1 commit into from
Closed

Conversation

timholy
Copy link
Member

@timholy timholy commented May 11, 2020

We can supply init for general reductions, but maximum and minimum do not support this keyword. This PR allows them to do so:

julia> maximum([1,2,3])
3

julia> maximum(Int[])
ERROR: ArgumentError: reducing over an empty collection is not allowed
Stacktrace:
 [1] _empty_reduce_error() at ./reduce.jl:299
 [2] reduce_empty(::Function, ::Type{T} where T) at ./reduce.jl:309


julia> maximum(Int[]; init=-1)
-1

This was motivated by writing a blog post on invalidations (JuliaLang/www.julialang.org#794), and together with some changes to Pkg to supply the init keyword it eliminates all of the reduce_empty invalidations from loading FixedPointNumbers (which extends reduce_empty).

It's worth noting one unfortunate fact: init has different meanings for reduce(+, src) than for sum!(dest, src). In the former case, it's the value for seeding the result; in the latter, it's a boolean indicating whether to re-initialize dest to the sentinel value or to keep whatever result is already there. This PR might make the situation slightly worse by supporting more cases where the conflict becomes appreciable (maximum vs maximum!). This feels like something we should fix for Julia 2.0.

Closes #35733

Copy link
Member

@tkf tkf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code LGTM but it looks like the doctest failed in CI.

It's worth noting one unfortunate fact: init has different meanings for reduce(+, src) than for sum!(dest, src).

Yes, it's also unfortunate that the meaning of init is slightly different in foldl/accumulate compared to those two families.

This feels like something we should fix for Julia 2.0.

I think we can introduce new keyword arguments (and, technically, also even deprecate init) in 1.x. But I guess I should open a new issue for discussing this.


_mapreduce_dim(f, op, ::NamedTuple{()}, A::AbstractArrayOrBroadcasted, ::Colon) =
_mapreduce_dim(f, op, ::_InitialValue, A::AbstractArrayOrBroadcasted, ::Colon) =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 That's what we did in JuliaArrays/StaticArrays.jl#750

($_fname)(a, ::Colon) = ($_fname)(identity, a, :)
($_fname)(f, a, ::Colon) = mapreduce(f, $op, a)
($_fname)(a, ::Colon; kw...) = ($_fname)(identity, a, :; kw...)
($_fname)(f, a, ::Colon; kw...) = mapreduce(f, $op, a; kw...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use init as a positional argument for $_fname. I guess that would shave off some compiler work (and avoid it to get in the way of the inliner)?

@tkf tkf closed this Jun 8, 2020
@tkf tkf reopened this Jun 8, 2020
@tkf tkf closed this in #36188 Jun 11, 2020
@DilumAluthge DilumAluthge deleted the teh/reduce_init branch March 25, 2021 22:04
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

Successfully merging this pull request may close these issues.

2 participants