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

MAINT: encore Windows type fixes #4369

Merged

Conversation

tylerjereddy
Copy link
Member

@tylerjereddy tylerjereddy commented Dec 15, 2023

  • in NumPy 2.0.0, the default integer size on Windows will be 64-bit; we have 10 related test failures on Windows alongside NumPy main at the moment, and this patch fixes those failures by forcing a reversion to the old behavior, effectively kicking the can down the road

  • I think we're going to proceed with a similar solution upstream for now (i.e., BUG: wheel runs have a *lot* of test fails at the moment. scipy/scipy#19605 (comment));

  • I haven't kept up with whether this should actually be targeted at the encore "subproject" now with the reorg happening; feel free to move it if so...

[skip cirrus]


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

* in NumPy `2.0.0`, the default integer size on Windows
will be 64-bit; we have 10 related test failures on Windows
alongside NumPy `main` at the moment, and this patch fixes
those failures by forcing a reversion to the old behavior,
effectively kicking the can down the road

* I think we're going to proceed with a similar solution upstream
for now (i.e., scipy/scipy#19605 (comment));

[skip cirrus]
Copy link

Linter Bot Results:

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

There are currently no issues detected! 🎉

@IAlibay
Copy link
Member

IAlibay commented Dec 15, 2023

With encore moving to MDAEncore, I'm not sure if this fix should go here or there or both :/

Copy link

codecov bot commented Dec 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2d6a8ce) 93.37% compared to head (d1424dc) 93.37%.

Additional details and impacted files
@@            Coverage Diff             @@
##           develop    #4369     +/-   ##
==========================================
  Coverage    93.37%   93.37%             
==========================================
  Files          170      184     +14     
  Lines        22340    23453   +1113     
  Branches      4085     4085             
==========================================
+ Hits         20859    21899   +1040     
- Misses         963     1036     +73     
  Partials       518      518             

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

@tylerjereddy
Copy link
Member Author

Ok, let me know what you want me to do. You have a PR to improve optional dep resolution right? Does that need to go in soon/first?

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

Given it's a single line fix, I'm tempted to say let's merge this and we can port it over (if that's ok with you @tylerjereddy) - does this sound reasonable with you @orbeckst ?

@IAlibay
Copy link
Member

IAlibay commented Dec 15, 2023

We do need to deprecate Encore properly, I'll raise an issue in the morning.

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

Thanks. Fix here and let’s port to MDAKit encore.

@IAlibay IAlibay merged commit 01f4277 into MDAnalysis:develop Dec 16, 2023
25 checks passed
@tylerjereddy tylerjereddy deleted the treddy_win_numpy2_int_fixes branch December 16, 2023 17:12
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.

3 participants