Fix Numpy 2.4 compatibility in PDB/vectors.py#5138
Fix Numpy 2.4 compatibility in PDB/vectors.py#5138Augustin-Zidek wants to merge 8 commits intobiopython:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5138 +/- ##
==========================================
+ Coverage 85.52% 86.41% +0.89%
==========================================
Files 282 282
Lines 59497 59499 +2
==========================================
+ Hits 50882 51418 +536
+ Misses 8615 8081 -534 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@mdehoon you're probably more familar with NumPy than me, does this look good to merge? Thanks! |
|
Is this one line change not enough to fix it? |
|
Yeah, that looks reasonable as well and more minimal -- I like it. I will test that on Monday. |
|
I tested this and changing that single line is not enough, all of the changes are needed. However, instead of set_homog_trans_mtx(a1[0].item(), a1[1].item(), a1[2].item(), tm)one could do: set_homog_trans_mtx(a1[0, 0], a1[1, 0], a1[2, 0], tm)But the changes to |
|
Can you combine this then please? The |
|
All done + I also changed it to use math for trigonometric functions instead of Numpy where you suggested. This uncovered a few more issues with passing non-scalar values which I fixed. See the latest commit. |
| sinang = math.sin(angle_rads) | ||
|
|
||
| mtx[0][0] = mtx[1][1] = cosang | ||
| mtx[0][0] = cosang |
There was a problem hiding this comment.
These lines feel odd, like it was written before double indexing was possible. Does mtx[0, 0] et work here?
| set_Z_homog_rot_mtx(-sc[1], mrz) | ||
| # rotate translated a2 -polar_angle about Y | ||
| set_Y_homog_rot_mtx(-sc[2], mry) | ||
| set_Y_homog_rot_mtx(-sc[2][0], mry) |
There was a problem hiding this comment.
Looks link -sc[2, 0] should work here as well or better than -cs[2][0] does.
There was a problem hiding this comment.
Hmm, the linter is unhappy:
Bio/PDB/vectors.py: note: In function "coord_space":
Bio/PDB/vectors.py:576:26: error: Value of type "float" is not indexable
[index]
set_Y_homog_rot_mtx(-sc[2][0], mry)
^~~~~~~~
Bio/PDB/vectors.py:623:25: error: Value of type "float" is not indexable
[index]
set_Y_homog_rot_mtx(sc[2][0], mry)
^~~~~~~~
Found 2 errors in 1 file (checked 298 source files)
|
These changes look good to me, thanks for the updates and thanks Peter for the reviews 🙏 |
|
This was failing, and I just merged #5165 which caused the conflicts. |
|
Linting failure from last month still there: |
|
Huh, a new failure: @Augustin-Zidek I may have broken this with the merge resolution? -- I am increasingly of the view that it is the data structure in the test is odd, and only worked due to a historical oddity in numpy and need not be supported - i.e. let's just merge #5161 to change the test? |
|
Hey, sorry for the delay. I added one more commit that should fix things. |
|
I believe the change is now smaller (just 3 changed lines) and the tests are green on my side. |
| return (0, 0, 0) | ||
| azimuth = _get_azimuth(xyz[0], xyz[1]) | ||
| polar_angle: float = np.arccos(xyz[2] / r) | ||
| polar_angle: float = np.arccos(xyz[2] / r).item() |
There was a problem hiding this comment.
I think this could just be math.acos to ensure a float.
Are there any other uses? I didn't notice this when working on #5165.
|
Did you try the full test suite? CI is reporting: This is part of why I think the test is wrong, and that while the old array input used to work it was not deliberate. |
Yes, I ran the full test suite, but not at head, but rather at an older commit that we have mirrored. But I also suspect something being wrong with the test setup, as the shape of the |
|
Ah, found the issue in sc = get_spherical_coordinates(
[
(0.5 if i else -0.5),
0.5 if j else -0.5,
(1 if k else -1) * srt22,
]
)This is calling the function with a list instead of a Numpy array. Moreover, it is a mixture of floats and Numpy floats (since |
|
If I change it to: xyz = np.array(
[
[0.5 if i else -0.5],
[0.5 if j else -0.5],
[(1 if k else -1) * srt22],
]
)
sc = get_spherical_coordinates(xyz)and change the code in polar_angle: float = math.acos(xyz[2, 0] / r)it works well. It might be worth running the type checker on the tests as well as the rest of the code (I am not sure whether you do that). |
|
I sent #5166 with a fix to the test. |
|
I have closed #5135 with the expedient route of changing the failing test. There may be a real failure with unusual inputs, so there's a good chance we'll have to reopen that issue once a new use case is reported. i.e. someone finds their existing PDB code broke with NumPy 2.4. |
I hereby agree to dual licence this and any previous contributions under both
the Biopython License Agreement AND the BSD 3-Clause License.
I have read the
CONTRIBUTING.rstfile, have runpre-commitlocally, and understand that continuous integration checks will be used to
confirm the Biopython unit tests and style checks pass with these changes.
I have added my name to the alphabetical contributors listings in the files
NEWS.rstandCONTRIB.rstas part of this pull request, am listedalready, or do not wish to be listed. (This acknowledgement is optional.)
This addresses the expired deprecation in Numpy 2.4: https://numpy.org/devdocs/release/2.4.0-notes.html#raise-typeerror-on-attempt-to-convert-array-with-ndim-0-to-scalar
Closes #5135