Add dropdims(f, args..; dims, kwargs..) for reductions to drop dims

[nickrobinson251]

• attempts to address array reductions (sum, mean, etc.) and dropping dimensions #16606 (with @StefanKarpinski's suggestion array reductions (sum, mean, etc.) and dropping dimensions #16606 (comment))
• builds on @mbauman's earlier work (Add squeeze(f, A, dims) for reductions to drop dims #23500)

[nickrobinson251]

As far as I can tell, test failures are unrelated 🤞

[fredrikekre]

Docstring needs to be included in the manual, and also add a compat annotation.

[nickrobinson251]

Docstring needs to be included in the manual

thanks for the help!

We already have Base.dropdims in the api docs (under Base/Arrays/Views)
https://github.com/JuliaLang/julia/blame/master/doc/src/base/arrays.md#L116
Do I need to add anything here?

We don't have anything in the Manual section of the docs - which I think is what you meant - where should I add something?

[fredrikekre]

We already have Base.dropdims in the api docs (under Base/Arrays/Views)

Okay, thats what I meant. I thought this was a new name.

[nickrobinson251]

pinging a few people who may be interested and (time allowing) well placed to review or recommend someone to review: @StefanKarpinski @mbauman @oxinabox @tpapp

[tpapp]

I am wondering if making dims a required argument to dropdims (instead of a keyword one) would make more sense. It is unlikely that one would use this function without it. But keeping it like this is also fine with me.

FWIW, I think the right approach is something like https://github.com/bramtayl/JuliennedArrays.jl, but that can live in a library.

[oxinabox]

I am wondering if making dims a required argument to dropdims

This matches current methods for dropdims
Also in general we moved a lot of dims from positional argument to kwargs in the julia 0.7
and I think it was an improvement

[nickrobinson251]

yes, both dropdims(rand(1, 2)), dropdims(sum, rand(1, 2)) will throw an informative UndefKeywordError saying that dims is not assigned.

[StefanKarpinski]

Marking for triage to have a last discussion about if this is the API we want. I kind of pushed this one through and I want others to have a chance to chime in before merging.

[tpapp]

I have nothing against this solution per se, but is there a compelling reason it has to live in Base, instead of a package?

I am asking because putting it in Base will make future incompatible changes impossible until 2.0.

[nickrobinson251]

is there a compelling reason it has to live in Base, instead of a package?

dropdims already exists in Base, this just a one-line addition to allow it to take a reduction as a first argument. And from the sounds of #16606 lots of people would find this helpful.

[JeffBezanson]

I agree we probably want something like this but this API seems a bit odd. dropdims(sum, abs2, a, dims=2) in particular is kind of a strange call. Both @Keno and I thought it might make more sense for dropdims to take a 2-argument function to reduce with, so that it's a drop-in (ha ha) replacement for reduce.

[Keno]

I had also suggested that dropdims(sum)(rand(1, 2); dims=1) would be a possible alternative, but @JeffBezanson thought that was too weird.

[nickrobinson251]

Yeah, I can see how the API is not ideal... If the implementation weren't so trivial perhaps we wouldn't choose it. Personally I'm not sure dropdims(sum)(...) is less odd. But I've no strong opinions here.

it might make more sense for dropdims to take a 2-argument function to reduce with

I am not sure what exactly this would look like

[tkf]

I think functions with keyword arguments are hard to compose (see also discussion on keyword argument weights #33310). Why not define a light-weight wrapper type that propagates dims information in a type stable manner? In #16606 I explained my PoC which can do dropdims(sum(along(x, 1, 2))) or dropdims(sum(along(x, &, &, :))) for dropdims(sum(x; dims=(1, 2)); dims=(1, 2)).

[nickrobinson251]

I dunno how best to move forward with this. I feel like we're settled on wanting this functionality and the question is "do we want this pleasingly trivial implementation at the cost of a merely-okay / not-beautiful API?". Feel like a question for @StefanKarpinski / triage more generally?

[StefanKarpinski]

We'll discuss on triage this week, which should be Thursday at 2:15 US Eastern.

[StefanKarpinski]

Notes from triage.

From @StefanKarpinski: one of the advantages of dropdims(sum)(a, dims=1) form is that you can pass a do block still:

dropdims(sum)(a, dims=1) do x
    # transform values here
end

From @vtjnash: he was somewhat surprised that dropdims requires the reducing function to take the dims keyword, as he would have expected it to implement dimension dropping for you, which would also be possible.

[StefanKarpinski]

Another observation from @JeffBezanson and @vtjnash is that while the argument was that it would be annoying to go around and add the drop keyword to all reducers, the same argument can be applied retroactively to the dims keyword: why did we do all the annoying work to add that keyword to all the reducers? Why didn't we have a more composable mechanism for turning any reducer into an operation that reduces along certain dimensions? Then the dropdims construct would just be a slight variation on that general construct. I think this is kind of what @tkf's comment may be getting at as well. Of course, we can't deprecate the dims keywords now, but perhaps we should use this is an API design guide and instead of implementing dropdims assuming that the dims keyword to exist, implement keepdims and dropdims if you will, assuming just a reducer.

[nickrobinson251]

Why didn't we have a more composable mechanism for turning any reducer into an operation that reduces along certain dimensions. Then the dropdims construct would just be a slight variation on that general construct... instead of implementing dropdims assuming that the dims keyword to exist, implement keepdims and dropdims if you will, assuming just a reducer.

I think this is a great point/idea!

I just wanted to say that personally i don't forsee having any time soon to work on a bigger, better versions of this PR. But i'd love it if someone else was excited about running with this idea.

[StefanKarpinski]

No worries, @nickrobinson251. Thanks for the PR—sometimes these things are deeper than they may appear at first and time seems to be the only recipe for understanding them better.

[AzamatB]

Bump, I want this. It would make my codebase less ugly.

[nickrobinson251]

closing, as i've no time to work on this (someone else is very welcome to take over if they see some value here), but also in favour of #32310