MAINT: shims for array copy semantics #4482

tylerjereddy · 2024-03-02T04:25:36Z

Fixes MAINT: test failures/segfaults against NumPy main #4481
this gets rid of segfaults/many test failurse against latest NumPy main while still passing the full suite against NumPy 1.26.4; it is pretty hard to test since needs special branch of SciPy (for the same reason, upstream hasn't fully resolved copy semantics)
I still see 2-3 test failures against main locally, but likely still a large improvement; may want to let this sit a little bit while the upstream storm settles, but I suspect this is the gist of the shims we'd want

One thing I didn't try was building against NumPy main and testing against 1.26.4, just did each completely separately (that's a good thing to try though). Upstream pre-release wheels will probably be delayed reflecting the related issues for a bit.

Getting late, so short explanation--copy=False got stricter, copy=None is the "try your best not to copy, but you can if you need to" old NumPy way.

📚 Documentation preview 📚: https://mdanalysis--4482.org.readthedocs.build/en/4482/

* Fixes MDAnalysis#4481 * this gets rid of segfaults/many test failurse against latest NumPy `main` while still passing the full suite against NumPy `1.26.4`; it is pretty hard to test since needs special branch of SciPy (for the same reason, upstream hasn't fully resolved copy semantics) * I still see 2-3 test failures against `main` locally, but likely still a large improvement; may want to let this sit a little bit while the upstream storm settles, but I suspect this is the gist of the shims we'd want

github-actions · 2024-03-02T04:27:53Z

Linter Bot Results:

Hi @tylerjereddy! Thanks for making this PR. We linted your code and found the following:

There are currently no issues detected! 🎉

codecov · 2024-03-02T04:34:21Z

Codecov Report

Attention: Patch coverage is 69.23077% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 89.52%. Comparing base (2c1aa4b) to head (ff93d70).
Report is 14 commits behind head on develop.

Files	Patch %	Lines
package/MDAnalysis/lib/util.py	20.00%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4482      +/-   ##
===========================================
- Coverage    93.38%   89.52%   -3.86%     
===========================================
  Files          171      180       +9     
  Lines        21744    22333     +589     
  Branches      4014     3619     -395     
===========================================
- Hits         20305    19993     -312     
- Misses         952     1891     +939     
+ Partials       487      449      -38

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tylerjereddy · 2024-03-02T21:31:30Z

package/MDAnalysis/lib/formats/cython_util.pyx

+    if np.__version__[0] == "2":
+        copy = None
+    else:
+        copy = False


A more concise/idiomatic upstream approach was applied in the revised version of my patch in case we care about that: https://github.com/scipy/scipy/pull/20172/files#diff-d28fbb0be287769c763ce61b0695feb0a6ca0e67b28bafee20526b46be7aaa84R59

Basically, abstracting the check to avoid duplication and using np.lib.NumpyVersion instead of pep440 or a raw string check like here. We probably don't need to worry about 2.0.0dev0 versions though, so we may care less about that, though the version check duplication I've added more broadly is indeed a bit much.

tylerjereddy · 2024-03-02T21:33:07Z

package/MDAnalysis/lib/formats/cython_util.pyx

@@ -71,7 +71,7 @@ cdef class ArrayWrapper:
        self.data_type = data_type
        self.ndim = ndim

-    def __array__(self):
+    def __array__(self, copy=None):


here and elswhere we should probably also add dtype=None before copy=None, similar to the revisions added in my PR upstream at: scipy/scipy#20172

It doesn't seem to matter for testing purposes at the moment, but adding the dtype=None is more formally correct now.

orbeckst · 2024-03-11T20:13:37Z

@hmacdope can you please have a look? Keeping on top of scipy/numpy changes is important. Thanks!

tylerjereddy · 2024-03-11T20:26:10Z

I should update this soon to reduce code duplication and test it against the beta release if it comes out today or tomorrow.

tylerjereddy · 2024-03-11T20:31:16Z

I think Sebastian also told me to use asarray instead of array in most of these cases. There used to be a good performance reason to use plain np.array() but I think that is long gone now.

hmacdope

Query about perhaps using a function to avoid some repetition.

hmacdope · 2024-03-12T00:58:46Z

package/MDAnalysis/lib/transformations.py

