Skip to content

BUG: fix compatibility with numpy 2.0 for np.broadcast_arrays (now returns a tuple)#15930

Merged
mhvk merged 1 commit intoastropy:mainfrom
neutrinoceros:tst/numpy2/broadcast_arrays
Jan 24, 2024
Merged

BUG: fix compatibility with numpy 2.0 for np.broadcast_arrays (now returns a tuple)#15930
mhvk merged 1 commit intoastropy:mainfrom
neutrinoceros:tst/numpy2/broadcast_arrays

Conversation

@neutrinoceros
Copy link
Copy Markdown
Contributor

@neutrinoceros neutrinoceros commented Jan 23, 2024 •
edited
Loading

Description

This pull request is to address part of #15926
Currently based atop #15925

xref numpy/numpy#25570

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jan 23, 2024

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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jan 23, 2024

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

This was referenced Jan 23, 2024
Closed
Closed
@neutrinoceros
Copy link
Copy Markdown
Contributor Author

neutrinoceros commented Jan 23, 2024

The following line makes my patch insufficient:

astropy/astropy/utils/masked/core.py

Line 905 in 97bf1e1

if not isinstance(dispatched_result, tuple):

I'll come back to this after lunch !

@neutrinoceros neutrinoceros force-pushed the tst/numpy2/broadcast_arrays branch from 5878c6d to fef2ee8 Compare January 23, 2024 11:17
@neutrinoceros
Copy link
Copy Markdown
Contributor Author

neutrinoceros commented Jan 23, 2024

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.

@neutrinoceros neutrinoceros changed the title TST: adapt a test to a change in numpy 2.0 (np.broadcast_arrays now returns a tuple) BUG: fix compatibility with numpy 2.0 for np.broadcast_arrays (now returns a tuple) Jan 23, 2024
@neutrinoceros neutrinoceros force-pushed the tst/numpy2/broadcast_arrays branch from 6086c8e to 7245cca Compare January 23, 2024 13:51
@neutrinoceros
Copy link
Copy Markdown
Contributor Author

neutrinoceros commented Jan 23, 2024

I think I'm approaching my limit in terms of plates spinning simultaneously. I might wait until #15925 (and possibly #15929) are sorted out before I come back to this.

mhvk
mhvk reviewed Jan 23, 2024
View reviewed changes
astropy/utils/masked/core.py Outdated
return self._not_implemented_or_raise(function, types)

if not isinstance(dispatched_result, tuple):
if not isinstance(dispatched_result, FunctionHelperReturnTuple):
Copy link
Copy Markdown
Contributor

@mhvk mhvk Jan 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Copy Markdown
Contributor Author

@neutrinoceros neutrinoceros Jan 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Copy Markdown
Contributor Author

@neutrinoceros neutrinoceros Jan 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhvk is it okay if I drop the second commit for now ? I think it should make it clearer what problems still need solving.

Copy link
Copy Markdown
Contributor

@mhvk mhvk Jan 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, makes sense - let's fix the easy things while we think about the slightly harder ones!

Copy link
Copy Markdown
Contributor Author

@neutrinoceros neutrinoceros Jan 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here we go !

@pllim pllim added this to the v6.0.1 milestone Jan 23, 2024
@neutrinoceros neutrinoceros force-pushed the tst/numpy2/broadcast_arrays branch from 7245cca to 7630878 Compare January 24, 2024 06:54
@neutrinoceros neutrinoceros changed the title BUG: fix compatibility with numpy 2.0 for np.broadcast_arrays (now returns a tuple) BUG: fix compatibility with numpy 2.0 for numpy functions that now return tuples Jan 24, 2024
@neutrinoceros neutrinoceros force-pushed the tst/numpy2/broadcast_arrays branch from 7630878 to 64faaa9 Compare January 24, 2024 13:59
mhvk
mhvk approved these changes Jan 24, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, this part can go in!

@mhvk
Copy link
Copy Markdown
Contributor

mhvk commented Jan 24, 2024

@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!

@neutrinoceros neutrinoceros changed the title BUG: fix compatibility with numpy 2.0 for numpy functions that now return tuples BUG: fix compatibility with numpy 2.0 for np.broadcast_arrays (now returns a tuple) Jan 24, 2024
@neutrinoceros
Copy link
Copy Markdown
Contributor Author

neutrinoceros commented Jan 24, 2024

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.

@neutrinoceros neutrinoceros marked this pull request as ready for review January 24, 2024 14:06
@neutrinoceros
Copy link
Copy Markdown
Contributor Author

neutrinoceros commented Jan 24, 2024

As far as I can tell all new failures are caused by the harder design (#15930 (comment)).
@mhvk how should we approach this one ?

@mhvk
Copy link
Copy Markdown
Contributor

mhvk commented Jan 24, 2024

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.

@mhvk mhvk merged commit 297af9f into astropy:main Jan 24, 2024
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Jan 24, 2024
@mhvk @meeseeksmachine
Backport PR astropy#15930: BUG: fix compatibility with numpy 2.0 for …
c2e9759 
…np.broadcast_arrays (now returns a tuple)
@meeseeksmachine meeseeksmachine mentioned this pull request Jan 24, 2024
Merged
@neutrinoceros neutrinoceros deleted the tst/numpy2/broadcast_arrays branch January 24, 2024 15:13
pllim added a commit that referenced this pull request Jan 24, 2024
@pllim
Merge pull request #15939 from meeseeksmachine/auto-backport-of-pr-15…
e82cfd6 
…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))
This was referenced Jan 24, 2024
Merged
Closed
@pllim
Copy link
Copy Markdown
Member

pllim commented Jan 25, 2024

@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

@neutrinoceros
Copy link
Copy Markdown
Contributor Author

neutrinoceros commented Jan 25, 2024

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

@pllim
Copy link
Copy Markdown
Member

pllim commented Jan 25, 2024

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).

@neutrinoceros
Copy link
Copy Markdown
Contributor Author

neutrinoceros commented Jan 25, 2024

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 ! 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mhvk mhvk mhvk approved these changes

Assignees

No one assigned

Labels

convolution nddata no-changelog-entry-needed numpy-dev table units utils.masked

Projects

None yet

Milestone

v6.0.1

Development

Successfully merging this pull request may close these issues.

3 participants

@neutrinoceros @mhvk @pllim