8000 BUG: fix an incoming incompatibility with numpy 2.1 in `utils.masked` by neutrinoceros · Pull Request #16755 · astropy/astropy · GitHub
[go: up one dir, main page]
Skip to content

BUG: fix an incoming incompatibility with numpy 2.1 in utils.masked #16755

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jul 24, 2024
Merged

BUG: fix an incoming incompatibility with numpy 2.1 in utils.masked #16755

merged 1 commit into from
Jul 24, 2024

Conversation

neutrinoceros
Copy link
Contributor

Description

xref: numpy/numpy#26846

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

8000
@github-actions GitHub Actions
Copy link
Contributor

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.

@github-actions GitHub Actions
Copy link
Contributor

👋 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?

@neutrinoceros neutrinoceros changed the title BUG: fix an incoming incompatibility with numpy 2.1 in utils.masked BUG: fix an incoming incompatibility with numpy 2.1 in utils.masked Jul 23, 2024
@neutrinoceros
Copy link
Contributor Author

Ah, it's failing the devdeps jobs because numpy nightlies have not yet caught up with the change this is addressing. For reference, I discovered and fixed it locally by building numpy's main branch

@neutrinoceros neutrinoceros force-pushed the utils.masked/compat/numpy2.1_printoptions branch 3 times, most recently from a796bac to 433bbf9 Compare July 23, 2024 08:07
@neutrinoceros
Copy link
Contributor Author

Technically ready for review so I’ll undraft now, but maybe others would rather wait for numpy nightlies to catch up before this goes in.

@neutrinoceros neutrinoceros marked this pull request as ready for review July 23, 2024 09:24
mhvk
mhvk approved these changes Jul 23, 2024
View reviewed changes
Copy link
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.

Yes, when I reviewed the numpy code changes, I feared we had to make changes too... But this all looks good. But let's wait with merging until the nightlies have updated. Thanks!

@mhvk mhvk added this to the v6.1.2 milestone Jul 23, 2024
@neutrinoceros
Copy link
Contributor Author
neutrinoceros commented Jul 23, 2024 •
edited
Loading

@mhvk actually, why don't we stick to using public API (namely, https://numpy.org/doc/stable/reference/generated/numpy.get_printoptions.html) instead ? Maybe performance reasons ? If so, maybe we should wrap these into try blocks and fallback to public API for future-proofing ?

@mhvk
Copy link
Contributor
mhvk commented Jul 23, 2024

The code was basically a copy from what was in arrayprint.py, with just a comment stating what changed. I think it still makes sense to do that, but possibly that code has in the meantime changed too... In any case, my sense would be to stick with what you have, but I'm also happy with moving to getprintoptions...

@neutrinoceros
Copy link
Contributor Author

I gave it a try and found something interesting:

here are the options we get with the current implementation without altering defaults:

               {'edgeitems': 3,
                'floatmode': 'maxprec',
                'formatter': None,
                'infstr': 'inf',
                'legacy': 9223372036854775807,
                'linewidth': 75,
                'nanstr': 'nan',
                'override_repr': None,
                'precision': 8,
                'sign': '-',
                'suppress': False,
                'threshold': 1000}

and here's what we get instead from np.get_printoptions

             {'edgeitems': 3,
              'floatmode': 'maxprec',
              'formatter': None,
              'infstr': 'inf',
              'legacy': False,
              'linewidth': 75,
              'nanstr': 'nan',
              'override_repr': None,
              'precision': 8,
              'sign': '-',
              'suppress': False,
              'threshold': 1000}

notice the value of legacy changed (and unfortunately, this does have an impact on some of our tests). From looking at np.get_printoptions's implementation, this is very clearly an intentional behavior there, so I think it may be considered a (minor) bug on our side that we apparently rely on implementation details from numpy for some tests, but I think fixing this can wait for astropy 7.0, so I'll open another PR to propose this change.

@neutrinoceros neutrinoceros mentioned this pull request Jul 23, 2024
Closed
1 task
@neutrinoceros
Copy link
Contributor Author

done in #16759

@pllim pllim requested a review from astrofrog July 23, 2024 20:48
@pllim
Copy link
Member
pllim commented Jul 23, 2024

tl;dr -- We need another commit here to fix this failure in devdeps?

ModuleNotFoundError: No module named 'numpy._core.printoptions'

@pllim
Copy link
Member
pllim commented Jul 23, 2024

OK nvm I just saw #16755 (comment)

I will come back to this when the "nightly" updates, I guess. Thanks!

@neutrinoceros
Copy link
Contributor Author

If I'm reading the cron right, the next batch of nightlies is expected in about 6 hours, so I'll rebase this tomorrow morning and hopefully CI should go full green here.

@neutrinoceros neutrinoceros force-pushed the utils.masked/compat/numpy2.1_printoptions branch from 433bbf9 to 6d5cf78 Compare July 24, 2024 06:47
@neutrinoceros
Copy link
Contributor Author

(semi-accidental haiku)

@neutrinoceros
Copy link
Contributor Author

Incidentally, if you want to see tests failing without this patch, see example logs

@pllim pllim merged commit d8e98c2 into astropy:main Jul 24, 2024
25 of 27 checks passed
@pllim
Copy link
Member
pllim commented Jul 24, 2024

Thanks! Windows failure is indeed unrelated.

meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Jul 24, 2024
@pllim @meeseeksmachine
Backport PR astropy#16755: BUG: fix an incoming incompatibility with …
e26c58a 
…numpy 2.1 in `utils.masked`
@meeseeksmachine meeseeksmachine mentioned this pull request Jul 24, 2024
Merged
@neutrinoceros neutrinoceros deleted the utils.masked/compat/numpy2.1_printoptions branch July 24, 2024 13:39
pllim added a commit that referenced this pull request Jul 25, 2024
@pllim
Merge pull request #16767 from meeseeksmachine/auto-backport-of-pr-16…
991aead 
…755-on-v6.1.x

Backport PR #16755 on branch v6.1.x (BUG: fix an incoming incompatibility with numpy 2.1 in `utils.masked`)
@neutrinoceros neutrinoceros mentioned this pull request Aug 19, 2024
8000
Closed
@pllim pllim modified the milestones: v6.1.2, v6.1.3 Aug 19, 2024
@pllim
Copy link
Member
pllim commented Aug 19, 2024

Re: #16851 (comment)

Fixed the milestone.

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

@astrofrog astrofrog Awaiting requested review from astrofrog

Assignees
No one assigned
Labels
no-changelog-entry-needed numpy-dev utils.masked
Projects
None yet
Milestone
v6.1.3
Development

Successfully merging this pull request may close these issues.
3 participants
@neutrinoceros @mhvk @pllim
0