@@ -970,8 +978,12 @@ def superimposition_matrix(v0, v1, scaling=False, usesvd=True):
    True

    """
-    v0 = np.array(v0, dtype=np.float64, copy=False)[:3]
-    v1 = np.array(v1, dtype=np.float64, copy=False)[:3]
+    if np.__version__[0] == "2":


Do we want to wrap this in a helper routine to avoid repetition eg:

v0 = np.array(v0, dtype=np.float64, copy=no_copy_shim())[:3] def no_copy_shim(): if np.__version__[0] == "2": copy=None else: copy=False ...

Ill leave it up to you @tylerjereddy?

* abstract the array copy semantics (NumPy 1.x vs. 2.x) handling to a utility function to reduce code duplication * add a missing `dtype=None` to an `__array__` signature, for more formal correctness with NumPy 2.x

pep8speaks · 2024-03-12T19:16:16Z

Hello @tylerjereddy! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file package/MDAnalysis/lib/transformations.py:

Line 519:80: E501 line too long (80 > 79 characters)

Comment last updated at 2024-04-07 20:50:51 UTC

tylerjereddy · 2024-03-12T19:22:20Z

I pushed in an update to reduce code duplication and use a more formally correct __array__ signature.

I checked the status of the updated PR vs. latest develop for full testsuite against the pre-release wheels for NumPy/the ecosystem. The segfaults are gone even for develop (NumPy made things a bit more backcompat friendly in time for the beta), but there are 9 test failures for develop on x86_64 Linux at the moment. On this branch, that drops down to 2 failures:

FAILED MDAnalysisTests/analysis/test_helix_analysis.py::TestHELANAL::test_residue_gaps_split - assert 295 == 1
FAILED MDAnalysisTests/analysis/test_helix_analysis.py::TestHELANAL::test_residue_gaps_no_split - assert 99 == 1

From a quick scan of the diff again, the only other place where I think I'm misbehaving a bit is in using a raw string version check instead of using np.lib.NumpyVersion for better future proofing/maintainability.

hmacdope · 2024-03-19T21:28:56Z

@tylerjereddy is the plan to move ahead here before addressing ^ test failures?

tylerjereddy · 2024-03-19T21:42:40Z

Possibly yeah, I didn't have immediate plans to tackle those, and may be quite different if they're real failures we need to worry about vs. some third-party issues (I didn't check).

tylerjereddy · 2024-03-29T17:05:58Z

Note that NumPy 2.0.0rc1 (ABI stable) is now imminent since the new pybind11 release is out. I'll be under a fair bit of pressure to get the supporting SciPy 1.13.0 RC2 and "final" release out within a few days (at most) of the NumPy RC1 posting to PyPI. I don't think MDA faces the same pressure, but we should retest this branch against NumPy RC1 as well once it is posted very soon, since that version has stability guarantees that the beta didn't (and can be used to build wheels to post on PyPI for NumPy 2 support).

tylerjereddy · 2024-04-06T00:03:56Z

Ecosystem support summary table is here: numpy/numpy#26191

I added us at the bottom below OpenCV. I need to circle back here soon to check against RC1, ping me if I don't.

hmacdope · 2024-04-06T04:31:57Z

Thanks @tylerjereddy, much appreciated.

* Update `vector_of_best_fit()` function in the helix analysis code to avoid use of `np.linalg.linalg` namespace, which is deprecated/private in NumPy `2.x`. Instead, just access `svd` via the public `np.linalg` namespace. * This fixes the last two remaining test failures for MDAnalysis against NumPy `2.0.0rc1`. * Switch the array `copy` shim to use the more appropriate `NumpyVersion` rather than a raw string check for version `2`.

tylerjereddy · 2024-04-07T20:56:15Z

Update here:

The latest MDAnalysis develop branch has 96 failures and 430 errors when built and tested against the ABI-stable NumPy 2.0.0rc1 on x86_64 Linux locally.
With the commit I just pushed in, the feature branch here passes the full MDA testsuite when built and tested against NumPy 2.0.0rc1 on x86_64 Linux locally.
I also confirmed that the full suite passes on this feature branch when we build MDA against NumPy 2.0.0rc1, then downgrade at run/test time to NumPy 1.23.2, so backwards compat seems to be working as expected for us.

I believe I've addressed both my own comments and the reviewer comments so far, and we appear to be in good shape for NumPy 2.x series, at least on x86_64 Linux.

IAlibay

Just one blocking question from me, otherwise this looks good. Thanks @tylerjereddy 🙏

IAlibay · 2024-04-07T23:49:28Z

package/MDAnalysis/analysis/helix_analysis.py

@@ -121,7 +121,7 @@ def vector_of_best_fit(coordinates):
    """
    centered = coordinates - coordinates.mean(axis=0)
    Mt_M = np.matmul(centered.T, centered)
-    u, s, vh = np.linalg.linalg.svd(Mt_M)
+    u, s, vh = np.linalg.svd(Mt_M)


Just checking - how backwards compatible is this? Do we need to try except an import or will this work with older versions of numpy?

I'm pretty sure we're safe since I tested the runtime downgrade to our oldest supported version (1.23.2) without issue.

If you don't believe me, you could use the version switcher in the NumPy docs and it is there in version 1.13: https://numpy.org/doc/1.13/reference/generated/numpy.linalg.svd.html

That's pretty ancient now

