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

Increase minimum numpy version to 1.21 #3983

Merged
merged 7 commits into from
Jan 20, 2023
Merged

Increase minimum numpy version to 1.21 #3983

merged 7 commits into from
Jan 20, 2023

Conversation

IAlibay
Copy link
Member

@IAlibay IAlibay commented Jan 2, 2023

As per NEP29, the 2.5.0 release of MDA will happen ~ March 2023 and will be Python 3.8+ & NumPy 1.21+

See: https://numpy.org/neps/nep-0029-deprecation_policy.html#support-table

Changes made in this Pull Request:

  • Raised the minimum NumPy version to 1.21.0
  • Change some uses of np.ndarray to npt.NDArray for typing

PR Checklist

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

@codecov
Copy link

codecov bot commented Jan 2, 2023

Codecov Report

Base: 93.50% // Head: 93.50% // No change to project coverage 👍

Coverage data is based on head (4d419fc) compared to base (130e862).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3983   +/-   ##
========================================
  Coverage    93.50%   93.50%           
========================================
  Files          190      190           
  Lines        24943    24943           
  Branches      3523     3523           
========================================
  Hits         23322    23322           
  Misses        1100     1100           
  Partials       521      521           
Impacted Files Coverage Δ
package/MDAnalysis/lib/distances.py 97.62% <100.00%> (ø)
package/MDAnalysis/lib/mdamath.py 100.00% <100.00%> (ø)
package/MDAnalysis/lib/pkdtree.py 83.83% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@IAlibay IAlibay changed the title Increase minimum numpy version to 1.21 [wip] Increase minimum numpy version to 1.21 Jan 2, 2023
@IAlibay IAlibay requested a review from jbarnoud January 2, 2023 22:25
@IAlibay IAlibay changed the title [wip] Increase minimum numpy version to 1.21 Increase minimum numpy version to 1.21 Jan 2, 2023
@IAlibay
Copy link
Member Author

IAlibay commented Jan 2, 2023

I think that might be fine now, I'm not a typing expert though so I'm heavily relying on mypy to tell me if I'm wrong (also I've only made changes where it was indicated in lib).

@IAlibay IAlibay added this to the 2.5.0 milestone Jan 2, 2023
Copy link
Member

@hmacdope hmacdope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick question.

reference_positions: np.ndarray = reference # type: ignore
configuration_positions: np.ndarray = configuration # type: ignore
reference_positions: npt.NDArray = reference # type: ignore
configuration_positions: npt.NDArray = configuration # type: ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to have subtyping on eg npt.NDArray[np.float32] ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure, but I'd probably ask that this get punted to a separate issue (unless these changes lessen the quality of typing from what it used to be)?

I'd prefer not move the goalpost here from "bumping up our numpy requirements" to "incidentally dealing with a ton of typing issues"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep totally fair. I'll raise an issue once merged. The typing here is improvement for sure, it can just be more stringent but agree its a separate issue.

Copy link
Member

@hmacdope hmacdope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No expert on all the places CI needs changing but the use of the new typing looks good.

@hmacdope
Copy link
Member

Note fixes #3748

@IAlibay IAlibay merged commit f966ec2 into develop Jan 20, 2023
@IAlibay IAlibay deleted the numpy-1.21 branch January 20, 2023 15:27
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.

2 participants