-
Notifications
You must be signed in to change notification settings - Fork 307
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
Bump dependencies for NumPy 2 compatibility #511
Conversation
OK, I'm struggling a bit with dependencies here. If you want to keep Python >= 3.9 and at the same time also support 3.13, scipy >= 1.13.1 works only for Python <= 3.12. Strangely, uv does not use scipy 1.14.1 (there is no binary wheel for scipy 1.13.1 for Python 3.13) for Python 3.13, but insists on using 1.13.1... |
Sure, seems like the problem is that "uv run --extra dev pytest" is doing something bizarre. I don't know exactly what that command is doing or trying to do, but it seems to be trying to install the oldest versions of packages that satisfy our dependencies. That is not what we want (that's not what any normal user does) and is not likely to lead to a good solution. |
It might actually be something at the core of how uv resolves dependencies, yes. I've opened an issue here: astral-sh/uv#8492 |
Hells yeah! |
Hmmm, the Windows 3.13 job crashed, but this might be unrelated. @bemoody or @tompollard could you please restart this job (I don't have the permissions to do that myself)? |
We're hitting another issue, this time related to CPython (python/cpython#125235). We gotta wait for 3.13.1 for the fix. |
This is ready for review. Everything works with NumPy >= 2. I've removed support for Python 3.8 and added Python 3.12. Python 3.13 can be added as soon as 3.13.1 is released, but I would do that in a separate PR. It would be great if you could push out a new release to PyPI once this PR gets merged. |
Note that we could also force specific NumPy versions for each Python version as shown e.g. here (they are testing the oldest supported NumPy version for each Python release). The NumPy >= 2 requirement is currently only necessary for the test, because otherwise these would use 1.26.4. WDYT? |
I appreciate your efforts to try to get to the bottom of these issues! However, I don't think wfdb package should require numpy 2 at this stage, and I'm not inclined to change that just to appease uv. Furthermore, before removing the "numpy < 2" requirement from pyproject.toml, I would like to see that the test suite is able to run on numpy 2, with NPY_PROMOTION_STATE set to "weak_and_warn", and with no warnings generated. Currently that's not the case. That aside, we really need to have a CI workflow that tests whether the package can be installed using pip. If uv doesn't install exactly the same packages that pip would have installed, then uv is not an effective substitute. |
Of course, that's why I suggested to pin several NumPy versions (e.g., the oldest supported NumPy version per Python version).
Sure, I can do that!
Yes, we can use the |
If you are completely unhappy with uv, I can also get rid of it again and replace it with pip, just let me know. |
Thanks!
Yeah, that seems like a reasonable workaround as long as it's not too much work.
Sounds like that's the best thing for now. |
OK, I'd really appreciate if someone took a look. I've now enabled The Windows jobs all emit a warning related to tk (see here), but I think these warnings have always been there (not a result of this PR). Another question that came up: is the Debian i386 job still needed? It runs the tests with Python 3.7, which is not supported anymore. Finally, and this is a question for when all other issues have been resolved, the test matrix is rather large. Is it really necessary to test on all combinations of OSs and Python versions? |
Since uv has a new solver which now by default behaves like pip (astral-sh/uv#9868), I'll revert to using |
Alright, this seems to be working now. @bemoody @tompollard it would be great if you could take a look and merge if you're happy with the changes. A quick follow-up release with these changes would then be super useful too! 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.
Thanks @cbrnr, just added a few quick comments.
wfdb/io/convert/edf.py
Outdated
@@ -399,7 +399,7 @@ def read_edf( | |||
} | |||
|
|||
sig_data = np.empty((sig_len, n_sig)) | |||
temp_sig_data = np.fromfile(edf_file, dtype=np.int16) | |||
temp_sig_data = np.fromfile(edf_file, dtype=np.int16).astype("int64") |
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.
edf.py was updated in #527 so this change is not needed.
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.
Got it!
Looks great, thanks again for your help @cbrnr! |
This pull request adds a changelog for `v4.2.0`. The changelog is based on the following auto-generated summary of merge commits generated by GitHub: ``` ## What's Changed * bug-fix: Numpy ValueError when cheking empty list equality by @ajadczaksunriselabs in #459 * bug-fix: Pandas set indexing error by @ajadczaksunriselabs in #460 * fix for /issues/452 by @tecamenz in #465 * Use numpydoc to render documentation by @SnoopJ in #472 * build(deps): bump readthedocs-sphinx-search from 0.1.1 to 0.3.2 in /docs by @dependabot in #477 * Update style by @bemoody in #482 * Fix NaN handling in Record.adc, and other fixes by @bemoody in #481 * Set upper bound on Numpy version (numpy = ">=1.10.1,<2.0.0"). Ref #493. by @tompollard in #494 * Update actions to use actions/checkout@v3 and actions/setup-python@v4. by @tompollard in #495 * Fix: Indent code to ensure 'j' is within for-loop in GQRS algorithm by @tompollard in #499 * Add write_dir argument to csv_to_wfdb. Fixes #67. by @tompollard in #492 * Fix warnings by @cbrnr in #502 * README improvements by @bemoody in #503 * Change in type promotion. Fixes to annotation.py by @tompollard in #506 * Use uv by @cbrnr in #504 * Change in type promotion. Fixes to _signal.py by @tompollard in #507 * Test round-trip write/read of supported binary formats by @bemoody in #509 * Corrected typo and extended allowed types for MultiSegmentRecord by @agent3gatech in #514 * Allow expanded physical signal in `calc_adc_params` by @briangow in #512 * Add capability to write signal with unique `samps_per_frame` to `wfdb.io.wrsamp` by @briangow in #510 * Fix selection of channels when converting to EDF by @SamJelfs in #519 * Change in type promotion introduced in Numpy 2.0. Fixes to edf.py. by @tompollard in #527 * Bump dependencies for NumPy 2 compatibility by @cbrnr in #511 * Bump version to v4.2.0 and update notes on creating new releases by @tompollard in #497 ## New Contributors * @ajadczaksunriselabs made their first contribution in #459 * @tecamenz made their first contribution in #465 * @SnoopJ made their first contribution in #472 * @dependabot made their first contribution in #477 * @agent3gatech made their first contribution in #514 * @SamJelfs made their first contribution in #519 **Full Changelog**: v4.1.2...v4.2.0 ```
v4.2.0 is now on PyPI: https://pypi.org/project/wfdb/ |
All tests pass and
demo.ipynb
works without any errors or warnings using NumPy >= 2.