ENH: Add where argument to np.mean #15852

sgasse · 2020-03-27T21:18:29Z

Harmonize the signature of np.mean with np.sum by adding a where
argument, closes gh-15818.

numpy/core/_methods.py

numpy/core/tests/test_multiarray.py

numpy/core/_methods.py

eric-wieser · 2020-03-28T22:44:33Z

Thanks for the var support, this looks surprisingly straightforward.

numpy/core/_methods.py

numpy/core/fromnumeric.py

eric-wieser · 2020-03-29T09:10:44Z

numpy/core/_methods.py

We should check which cases ufunc.reduce (c function name is something like GenericReduction) has this optimization for, and try the same ones. In particular, I'm trying to remember if we optimized for np.bool_.

I found the PyArray_GenericReduceFunction in numpy/core/src/multiarray/number.c however that function looks more like a wrapper, being only 15 lines long. Could you point me in the right direction?

I think @eric-wieser is indicating at this function : PyUFunc_GenericReduction in numpy/core/src/umath/ufunc_object.c . I wasn't able to find any special casing for np.bool except that where = True is being treated as if no where is provided.

OK thank you for checking @anirudh2290.

numpy/core/_methods.py

numpy/core/fromnumeric.py

eric-wieser · 2020-04-28T07:03:59Z

numpy/core/_methods.py

This doesn't do the right thing for arrays containing np.inf. I recommend we skip ptp for now, and attempt it in a future PR - it's sufficiently different from mean and var that I think it's worth handling separately, so that we can get the rest merged.

Alright, I will remove changes made to ptp and upload a commit without it.

sgasse · 2020-04-28T18:22:10Z

@eric-wieser I removed all changes related to ptp. Please let me know if other changes are required before the commit can be merged. Looking at the failed checks, it appears to me that they are unrelated to my changes. However I build locally on Linux and cannot test a build on Windows.

seberg · 2020-05-14T21:37:01Z

numpy/core/_methods.py

The kwarg will have some performance implications for small arrays <1000 elements or so, until my parsing improvements would be merged. But just a note, to be honest there is not much we can do about it. It seems passing by position is thwarted by the initial argument (though that could possibly be changed).

@eric-wieser do you want to persue this for 1.19? I will push it off for now.

sgasse · 2020-05-22T08:18:39Z

@eric-wieser would you have time to take a last quick glance at this so it could maybe go in?

seberg

Hmm, sorry for not moving this forward earlier. @eric-wieser you were happy with this? It seems good to me, would do another more careful path before merging maybe, but seems all good.

@sgasse would you be willing to add an improvement release note for this? since these are all reduction like functions, adding an argument that all reductions accept seems like a straight forward API decision.

numpy/core/fromnumeric.py

sgasse · 2020-07-17T13:57:42Z

Hey @seberg , nice to hear back on this pull request :)
Sure, later/tomorrow, I can update all the versionadded tags in my changes. Where exactly can I add the improvement release note?

seberg · 2020-07-17T15:42:32Z

Thanks, releaes notes would go into doc/release/upcoming_changes, there is a readme in that file detailing the process.

Harmonize the signature of np.mean, np.var np.std, np.any, np.all, and their respective nd.array methods with np.sum by adding a where argument, see numpygh-15818.

sgasse · 2020-07-18T11:19:31Z

Alright, thank you @seberg for the location of of the release notes. I added a document there and updated all the versionadded tags. Maybe you can have a look at the release note document.

seberg · 2020-08-06T00:22:32Z

Sorry, I was AFK for a bit. The release notes seem long but fine. @eric-wieser if there was anything you felt still doing here please have a quick look. I somewhat remembered that you almost had signed this off. But maybe that was not the case.

As a general note, the slowdown is:

In [2]: %timeit np.mean(arr)                                                                                            
10.8 µs ± 575 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

vs.

In [3]: %timeit np.mean(arr)                                                                                            
17.7 µs ± 132 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

For arr.any() the difference is 2.4µs vs. 3µs after. Since this adds a reduce argument to reduce-like operations. I am generally in favor of adding this, but it is a fairly large speed impact for small arrays there with mean, and I am uncertain that it will be easy to micro-optimize away.

charris · 2020-11-21T17:26:50Z

Ping everyone here. Sounds like this is ready.

seberg · 2020-11-21T17:57:17Z

I am OK with putting it in as is. The only thing that made me pause in the first place was the slowdown, which is a silly reason probably.

sgasse · 2020-11-22T08:33:02Z

@charris @seberg nice to hear back - should I rebase this or is it fine as is? I see the next major version is still 1.20, so this does not need to be updated, right?

charris · 2020-11-22T16:42:48Z

In it goes, thanks @sgasse .

eric-wieser reviewed Mar 28, 2020

View reviewed changes