BUG: support numpy's unwrap ufunc - #20132

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.

Nightlies are not caught up yet. Pending.

@mhvk by "override", do you mean the existing @dispatched_function implementation ?

@neutrinoceros - yes, I think if we support the new _unwrap ufunc, we should no longer need to dispatch on np.unwrap itself (but of course numpy version specific...). For us, not necessarily really better, but it helps ensure numpy treats subclasses as well as possible...

FWIW I see the ufunc failure in devdeps now, so would be nice if we can wrap this up for merging. Thanks, all!

I'll wrap this up tomorrow

I found a single precedent for this situation (a new ufunc)
ref 1c6fca3

however, I'm still not confident in my fix:

• I do not remember exactly how helper functions are supposed to work (nor did I find the time to properly dig into it)
• my test seems to pass even if the helper isn't registered (bad), so I'm not trusting that my implementation is correct
• I'm not sure what to do with the array_function override. Removing it leads to serious-looking failures so I'm keeping it for the time being, but I don't think I understand how numpy behaves when both the override and the helper are registered.

my test seems to pass even if the helper isn't registered

my test was definitely incorrect. I removed it for now. Of course I'm even less confident in the patch now 😅

The devdeps is green (I guess it ran before #20164 happened), so do we proceed despite the decrease in confidence or are we pursuing an alternative fix? Hope you can advise. Thanks!

It's green but the helper isn't covered. I'd like to get feedback from @mhvk before moving forward with this.

@nstarman also would need to re-review because he, too, requested changes.

Thanks, all!

I'm having a look...

OK, I added the code to properly support the ufunc. Unfortunately, actually letting Quantity pass through from np.unwrap is a bit tricky, since the defaults for discont and period are dimensionless by default, and in the helper I have no access to their values, so would need to assume anything dimensionless can also be in radian. It seemed easier to just keep the np.unwrap override in place, which can deal with this a little better.

Anyway, bottom line, we now support a gufunc that nobody is likely to call with Quantity inputs...

Thanks a lot Marten ! I would self-approve too if authors were allowed to.
I also note that @nstarman's request for change was addressed a couple iterations back.

Looks like for v7.2.x, we either have to max pin numpy or manually backport this. See #20170