ENH: implement np.minmax - #32231

I don't know what the above message is.

I don't know what the above message is.

This seems to be a mypy bug that's related to #32215 and only occurs for certain mypy configurations in there's a assignment to a variable called _. I'll open a mypy issue for this.

TLDR; it's unrelated

@ngoldbaum I think this should be ready for a quick pass as well.

@seiko2plus do you have any thoughts on the SIMD here and extending our current .c.src templating?

Maybe @mhvk has an opinion?

IMO this is fine and the simplest way to do this. It does make it marginally more complicated for someone else to eventually move away from .c.src templating but it also doesn't leave us in a weird half-converted state either.

I'd argue that this was the simplest way to do this. We want np.minmax to have identical behavior to return np.min(...), np.max(...) in all cases so adding the loops right below where the minimum/maximum loops are defined using the same optimizations as them in .src files, creating the ufunc using the legacy way to have it implicitly get the legacy dtype promotion too, and finally using only a small c++ file just to add the reduction loops to the already created ufunc seemed like the simplest and shortest way to get this to me.

@ngoldbaum - On the string dtype (and potentially for other user dtypes) how about we add a generic fallback to the minmax python wrapper that just does return np.min(...), np.max(...) if the minmax loops raises such a type error? Such a thing would also cover other user dtypes that implement min and max but not minmax. Then I can do string dtype loops in an immediate follow-up PR and increase coverage in test_strings.py for string dtype in particular.

For masked arrays, I'd argue to not add a minmax method, but we can simply add np.ma.minmax that just does return np.ma.min(...), np.ma.max(...) unless you are worried about the fact that np.min now works properly for masked arrays (because it uses the ndarray method) and similarly want np.minmax to work properly for masked arrays but I'd argue that this is an asymmetry because ndarrays have no minmax method here so it would probably be best to not have one for masked arrays too. If you definitely want np.minmax to properly dispatch to the masked array one for masked arrays, I'd say we add minmax as a method on the ndarray then but we did not want such a thing originally and that automatic dispatching unfortunately only works for things that have corresponding ndarray methods. In any case, I don't think we should ever write minmax mask-aware loops for masked arrays.

A generic fallback for stringdtype and user dtypes would be nice!

Fair enough on masked arrays.

Agreed with @ikrommyd to just add a np.ma.minmax function - that is how it works for much of the rest of the numpy functions, and thus leaves things in a consistent state (until someone gets around to implement MaskedArray.__array_ufunc__ and __array_function__...)

For masked arrays, I'd argue to not add a minmax method, but we can simply add np.ma.minmax that just does return np.ma.min(...), np.ma.max(...)

Should we really be adding new np.ma functionality though 😅?

For masked arrays, I'd argue to not add a minmax method, but we can simply add np.ma.minmax that just does return np.ma.min(...), np.ma.max(...)

Should we really be adding new np.ma functionality though 😅?

It's good for consistency and it's literally a one-liner function. The docstring what we're adding effectively to the review.

For masked arrays, I'd argue to not add a minmax method, but we can simply add np.ma.minmax that just does return np.ma.min(...), np.ma.max(...)

Should we really be adding new np.ma functionality though 😅?

It's good for consistency and it's literally a one-liner function. The docstring what we're adding effectively to the review.

I'm more worried that when we put it in the relnotes, it might lead to people thinking that it's a good idea to use np.ma 😛

We can sneak it in the single release note for numpy.minmax like I have it now lol. Unfortunately people do use masked arrays regardless. We are not giving them any benefits here apart from the API anyways.

I believe I have addressed this round of review comments from everyone.

Hi @ikrommyd - On the top-level API question - np.minmax would definitely be helpful. Bounding boxes and plot axis limits both need min and max on the same array and currently that's two separate calls walking the data twice. Even more useful on large datasets. A nanminmax version alongside np.minmax would be useful too. If minmax ships without it, users will hit the gap immediately and end up writing data[~np.isnan(data)] every time.

Thanks @seberg! I think I did what you wanted in 76eefc1
I also rebased to be on top of main and found a build failure that's now fixed.

Yap, thanks, fly-by fixes also looks right. Let's merge when CI is done and then follow-up if necessary :).

Did anybody benchmark on how much of a difference this makes?

Did anybody benchmark on how much of a difference this makes?

@ikrommyd did but I can't find a public link offhand.

However it's pretty easy to find cases that are wins:

In [1]: arr = np.random.random(100_000_000)

In [2]: %timeit (np.min(arr), np.max(arr))
17.7 ms ± 133 μs per loop (mean ± std. dev. of 7 runs, 100 loops each)

In [3]: %timeit np.minmax(arr)
9.15 ms ± 56.2 μs per loop (mean ± std. dev. of 7 runs, 100 loops each)

In [4]: (np.min(arr), np.max(arr))
Out[4]: (np.float64(2.454591385703253e-08), np.float64(0.9999999596284977))

In [5]: np.minmax(arr)
Out[5]: (np.float64(2.454591385703253e-08), np.float64(0.9999999596284977))

In [6]: arr = np.random.random(12)

In [7]: %timeit (np.min(arr), np.max(arr))
457 ns ± 1.33 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

In [8]: %timeit np.minmax(arr)
299 ns ± 4.5 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

In fact, I think we checked at one point and couldn't find any cases where it isn't a win to use minmax if you really do need both min and max.

@GniLudio - Yeah I had done some benchmarks. it's 1.25x to 2x better usually and never worse. Think about it this way, your core computation does not change, you still need to accumulate the min and max. What changes is how often you pass over the array and read it's data. If your operation is bottleneck is reading from memory (you are reducing along a contiguous axis), then the boost is around 2x cause you reduced the reading by half. If your bottleneck is actually calculating the min and max, then the speed up is much smaller at around 25%. These are usually platform dependent too and dependent on what instructions your cpu can run. If you find a case that it isn't faster, it's almost surely a bug that we should fix.

@GniLudio - Yeah I had done some benchmarks. it's 1.25x to 2x better usually and never worse. Think about it this way, your core computation does not change, you still need to accumulate the min and max.

I was just surprised to not see any mentions of benchmarking results. It's always easy to assume that something is faster, just to find out that it's not. Glad to see that you actually tested it and got the expected speedups.

Very cool! Thank you for working on this Iason. Also thank you everyone who helped review 🙏

Sorry for commenting on a merged PR. Is it usual to add a .. versionadded:: to the docstring for additions like this? End users often need to know in what version of the package functions were added.

I thought it usually is for features on an existing function. Will make a PR.

Edit: #32607