-
Notifications
You must be signed in to change notification settings - Fork 31
Add tests for collections and tools; improve numpy2 support #139
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
Conversation
# Conflicts: # src/instamatic/_collections.py # src/instamatic/experiments/fast_adt/experiment.py
@stefsmeets I guess these were said "non-trivial issues with data type conversions", in which case this fixes #130. Tests work with numpy 2.3.2 for me, I will temporarily force numpy version above 2 for testing on github. In case it passes, I will revert the lower limit to |
Successfully tested using
The only warning comes from " Reverting the previous commit to allow python 1.17+ builds, though in the long term I would prefer to switch to numpy > 2 seeing how ubiquitously successful and available it is. Merging tomorrow unless anyone objects. |
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.
Looks good to me.
https://docs.astral.sh/ruff/rules/numpy2-deprecation/ might also catch something not caught by the tests.
Numpy 2.0 does not drop any Python versions, 3.9 being the lowest supporded version which matches Instamatic. I think we can safely use numpy>=2. |
@stefsmeets In that case I would absolutely prefer to rip the band-aid. Newer numpy is more consistent and adds cool typing features. Also I have some cleaning to do: $ python -m ruff check ./src ./tests --select NPY201
warning: Invalid `# noqa` directive on src\instamatic\gui\fast_adt_frame.py:73: expected a comma-separated list of codes (e.g., `# noqa: F401, F841`).
warning: Invalid `# noqa` directive on src\instamatic\calibrate\calibrate_stage_rotation.py:166: expected a comma-separated list of codes (e.g., `# noqa: F401, F841`).
All checks passed! |
I'd maybe suggest leaving switch to numpy 2 for a few more months. In case someone must use Python 3.9, as it is still supported for a month or so, the only minor numpy 2 version available is 2.0. Planning a switch for 2026 though. |
A short PR, to be merged after #137. I had some time today so I added tests for all the simple
instamatic._collections
andinstamatic.tools
. These are mostly straightforward but it's a good practice to have them all tested. I removed one duplicate and some unused imports while I was at it. Apparently this pushes instamatic line coverage from 48% to 49%.When testing with numpy 2.3.2 I have also found and fixed faulty use of
np.frombuffer
ininstamatic/camera/gatansocket3.py
that caused numpy 2 to fail tests that numpy 1 passed. In principle,gatansocket3.py
implementation correctly tried to buff its messages to a multiple ofnp.int_
byte size (asnp.frombuffer
requires) but on modern 64-bit systems the size ofnp.int_
is 8, not 4, as it was hard-coded before. Numpy 1 buffed the incorrectly-sized bytes as needed, but numpy 2 does not.This PR does not introduce any new features. It may fixes some of the issues mentioned by @stefsmeets in #130.