Skip to content
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.

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

MNT: more compatibility with NumPy 2.0 #15235

Merged
merged 7 commits into from Aug 29, 2023
Merged

Conversation

pllim
Copy link
Member

@pllim pllim commented Aug 27, 2023

Description

This pull request is to play catch up with ever changing numpy-dev.

p.s. Maybe the code can be cleaner but could use some advice from subpackage maintainers.

If we decide not to go ahead with #15234 , then we have to backport this but for now, I set the PR to not be backported.

Fixes #15233

  • 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

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

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

@pllim
Copy link
Member Author

pllim commented Aug 27, 2023

pre-commit.ci autofix

@pllim
Copy link
Member Author

pllim commented Aug 27, 2023

Now we're hitting this but I am not sure how to respond to it:

Do we simply update the imports or we are not supposed to touch this now?

astropy/utils/masked/__init__.py:10: in <module>
    from .core import *
astropy/utils/masked/core.py:25: in <module>
    from .function_helpers import (
astropy/utils/masked/function_helpers.py:1077: in <module>
    for nanfuncname in np.lib.nanfunctions.__all__:
numpy/lib/__init__.py:87: in __getattr__
    raise AttributeError("module {!r} has no attribute "
E   AttributeError: module 'numpy.lib' has no attribute 'nanfunctions'

def masked_nanfunc(nanfuncname):

for nanfuncname in np.lib.nanfunctions.__all__:
globals()[nanfuncname] = dispatched_function(
masked_nanfunc(nanfuncname), helps=getattr(np, nanfuncname)
)

@pllim
Copy link
Member Author

pllim commented Aug 28, 2023

And then

AttributeError: `np.unicode_` was removed in the NumPy 2.0 release. Use `np.str_` instead.

😩

@pllim
Copy link
Member Author

pllim commented Aug 28, 2023

Oh dear, np.float_ too? Just how many more in one weekend?

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! Quite a few small comments, all easy now that you found what was wrong. Indeed, easy enough I might as well do them and force-push...

astropy/io/votable/converters.py Show resolved Hide resolved
astropy/io/votable/converters.py Outdated Show resolved Hide resolved
astropy/io/votable/tests/test_vo.py Show resolved Hide resolved
astropy/table/meta.py Show resolved Hide resolved
astropy/utils/masked/function_helpers.py Outdated Show resolved Hide resolved
astropy/utils/masked/function_helpers.py Outdated Show resolved Hide resolved
astropy/utils/masked/function_helpers.py Outdated Show resolved Hide resolved
@mhvk mhvk changed the title MNT: np.asfarray compat for NumPy 2.0 MNT: more compatibility with NumPy 2.0 Aug 28, 2023
@mhvk
Copy link
Contributor

mhvk commented Aug 28, 2023

p.s. Also added a commit that removes the use of np.float_ as an alias of np.float64.

@mhvk
Copy link
Contributor

mhvk commented Aug 28, 2023

Arrgg, I should have guessed given np.float_ being removed - of course the other aliases are removed too. I'll push a further fix...

@pllim
Copy link
Member Author

pllim commented Aug 28, 2023

That is the plan but looks like skyfield now gives trouble.

@pllim
Copy link
Member Author

pllim commented Aug 28, 2023

Was np.float_ never deprecated? I feel like if it was, we would have caught it and handled it before it breaks everything like this.

because not all upstream dependencies are so cutting edge.
@pllim
Copy link
Member Author

pllim commented Aug 28, 2023

Gonna take a few hours for the exotic archs to finish running. I'll check back later.

@pllim
Copy link
Member Author

pllim commented Aug 28, 2023

Oh, no. A new one?

AttributeError: module 'numpy.lib' has no attribute 'function_base'

@bsipocz
Copy link
Member

bsipocz commented Aug 28, 2023

Oh, no. A new one?

Maybe it's somewhat new, but not necessarily, the latest wheel is 12hr old, so should have been used in CI here for a while now), but it might have been hidden by the other errors.

@pllim
Copy link
Member Author

pllim commented Aug 28, 2023

Alas, I ran out of time today, will have to revisit tomorrow unless @mhvk get to it (again) first.

BTW, thanks for the clean up this morning, @mhvk !

@pllim pllim requested a review from a team as a code owner August 29, 2023 01:39
Handle warning in test_bounds_gauss2d_lsq
that is flaky and we do not really care for, so just ignore it.
@mhvk
Copy link
Contributor

mhvk commented Aug 29, 2023

Am travelling today, so maybe you'll get to it first... And, yes, lots of renaming happening, to indicate things are private... But I think it is a one-time only reorganization...

@pllim
Copy link
Member Author

pllim commented Aug 29, 2023

Everything is green now except RTD that somehow killed itself. I restarted it. 🤞

@pllim
Copy link
Member Author

pllim commented Aug 29, 2023

I am not sure about changing devdeps to pull only "tests" instead of "tests_all" but nothing we can do about that right now if we want this in, so I'll open follow-up issue for that one (#15242).

@pllim
Copy link
Member Author

pllim commented Aug 29, 2023

codecov complain is unavoidable because we don't check coverage for the numpy>=2 routes.

@dhomeier
Copy link
Contributor

@pllim I've opened a PR addressing the Windows issues in pllim#6, but it is not running the CI there, and I have no access to either a Windows or 32 bit system to make any tests myself.

@pllim
Copy link
Member Author

pllim commented Aug 29, 2023

Thanks, @dhomeier . I want to kick off the dev wheel for astropy, so I am going to merge this. Can you please turn your branch into a PR here (instead of to my fork) after that? I think we need to think about why win32 emit different warnings now. Is there some real bug we have to fix?

@pllim pllim merged commit 2dd07e3 into astropy:main Aug 29, 2023
55 of 56 checks passed
@dhomeier
Copy link
Contributor

dhomeier commented Aug 29, 2023

Thanks; don't know it it's a bug or what the Windows version is actually reading from that.
For investigating on a Windows system outside of pytest in principle it should suffice to run

import warnings
from astropy.io import ascii
from io import BytesIO
StringIO = lambda x: BytesIO(x.encode("ascii"))

warnings.resetwarnings()
x = ascii.read(StringIO("-1799E+305 0.2e-323 5200e-327"), format='no_header', fast_reader={"use_fast_converter": False})
warnings.resetwarnings()
y = ascii.read(StringIO("0." + 307 * "0" + "1"), format='no_header', fast_reader={"use_fast_converter": False})

and then check the number of warnings and the content of tables x and y.

@pllim pllim deleted the no-more-np-asfarray branch August 29, 2023 14:04
@pllim
Copy link
Member Author

pllim commented Aug 29, 2023

Alas, pytest is very broken on my Windows env and I don't have time to deal with it. I don't see this problem on WSL2. 😿

@dhomeier
Copy link
Contributor

Alas, pytest is very broken on my Windows env and I don't have time to deal with it. I don't see this problem on WSL2. 😿

Yes, someone with access to a Windows machine would have to run that just manually, but if they all behave different, that might not give us much info anyway...

@@ -184,6 +183,7 @@ def test_bounds_slsqp(self):
assert intercept + 10**-5 >= bounds["intercept"][0]
assert intercept - 10**-5 <= bounds["intercept"][1]

@pytest.mark.filterwarnings("ignore:The fit may be unsuccessful")
Copy link
Member Author

Choose a reason for hiding this comment

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

@WilliamJamieson , if you are not okay with this, please open follow-up PR. Thanks!

in_sig, out_sig = np.lib.function_base._parse_gufunc_signature(
ufunc.signature.replace(" ", "")
)
if NUMPY_LT_2_0:
Copy link
Member Author

Choose a reason for hiding this comment

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

@mhvk , this might not be as clean as you like based on your previous changes. If this bothers you, please open follow-up PR. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries, that looks fine! Thanks!

@pllim
Copy link
Member Author

pllim commented Aug 29, 2023

@bsipocz , new dev wheels should be uploaded already. Please let me know if you cannot access them. Thanks!

@bsipocz
Copy link
Member

bsipocz commented Aug 29, 2023

Nope, they work, I restarted everything and CI is passing now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build all wheels Run all the wheel builds rather than just a selection Extra CI Run cron CI as part of PR no-changelog-entry-needed numpy-dev units utils.masked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle "np.asfarray was removed in the NumPy 2.0 release"
5 participants