Bugfix for short version of np.nanmedian

[byrdie]

numpy/numpy@6ac4d6d changed numpy.lib.utils._median_nancheck() to use numpy.any() which is not supported by astropy.units.Quantity. This PR overrides nanmedian to try and deal with this properly.

Fixes #15225

[github-actions]

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?
• Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
• 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.

[mhvk]

@byrdie - thanks! But I fear it is not as simple. See inline comment...

[byrdie]

@mhvk, do you think overriding nanmedian is the right approach, or should we be focusing on changing the implementation in Numpy to avoid the use of any()?

[mhvk]

@byrdie - I had to find some time to test, but this is odd: one should not be able to get a boolean array with units. But looking in more detail, I see that the problem is that somewhere along the line the quantity gets turned into masked_Quantity, i.e., things are fed to np.ma.masked_array, and that's deadly. This happens in _nanmedian_small, in code that has been around since 2014. I think the commit you pointed to only happened to expose it.

Anyway, I think an override is appropriate here!

[byrdie]

@mhvk, yeah you can see in #15225 that I was also really surprised that we had managed to get a np.ma.masked_array wrapped around a Quantity, definitely not something we want to have happen.

[fdeugenio]

The deadly bit if i understand correctly is in numpy filled. This is the guy who takes our beautiful maske_Quantity and makes it a bool Quantity. This is only if the mask is everywhere False: filled returns a ndarray instead of a masked array. THis is also written in the docstring of the function. Maybe we should try to understand why they do this in the first place?
What's wrong with a masked array that has all masks=False? I find it weird that we get a different type for just this case.

[mhvk]

@fdeugenio - unfortunately, getting masked quantities using np.ma.MaskedArray is broken in far more places than that one. And the design is such that it really is not possible to fix it, which is why we ended up making our own Masked class -- see third difference in https://docs.astropy.org/en/latest/utils/masked/index.html#differences-from-numpy-ma-maskedarray

Numpy itself fortunately does not use its masked class much, so there rarely is a problem. Sadly, nanmedian is an exception!

[mhvk]

Looks great, only a minor note about the release note.

[mhvk]

py39-test-image-mpl334-cov failure surely unrelated!

[mhvk]

Great, let's get this in. Thanks, @byrdie, and sorry it turned out to be a bit more involved than we thought at the start!

[pllim]

Thanks! Looks like another one here: #15233

I am starting to think maybe we should pin numpy<2 for astropy 5.3.x (#15234) 🤔