BUG: support numpy's mimimummaximum ufunc - #20391

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

• Do the proposed changes actually accomplish desired goals?
• Do the proposed changes follow the Astropy coding guidelines?
• Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
• Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
• Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
• Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
• Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
• Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
• At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

Tests are passing locally. I expect to be able to undraft this when CI completes.

Whoops, I forgot about masked.

I added test coverage for np.minmax on the masked side but it doesn't work out of the box. It's also not 100% clear how to implement support. An obvious approach would be to re-use np.min and np.max internally, which would output a correct result, at the cost of violating the promise that np.minmax only scans the input array once.

FYI in numpy/numpy#32577 we discovered some edge cases that np.minmax doesn't handle how we'd like it too regarding legacy user dtypes and __array_ufunc__. I don't know if that affects you here, I'm just mentioning it.

@neutrinoceros - I think you're running into the fact that, by default, a masked reduction always or's all the masks together. This is reasonable for some reductions (like np.add.reduce, np.multiply.reduce, np.logical_or.reduce, etc., where refusing the temptation to guess that a masked element should be 0, 1, or False makes sense), but not for ones where one is effectively selecting items like np.minimum.reduce and np.maximum.reduce. For np.min and np.max, this is avoided by implementing the methods that those will defer to, MaskedNDArray.min and MaskedNDArray.max, each of which create a default where=~self.mask via self._reduce_defaults.

So, I think for np.minmax, we need to add a function override that similarly creates defaults for where and initial.

Note that with that in place, we can also update the ptp method to make use of it (actually, that method should really be deprecated, but that would be for another PR...)

@mhvk yeah in numpy we chose on the PR to not add yet another method to the ndarray. Would you argue that making minmax a method too is helpful? We had settled on doing np.minmax and np.ma.minmax where np.minmax has no knowledge of the mask and will just ignore it as there is no method it can call. But np.ma.minmax is not a fused kernel. It's just an API helper that delegates to separate masked min and max calculations.

@ikrommyd - thanks for wondering! No, I don't think np.minmax needs a method -- while it is a nice addition, I don't think it is common enough to be a method (I similarly think it was a good idea to deprecate and remove ndarray.ptp()). I mentioned it above only because it explains why in our __array_function__ implementation we do not need to override np.min and np.max, but will need to override np.minmax.

think for np.minmax, we need to add a function override that similarly creates defaults for where and initial.

I don't think I understand how np._core.umath.minmummaxim.reduce relates to np.minmax. In fact I already don't understand the implementation details for MaskedNDArray's min and max method and why they are passing reduce kwargs to their superclass (presumably just ndarray ?).

think for np.minmax, we need to add a function override that similarly creates defaults for where and initial.

I don't think I understand how np._core.umath.minmummaxim.reduce relates to np.minmax. In fact I already don't understand the implementation details for MaskedNDArray's min and max method and why they are passing reduce kwargs to their superclass (presumably just ndarray ?).

np._core.umath.minmummaximum is the ufunc whose reduce method is np.minmax at the top level api

@neutrinoceros - the .max() and .min() methods pass where=~self.mask (as well as an initial value) so that in the reduction any masked elements get ignored (and their mask thus also gets ignored).

Thank you both. I think I have a mostly¹ correct implementation now:

• test_no_duplicates is failing
• it's not called in my test, which I'm sure has the same root cause, but I'm not seeing what's used instead