BUG: fix compatibility with numpy 2.0 for np.broadcast_arrays (now returns a tuple) #15930
Conversation
|
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.
|
|
👋 Thank you for your draft pull request! Do you know that you can use |
|
The following line makes my patch insufficient: astropy/astropy/utils/masked/core.py Line 905 in 97bf1e1 I'll come back to this after lunch ! |
5878c6d to
fef2ee8
Compare
|
I pushed a tentative workaround but I'm really not sure it'll work correctly on first try. I also note that I'm getting different errors locally and in CI, which may indicate that dev wheels are not caught up with numpy dev. |
6086c8e to
7245cca
Compare
astropy/utils/masked/core.py
Outdated
| @@ -902,7 +903,7 @@ def __array_function__(self, function, types, args, kwargs): | |||
| except NotImplementedError: | |||
| return self._not_implemented_or_raise(function, types) | |||
|
|
|||
| if not isinstance(dispatched_result, tuple): | |||
| if not isinstance(dispatched_result, FunctionHelperReturnTuple): | |||
There was a problem hiding this comment.
I see why you wrote it like this, but my sense is that really my design was just wrong. A dispatched function should just return a result, not possibly some combination - that should more logically be a different type of dispatched function.
I'll need to have a look at how often each case is used...
There was a problem hiding this comment.
In any case I think we should prioritise #15925, but yes, my patch is clearly not working as intended anyway so this might be a good time to re-evaluate the design here :)
There was a problem hiding this comment.
@mhvk is it okay if I drop the second commit for now ? I think it should make it clearer what problems still need solving.
There was a problem hiding this comment.
Yes, makes sense - let's fix the easy things while we think about the slightly harder ones!
7245cca to
7630878
Compare
7630878 to
64faaa9
Compare
|
@neutrinoceros - could you set the PR as "ready for review" - at least, I don't think I can do it and I cannot merge it as draft! |
|
sure, but maybe let's not auto-merge. I'm worried about the amount of failures that are still to be expected after this patch. |
|
As far as I can tell all new failures are caused by the harder design (#15930 (comment)). |
|
Let's get this one in! I'm reviewing a long numpy PR now, but will try to get back to the design question later. |
…np.broadcast_arrays (now returns a tuple)
…930-on-v6.0.x Backport PR #15930 on branch v6.0.x (BUG: fix compatibility with numpy 2.0 for np.broadcast_arrays (now returns a tuple))
|
@bsipocz reported failure in astroquery using astropy nightly wheel that I suspect is caused by this patch. Example log: https://github.com/astropy/astroquery/actions/runs/7651124450/job/20848349480?pr=2929 I asked her for clarification since this patch didn't trip our CI. If you are on Astropy Slack, link is https://astropy.slack.com/archives/C79U78JKT/p1706167941624919 |
|
I don't think the present patch caused these failures. In fact we're currently seeing similar failures on astropy devdeps, that are solved by #15948 |
|
Hmm... She was seeing a different set of failures until she picked up astropy nightly wheel with the change from this PR (see the Slack convo). |
|
Actually that's to be expected; we're seeing different failure modes but solving them iteratively, so it's highly confusing indeed; that's why we're not caught up with all failures from Monday yet, but we're getting there ! 😅 |
Description
This pull request is to address part of #15926
Currently based atop #15925xref numpy/numpy#25570