So sorry @tylerjereddy - I completely missed this from your previous comment message (it's the second time in a row over two PRs really sorry about this).

IAlibay

lgtm, I cycled CI - looks like all the failures are codecov related.

hmacdope

LGTM @tylerjereddy, thanks for taking this on. I will cycle CI to see if we can get a few more greens.

tylerjereddy · 2024-04-11T17:30:18Z

I think codecov was removed from a number of upstream projects because of flakiness/security concerns that kept cropping up. MDA might want to consider doing the same (as much as I hate to see removal of a method of measuring test coverage).

IAlibay · 2024-04-21T14:15:47Z

I think codecov was removed from a number of upstream projects because of flakiness/security concerns that kept cropping up. MDA might want to consider doing the same (as much as I hate to see removal of a method of measuring test coverage).

Yeah it's been a bit annoying lately - issue has been opened and temporary fix PRed. I'm going to go ahead and merge this, CI passed fine outside of report uploading. Thanks @tylerjereddy !

tylerjereddy added the maintainability label Mar 2, 2024

github-actions bot added the Component-lib label Mar 2, 2024

tylerjereddy commented Mar 2, 2024

View reviewed changes

IAlibay requested a review from hmacdope March 10, 2024 20:13

IAlibay assigned hmacdope Mar 10, 2024

IAlibay added the Priority-High label Mar 10, 2024

hmacdope requested changes Mar 12, 2024

View reviewed changes

MAINT: PR 4482 revisions

320a940

* abstract the array copy semantics (NumPy 1.x vs. 2.x) handling to a utility function to reduce code duplication * add a missing `dtype=None` to an `__array__` signature, for more formal correctness with NumPy 2.x

tylerjereddy mentioned this pull request Apr 6, 2024

Ecosystem compatibility with numpy 2.0 numpy/numpy#26191

Open

github-actions bot added the Component-Analysis label Apr 7, 2024

IAlibay requested changes Apr 7, 2024

View reviewed changes

IAlibay approved these changes Apr 8, 2024

View reviewed changes

hmacdope approved these changes Apr 11, 2024

View reviewed changes

tylerjereddy mentioned this pull request Apr 19, 2024

TST, MAINT: test_thread_limit_apply failure with NumPy 2.1.0.dev #4570

Open

IAlibay merged commit e8637a9 into MDAnalysis:develop Apr 21, 2024
15 of 23 checks passed

tylerjereddy deleted the treddy_issue_4481 branch April 21, 2024 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAINT: shims for array copy semantics #4482

MAINT: shims for array copy semantics #4482

tylerjereddy commented Mar 2, 2024 •

edited by github-actions bot

github-actions bot commented Mar 2, 2024 •

edited

codecov bot commented Mar 2, 2024 •

edited

tylerjereddy Mar 2, 2024

tylerjereddy Mar 2, 2024

orbeckst commented Mar 11, 2024

tylerjereddy commented Mar 11, 2024

tylerjereddy commented Mar 11, 2024

hmacdope left a comment

hmacdope Mar 12, 2024

pep8speaks commented Mar 12, 2024 •

edited

tylerjereddy commented Mar 12, 2024

hmacdope commented Mar 19, 2024

tylerjereddy commented Mar 19, 2024

tylerjereddy commented Mar 29, 2024

tylerjereddy commented Apr 6, 2024

hmacdope commented Apr 6, 2024

tylerjereddy commented Apr 7, 2024 •

edited

IAlibay left a comment

IAlibay Apr 7, 2024

tylerjereddy Apr 8, 2024

IAlibay Apr 8, 2024

IAlibay left a comment

hmacdope left a comment

tylerjereddy commented Apr 11, 2024 •

edited

IAlibay commented Apr 21, 2024

MAINT: shims for array copy semantics #4482

MAINT: shims for array copy semantics #4482

Conversation

tylerjereddy commented Mar 2, 2024 • edited by github-actions bot

github-actions bot commented Mar 2, 2024 • edited

Linter Bot Results:

codecov bot commented Mar 2, 2024 • edited

Codecov Report

tylerjereddy Mar 2, 2024

Choose a reason for hiding this comment

tylerjereddy Mar 2, 2024

Choose a reason for hiding this comment

orbeckst commented Mar 11, 2024

tylerjereddy commented Mar 11, 2024

tylerjereddy commented Mar 11, 2024

hmacdope left a comment

Choose a reason for hiding this comment

hmacdope Mar 12, 2024

Choose a reason for hiding this comment

pep8speaks commented Mar 12, 2024 • edited

Comment last updated at 2024-04-07 20:50:51 UTC

tylerjereddy commented Mar 12, 2024

hmacdope commented Mar 19, 2024

tylerjereddy commented Mar 19, 2024

tylerjereddy commented Mar 29, 2024

tylerjereddy commented Apr 6, 2024

hmacdope commented Apr 6, 2024

tylerjereddy commented Apr 7, 2024 • edited

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay Apr 7, 2024

Choose a reason for hiding this comment

tylerjereddy Apr 8, 2024

Choose a reason for hiding this comment

IAlibay Apr 8, 2024

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

hmacdope left a comment

Choose a reason for hiding this comment

tylerjereddy commented Apr 11, 2024 • edited

IAlibay commented Apr 21, 2024

tylerjereddy commented Mar 2, 2024 •

edited by github-actions bot

github-actions bot commented Mar 2, 2024 •

edited

codecov bot commented Mar 2, 2024 •

edited

pep8speaks commented Mar 12, 2024 •

edited

tylerjereddy commented Apr 7, 2024 •

edited

tylerjereddy commented Apr 11, 2024 •

edited