Skip to content
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

NEP29, update to Numpy 1.22[.3] #4160

Merged
merged 4 commits into from
Jun 23, 2023
Merged

NEP29, update to Numpy 1.22[.3] #4160

merged 4 commits into from
Jun 23, 2023

Conversation

IAlibay
Copy link
Member

@IAlibay IAlibay commented Jun 1, 2023

We won't be making a release in the next month, and June 23rd is when we go to Numpy 1.22+

Here I'm actually pinning builds to 1.22.3 because it makes things a bit cleaner (there's issues on a couple of archs and windows which makes life just eaiser to pin there). If we think we should pin as much as we can to 1.22.0, I can do that too if necessary.

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

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

@github-actions
Copy link

github-actions bot commented Jun 1, 2023

Linter Bot Results:

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

There are currently no issues detected! 🎉

@codecov
Copy link

codecov bot commented Jun 1, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (07c9d1f) 93.61% compared to head (fb9c7cf) 93.61%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #4160   +/-   ##
========================================
  Coverage    93.61%   93.61%           
========================================
  Files          193      193           
  Lines        25168    25168           
  Branches      4058     4058           
========================================
  Hits         23560    23560           
  Misses        1092     1092           
  Partials       516      516           
Impacted Files Coverage Δ
package/MDAnalysis/lib/mdamath.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@IAlibay
Copy link
Member Author

IAlibay commented Jun 2, 2023

This mypy error makes absolutely no sense to me - the numpy docs specifically say that input is expected to be array_like..

@RMeli
Copy link
Member

RMeli commented Jun 6, 2023

Odd... From a quick glance at the source code it seems that it requires arrays though? Explicitly converting (if needed) seems to solve the issue:

diff --git a/package/MDAnalysis/lib/mdamath.py b/package/MDAnalysis/lib/mdamath.py
index 260bf0b87..d89805dca 100644
--- a/package/MDAnalysis/lib/mdamath.py
+++ b/package/MDAnalysis/lib/mdamath.py
@@ -104,7 +104,7 @@ def normal(vec1: npt.ArrayLike, vec2: npt.ArrayLike) -> npt.NDArray:
     .. versionchanged:: 0.11.0
        Moved into lib.mdamath
     """
-    normal: npt.NDArray = np.cross(vec1, vec2)
+    normal: npt.NDArray = np.cross(np.asarray(vec1), np.asarray(vec2))
     n = norm(normal)
     if n == 0.0:
         return normal  # returns [0,0,0] instead of [nan,nan,nan]
@@ -171,7 +171,7 @@ def stp(vec1: npt.ArrayLike, vec2: npt.ArrayLike, vec3: npt.ArrayLike) -> float:
     .. versionchanged:: 0.11.0
        Moved into lib.mdamath
     """
-    return np.dot(vec3, np.cross(vec1, vec2))
+    return np.dot(vec3, np.cross(np.asarray(vec1), np.asarray(vec2)))


 def dihedral(ab: npt.ArrayLike, bc: npt.ArrayLike, cd: npt.ArrayLike) -> float:

Proof: RMeli#3

@IAlibay
Copy link
Member Author

IAlibay commented Jun 6, 2023

@tylerjereddy as the numpy expert here, do you have any ideas why the array_like type check would be falling here?

@tylerjereddy
Copy link
Member

I'm on vacation with just my phone until Sunday, but is there any code path where the arguments might be object arrays? I think static analysis prohibits that while runtime allows it, and in 1.22 release notes typing improvements are noted. @BvB93 implemented many of the NumPy static analysis improvements so they may know more.

@tylerjereddy
Copy link
Member

Seems to expect bool type, I wonder why, can't check till Monday though

@BvB93
Copy link

BvB93 commented Jun 7, 2023

Looking at the errors this seems to be another case of python/mypy#11347, an upstream bug wherein mypy fails to properly deal with overload ambiguity (due to a statically unknown dtype). There was a PR from ~1.5 years ago to fix the issue, but it seems to have gotten stuck in limbo.

@IAlibay
Copy link
Member Author

IAlibay commented Jun 23, 2023

Thanks so much for looking at this @BvB93 !

I'm going to treat this for what it is, and just type ignore. Hopefully that upstream issue will get addressed at some point!

package/CHANGELOG Outdated Show resolved Hide resolved
Co-authored-by: Rocco Meli <[email protected]>
@IAlibay IAlibay merged commit 4d6d668 into develop Jun 23, 2023
@IAlibay IAlibay deleted the numpy122 branch June 23, 2023 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants