-
Notifications
You must be signed in to change notification settings - Fork 16
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 #388
numpy 2.0 #388
Conversation
@DilyOng will likely be interested in @AdamOrmondroyd's observation PyTables/PyTables#1172, as she's been trying to use hdf5 recently without success. |
Provided numpy<2.0.0, hdf is working fine on my mac. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #388 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 36 36
Lines 3069 3076 +7
=========================================
+ Hits 3069 3076 +7 ☔ View full report in Codecov by Sentry. |
Thanks for this @AdamOrmondroyd! |
I think the test suite may have misled you. All the pytests involving fastkde skip rather than fail due to the failed import, which is a flaw in how our optional dependency tests work. Really, the "true" tests should fail.
I was waiting for fastkde to release a pypi update.
This should be covered by the minimum dependencies test, if it isn't that's a bug. |
I agree that we should support numpy 1 -- given that it was non-zero work for @AdamOrmondroyd to upgrade our code, we shouldn't expect others to. Our test suite should check both cases, but we'll only need one numpy v1 case rather than the full grid |
Ah I see the problem - min_requirements doesn't do anything with <= requirements, should we define a minimum numpy and scipy? |
…ll earlier versions did not want to install locally
oh yeah, that is annoying... :/ |
Do you have a favourite minimum scipy? |
Does some dark codecov magic need to happen? |
Nope... Leave it for now? Or go with the getdist requirement of
Ah cr**, yeah, and I always forget what the right order of things to do is... |
…ther a python package is installed, appears more robust
reason = "requires fastkde package" | ||
condition = 'fastkde' not in sys.modules | ||
condition = find_spec('fastkde') is None | ||
raises = ImportError | ||
fastkde_mark_skip = pytest.mark.skipif(condition, reason=reason) | ||
fastkde_mark_xfail = pytest.mark.xfail(condition, raises=raises, reason=reason) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change addresses the problem that @AdamOrmondroyd raised in a comment:
All the pytests involving fastkde skip rather than fail due to the failed import, which is a flaw in how our optional dependency tests work. Really, the "true" tests should fail.
With these changes the extras=true
tests now fail, and the extras=false
tests pass. Hopefully all tests pass once fastkde functions properly with numpy 2.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just what we need, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works locally with the current main branch of fastkde, just need them to update it on pypi when they've fixed their ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic, many thanks @AdamOrmondroyd!
Description
Changes for compatibility with numpy 2.0
np.trapz
has been deprecated in favour ofnp.trapezoid
fixes #389
*TODO: change numpy restriction once #390 is merged and pytables is updated
Checklist:
flake8 anesthetic tests
)pydocstyle --convention=numpy anesthetic
)python -m pytest
)