perf: add specializations for mapreduce min/max

[pfackeldey]

This PR add specializations for mapreduce(maximum, max, V) and mapreduce(minimum, min, V) to directly dispatch to maximum(V.data) and minimum(V.data) and thus avoiding allocating intermediates.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.21%. Comparing base (021083f) to head (2dd33ca).

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #48      +/-   ##
==========================================
+ Coverage   96.20%   96.21%   +0.01%     
==========================================
  Files           8        8              
  Lines         527      529       +2     
==========================================
+ Hits          507      509       +2     
  Misses         20       20              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[oschulz]

Thanks @pfackeldey ! Could you add this specialization for ArrayOfSimilarArrays too?

Also,, should we add specializations for Base.mapreduce(::typeof(sum), ::typeof(+), ...)?

[Moelf]

ArrayOfSimilarArrays

it would be useful to have a table at the top of documentation to show how the types are related to each other.

Is this type disjoint from ArrayOfArrays?

Also,, should we add specializations for Base.mapreduce(::typeof(sum), ::typeof(+), ...)?

yeah seems useful...

[oschulz]

it would be useful to have a table at the top of documentation to show how the types are related to each other.

Yes, good idea!

Is this type disjoint from ArrayOfArrays

We don't have a type ArrayOfArrays in the package, but ArrayOfSimilarArrays and VectorOfArrays are disjoint.

[oschulz]

Does it make sense to forward the kwargs to minimum here? The kwargs of mapreduce and minimum are not compatible, right?

[Moelf]

yeah technically they don't completely overlap, however, the problem is we need init because other wise there's no way to make reducing over empty arrays work

[pfackeldey]

would you in this case rather curry minimum to a minimum(..., init=0) and put this in mapreduce? and that hopefully still works because the typeof(minimum) would still trigger on the curried function?

I don't know how this would look like in Julia, but in Python it would be something like the following pseudo-code:

minimum_with_init = partial(minimum, init=0)

mapreduce(minimum_with_init, min, A)

I'm not sure how multiple dispatch works with curried functions, and not sure if this is a common solution/paradigm in Julia.

[oschulz]

I think it might be best to declare and handle the kwargs explicitly.

As for the minimum with init, I doubt many users would write their code like that ...

[Moelf]

I think it might be best to declare and handle the kwargs explicitly.

there's no good default for init=, so if user doesn't provide one, we also don't

[pfackeldey]

done in ffe6030

[oschulz]

On generic arrays the default implementation of mapreduce is

mapreduce(f, op, A::AbstractArrayOrBroadcasted; dims=:, init=_InitialValue()) =
_mapreduce_dim(f, op, init, A, dims)

but on GPU arrays it is

Base.mapreduce(f, op, A::AnyGPUArray, As::AbstractArrayOrBroadcasted...; dims=:, init=nothing)

So handling of the init default differs, and _InitialValue is not public. it would be nice of there was an official "default init" construct, but there isn't, so we have to get a bit creative.

Also, we can't just pass a dims kwarg that is not : through to the aggregation function on the flattened collection, it would result in incorrect results in many cases.

I suggest we do something like this (untested!):

using Base: add_sum, mul_prod

for (f, op) in [
    (:sum, :add_sum), (:product, :mul_prod), (:maximum, :max), (:minimum, :min)
]
    for AoA in [:AbstractArrayOfSimilarArrays, :AbstractVectorOfArrays]
        @eval begin
            Base.mapreduce(::typeof($f), ::typeof($op), A::$AoA; dims = :, init = nothing)
                = _aoa_associative_mapreduce($f, $op, $AoA, init)
        end
    end
end

_aoa_associative_mapreduce(f, ::Any, A, ::Colon, ::Nothing) = f(flatview(A))
_aoa_associative_mapreduce(f, ::Any, A, ::Colon, init) = f(flatview(A); init = init)

function _aoa_associative_mapreduce(f, op, A, dims, init) =
    # Wrap `f` to dispatch to generic mapreduce implementation for `dims!=:`:
    wrapped_f = x -> f(x)
    return maprecduce_impl(x -> f(x), op, A, dims = dims, init = init)
end

[Moelf]

@oschulz I think we're gonna give up making PR now 😂

[oschulz]

Well, we have to do it somewhat cleanly, esp. the dims handling.

[Moelf]

there's no dims to handle if we're just specializing VectorOfX, because vector is 1D

let's do that first I guess