8000 Remove calls to deprecated `np.round_` alias for Numpy 2.0 by dhomeier · Pull Request #15266 · astropy/astropy · GitHub
[go: up one dir, main page]
Skip to content

Remove calls to deprecated np.round_ alias for Numpy 2.0 #15266

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
Sep 4, 2023
Merged

Remove calls to deprecated np.round_ alias for Numpy 2.0 #15266

merged 1 commit into from
Sep 4, 2023

Conversation

dhomeier
Copy link
Contributor
@dhomeier dhomeier commented Sep 1, 2023 •
edited
Loading

Description

This pull request is to address a new one from numpy/numpy#24477 – apparently 'numpy==2.0.0.dev0' is only updated every couple of days.
To reproduce issue, run e.g.

from astropy.units import Quantity

with current numpy main installed.

  • 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
@dhomeier dhomeier added this to the v6.0 milestone Sep 1, 2023
@github-actions
Copy link
Contributor
github-actions bot commented Sep 1, 2023

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.

@dhomeier dhomeier marked this pull request as draft September 1, 2023 18:16
@dhomeier
Copy link
Contributor Author
dhomeier commented Sep 1, 2023

Seems this has to wait until the pip numpy-dev is actually updated.

@pllim
Copy link
Member
pllim commented Sep 1, 2023 •
edited
Loading

Thanks for being proactive on this!

mhvk
mhvk reviewed Sep 2, 2023
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.

Thanks, indeed, for being proactive. I fear we're not yet done but we'll get there...

@dhomeier dhomeier marked this pull request as ready for review September 4, 2023 09:58
@dhomeier dhomeier requested review from mhvk and pllim September 4, 2023 09:59
@dhomeier
Copy link
Contributor Author
dhomeier commented Sep 4, 2023 •
edited
Loading

My pip somehow is unable to install the "2.0.0dev", so I had to install directly from GitHub ;-)
Yes, we've been discussing in the DevCon on Friday if we would manage to catch up with everything Numpy 2.0 and Python 3.12 for 6.0... But I hope it is a continuing, but decreasing rate of changes approaching zero by then!

The change seems to be in now, so this should be ready once the extra CI is through.

@dhomeier dhomeier mentioned this pull request Sep 4, 2023
Closed
1 task
mhvk
mhvk reviewed Sep 4, 2023
View reviewed changes
astropy/units/tests/test_quantity_non_ufuncs.py
@@ -692,6 +692,7 @@ def test_round(self):
self.check(np.round)

# NUMPY_LT_1_25
@pytest.mark.skipif(not NUMPY_LT_2_0, reason="np.round_ is removed in NumPy 2.0")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think what is needed here is to only define the function on numpy < 2, i.e.,

if NUMPY_LT_2_0:
    @pytest.mark.filterwarnings(...)
    def test_round_(...)

The way the test for completeness is done is that defining the test causes the name to be added to a tested list.

Copy link
Contributor Author
@dhomeier dhomeier Sep 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, I did not see that fail in https://github.com/astropy/astropy/actions/runs/6052493225/job/16468784807#step:7:1349 or the tests for #15274 or my local tests so far – I'd expect pytest to skip already the function definition here, could this be different behaviour over different pytest versions? But the reference to np.round_ would only be made once self.check(...) is executed, so I really see no possible difference to test_function_helpers here. In what setup did you see the failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way the test for completeness is done is that defining the test causes the name to be added to a tested list.

Or what did you mean by "test for completeness" – something codecov-related?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm very confused. When I looked at the tests before, the -dev test did have a failure, but now it has all passed. I agree that logically pytest.skipif should work, so all is good now! Let's get it in!!

@mhvk
Copy link
Contributor
mhvk commented Sep 4, 2023

Sadly, there still is a failure np.round_ is still included somewhere; astropy.units.tests.test_quantity_non_ufuncs.TestFunctionHelpersCompleteness fails. See in-line comment for what should fix it.

@dhomeier
Copy link
Contributor Author
dhomeier commented Sep 4, 2023

Well, hypothesis 6.84.0 is out, solving #15274 (comment), so I've re-run all tests again and still see no failures remaining.

mhvk
mhvk approved these changes Sep 4, 2023
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.

Happy to approve now tests pass - the changes looked good before and I am not sure why there was a failure in the earlier -dev run.

@mhvk
Copy link
Contributor
mhvk commented Sep 4, 2023

removed/restored label to try to get towncrier to pass.

@mhvk mhvk enabled auto-merge September 4, 2023 15:12
@mhvk
Copy link
Contributor
mhvk commented Sep 4, 2023

p.s. Sadly, that triggered a whole slew more. Anyway, enabled auto-merge. Thanks, @dhomeier!

@mhvk mhvk merged commit 3ccb94f into astropy:main Sep 4, 2023
@dhomeier
Copy link
Contributor Author
dhomeier commented Sep 4, 2023

Thanks for reviewing! The very first test failed because the pip dev version had not updated yet, leaving np.round_ uncovered in function_helpers, but I was struggling as well to locate the first re-run today, so not sure what might have shown up in the meantime. I did not re-run the cron jobs, as they are only testing numpy 1.26 beta.
No idea what towncrier is still waiting for :(

@mhvk
Copy link
Contributor
mhvk commented Sep 4, 2023

OK, I must have seen something old somehow. Anyway, thanks for the fix!!

@pllim
Copy link
Member
pllim commented Sep 5, 2023

Thank you. It feels good to see green CI after USA Labor Day. 😉 🙇‍♀️

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

@neutrinoceros neutrinoceros neutrinoceros approved these changes

@pllim pllim Awaiting requested review from pllim

Assignees
No one assigned
Labels
Bug Extra CI Run cron CI as part of PR no-changelog-entry-needed numpy-dev units utils.masked
Projects
None yet
Milestone
v6.0.0
Development

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