Conversation
tkf
left a comment
There was a problem hiding this comment.
The code LGTM but it looks like the doctest failed in CI.
It's worth noting one unfortunate fact:
inithas different meanings forreduce(+, src)than forsum!(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.
| mapfoldl_impl(f, op, nt, A) | ||
|
|
||
| _mapreduce_dim(f, op, ::NamedTuple{()}, A::AbstractArrayOrBroadcasted, ::Colon) = | ||
| _mapreduce_dim(f, op, ::_InitialValue, A::AbstractArrayOrBroadcasted, ::Colon) = |
There was a problem hiding this comment.
👍 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...) |
There was a problem hiding this comment.
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)?
We can supply
initfor general reductions, butmaximumandminimumdo not support this keyword. This PR allows them to do so: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
initkeyword it eliminates all of thereduce_emptyinvalidations from loading FixedPointNumbers (which extendsreduce_empty).It's worth noting one unfortunate fact:
inithas different meanings forreduce(+, src)than forsum!(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-initializedestto 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 (maximumvsmaximum!). This feels like something we should fix for Julia 2.0.Closes #35733