Skip to content

MNT: more compatibility with NumPy 2.0 - #15235

Merged
pllim merged 7 commits into
astropy:mainfrom
pllim:no-more-np-asfarray
Aug 29, 2023
Merged

pllim merged 7 commits into
astropy:mainfrom
pllim:no-more-np-asfarray

Conversation

@pllim

@pllim pllim commented Aug 27, 2023 •

Copy link
Copy Markdown
Member

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
Copy Markdown
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?
  • 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

👋 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

pllim commented Aug 27, 2023

Copy link
Copy Markdown
Member Author

pre-commit.ci autofix

@pllim

pllim commented Aug 27, 2023 •

Copy link
Copy Markdown
Member Author

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
pllim force-pushed the no-more-np-asfarray branch from 1a3be18 to 029a509 Compare August 27, 2023 17:12
@pllim

pllim commented Aug 28, 2023 •

Copy link
Copy Markdown
Member Author

And then

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

😩

@pllim

pllim commented Aug 28, 2023

Copy link
Copy Markdown
Member Author

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

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread astropy/io/votable/converters.py Resolved
Comment thread astropy/io/votable/converters.py Resolved
Comment thread astropy/io/votable/tests/test_vo.py Resolved
Comment thread astropy/table/meta.py Resolved
@mhvk
mhvk force-pushed the no-more-np-asfarray branch from b06ebd4 to 236d90d Compare August 28, 2023 07:55
@mhvk mhvk changed the title MNT: np.asfarray compat for NumPy 2.0 MNT: more compatibility with NumPy 2.0 Aug 28, 2023
@mhvk

mhvk commented Aug 28, 2023

Copy link
Copy Markdown
Contributor

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

@mhvk

mhvk commented Aug 28, 2023

Copy link
Copy Markdown
Contributor

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

@mhvk
mhvk force-pushed the no-more-np-asfarray branch from 4000c94 to 61b6d15 Compare August 28, 2023 08:47
@mhvk
mhvk marked this pull request as ready for review August 28, 2023 08:47
@pllim

pllim commented Aug 28, 2023

Copy link
Copy Markdown
Member Author

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

@pllim

pllim commented Aug 28, 2023

Copy link
Copy Markdown
Member Author

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

pllim commented Aug 28, 2023

Copy link
Copy Markdown
Member Author

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

@pllim

pllim commented Aug 28, 2023

Copy link
Copy Markdown
Member Author

Oh, no. A new one?

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

@bsipocz

bsipocz commented Aug 28, 2023

Copy link
Copy Markdown
Member

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

pllim commented Aug 28, 2023

Copy link
Copy Markdown
Member Author

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.
@pllim
pllim force-pushed the no-more-np-asfarray branch from a07518a to b6deb07 Compare August 29, 2023 02:15
@mhvk

mhvk commented Aug 29, 2023

Copy link
Copy Markdown
Contributor

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

pllim commented Aug 29, 2023

Copy link
Copy Markdown
Member Author

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

@pllim

pllim commented Aug 29, 2023 •

Copy link
Copy Markdown
Member Author

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

pllim commented Aug 29, 2023

Copy link
Copy Markdown
Member Author

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

@dhomeier

Copy link
Copy Markdown
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

pllim commented Aug 29, 2023

Copy link
Copy Markdown
Member Author

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

dhomeier commented Aug 29, 2023 •

Copy link
Copy Markdown
Contributor

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

pllim commented Aug 29, 2023

Copy link
Copy Markdown
Member Author

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

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

pllim commented Aug 29, 2023

Copy link
Copy Markdown
Member Author

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

@bsipocz

bsipocz commented Aug 29, 2023 •

Copy link
Copy Markdown
Member

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