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

numpy 2.0 compatibility #3461

Merged
merged 21 commits into from
Jun 6, 2024
Merged

numpy 2.0 compatibility #3461

merged 21 commits into from
Jun 6, 2024

Conversation

megies
Copy link
Member

@megies megies commented May 24, 2024

What does this PR do?

Tries to get ourselves ready for upcoming numpy 2.0 in June 2024

Why was it initiated? Any relevant Issues?

fixes #3460

PR Checklist

  • Correct base branch selected? master for new features, maintenance_... for bug fixes
  • This PR is not directly related to an existing issue (which has no PR yet).
  • While the PR is still work-in-progress, the no_ci label can be added to skip CI builds
  • If the PR is making changes to documentation, docs pages can be built automatically.
    Just add the build_docs tag to this PR.
    Docs will be served at docs.obspy.org/pr/{branch_name} (do not use master branch).
    Please post a link to the relevant piece of documentation.
  • If all tests including network modules (e.g. clients.fdsn) should be tested for the PR,
    just add the test_network tag to this PR.
  • All tests still pass.
  • Any new features or fixed regressions are covered via new tests.
  • Any new or changed features are fully documented.
  • Significant changes have been added to CHANGELOG.txt .
  • First time contributors have added your name to CONTRIBUTORS.txt .
  • If the changes affect any plotting functions you have checked that the plots
    from all the CI builds look correct. Add the "upload_plots" tag so that plotting
    outputs are attached as artifacts.
  • New modules, add the module to CODEOWNERS with your github handle
  • Add the yellow ready for review label when you are ready for the PR to be reviewed.

@megies megies added installation issues with installing obspy CI issue generally related to continuous integration packaging issue related to our packaging efforts (e.g. conda, Debian, ...) numpy test_network tell github actions to also run network tests for this PR test_wheels labels May 24, 2024
@megies megies added this to the 1.5.0 milestone May 24, 2024
@megies megies requested a review from trichter May 24, 2024 07:41
@megies megies changed the title CI: try to add a np 2.0.0rc2 matrix node numpy 2.0 compatibility May 24, 2024
@trichter trichter mentioned this pull request May 24, 2024
@trichter
Copy link
Member

Looks good, maybe also add test cases without turning warnings into errors and network test cases?

@megies
Copy link
Member Author

megies commented May 24, 2024

Looks good, maybe also add test cases without turning warnings into errors and network test cases?

Yea, maybe change it later.. only concern is how that would blow up the size of our test matrix. Working on the numpy stuff rn

@megies
Copy link
Member Author

megies commented May 24, 2024

It's looking quite good. Not a lot more to figure out, I think, based on local tests. Let's see CI. Need to fix some bitshift thing in mseed and some precision loss issue in realtime that needs some upcasting in places probably.

Most dangerous stuff I've seen is when things like np.uint8 or np.float32 go into calculations without upcasting which runs into precision loss sometimes. And some weird stuff going on with bitshifts that used to upcast in weird fashion, feels like that was abusing bitshifts, not sure though, no bitshift magician, really.

@megies
Copy link
Member Author

megies commented May 25, 2024

The test fail in trace preview creation seems to come from a numpy bug (or at least changed behavior, kind of) in the ptp() method-now-function, see numpy/numpy#26530

If numpy doesn't change this we likely want to add something like this replacing all ptp usage across our code:

def ptp(x, *args, **kwargs):
    if np.ma.is_masked(x):
        return np.ma.ptp(x, *args, **kwargs)
    return np.ptp(x, *args, **kwargs)

EDIT: Problem kind of is that now there's two functions np.ptp() and np.ma.ptp() when before we could just call the method on the array and it would do "the right thing" depending on whether it's a regular or masked array.

@megies
Copy link
Member Author

megies commented Jun 5, 2024

see numpy/numpy#13718 for a deprecation warning shown in io.nordic

@megies megies requested a review from flixha as a code owner June 5, 2024 11:05
since we're using a non-interactive backend during tests, plt.show()
cant open a window
when running tests, obspy gets imported before warning filter rules
specified in pytest.ini take effect, since conftest.py gets loaded,
which sits in the root obspy source code directory and then the
__init__.py gets loaded which imports the main core classes and
functions into our main namespace.
so the only way to make "pytest -W error" pass without error is to
ignore that warning directly in our obspy root __init__.py
@megies megies added the ready for review PRs that are ready to be reviewed to get marked ready to merge label Jun 5, 2024
Copy link
Contributor

@ThomasLecocq ThomasLecocq left a comment

Choose a reason for hiding this comment

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

looks all good to me! Amazing job here!

.github/workflows/tests.yml Show resolved Hide resolved
@megies
Copy link
Member Author

megies commented Jun 5, 2024

I think this is ready. Most changes for numpy v2.0 are quite trivial, mostly replacing some removed function with its drop in replacement.

The most concern is numpy from v2 not automatically upcasting to higher datatypes wherever needed, which can lead to e.g. np.int8 and similar overflowing if not manually upcast or precision loss in single precision floats that used to automatically get upcast to double precision whenever needed.
I'm fairly sure, that our tests might not catch all cases where this potentially happens, but there isn't really much we can do about it now. I added manual upcasts wherever our tests started failing (usually just at the x-th digit behind the comma..).

This also happens in some bitwise operations. I'm not really used to using bitwise operations a lot, so if somebody could double check these, would be great. In any case tests still pass..

Network test fails are unrelated, something broke with EarthScope FDSN WS

That one failing test on windows is unrelated too, looks like an issue with removing read permission from a test file (so we can check it is skipped by our Scanner utility)

@megies megies merged commit 236b507 into master Jun 6, 2024
47 of 54 checks passed
@megies megies deleted the np2 branch June 6, 2024 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI issue generally related to continuous integration installation issues with installing obspy numpy packaging issue related to our packaging efforts (e.g. conda, Debian, ...) ready for review PRs that are ready to be reviewed to get marked ready to merge test_network tell github actions to also run network tests for this PR test_wheels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Numpy 2.0(.0rc2)
4 participants