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

Fix future warning from GSDReader, go cutting edge #4153

Merged
merged 14 commits into from Jun 28, 2023

Conversation

richardjgowers
Copy link
Member

@richardjgowers richardjgowers commented May 26, 2023

Fixes #4152

Changes made in this Pull Request:

PR Checklist

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

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

@IAlibay
Copy link
Member

IAlibay commented May 26, 2023

What's the regression like here? i.e. what minimum version of gsd will work here?

@codecov
Copy link

codecov bot commented May 26, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (b943ca0) 93.61% compared to head (2fc3b58) 93.61%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #4153   +/-   ##
========================================
  Coverage    93.61%   93.61%           
========================================
  Files          193      193           
  Lines        25170    25170           
  Branches      4059     4059           
========================================
  Hits         23562    23562           
  Misses        1092     1092           
  Partials       516      516           
Impacted Files Coverage Δ
package/MDAnalysis/coordinates/GSD.py 91.89% <100.00%> (ø)
package/MDAnalysis/topology/GSDParser.py 92.85% <100.00%> (ø)

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

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.

The minimum numpy failure highlights the issue here, we can't make this change within the current NEP29 window.

@IAlibay
Copy link
Member

IAlibay commented May 26, 2023

Next NEP29 window is June 23 - let's try this after 2.5 gets released

@IAlibay IAlibay changed the title fix future warning from GSDReader [HOLD 2.6] fix future warning from GSDReader May 26, 2023
@IAlibay
Copy link
Member

IAlibay commented Jun 1, 2023

So the 'r' writing mode was only introduced in the very latest version of GSD (released like 2 weeks ago). I don't know what the best approach is here - do we force folks to install the latest GSD, or do we leave it for now and accept that we are going to get a futurewarning for some cases? Or we support both the old and new version of gsd and switch between the two modes as necessary.

@IAlibay
Copy link
Member

IAlibay commented Jun 23, 2023

@richardjgowers I'mk going to use this PR as the place to test GSD 3.0.1. My view here is that now that it's not a core dependency we can just go cutting edge (if CI works)?

@IAlibay IAlibay changed the title [HOLD 2.6] fix future warning from GSDReader Fix future warning from GSDReader, go cutting edge Jun 23, 2023
@github-actions
Copy link

github-actions bot commented Jun 23, 2023

Linter Bot Results:

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

Some issues were found with the formatting of your code.

Code Location Outcome
main package ⚠️ Possible failure
testsuite ⚠️ Possible failure

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/5387612556/jobs/9779169252


Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

@IAlibay IAlibay changed the title Fix future warning from GSDReader, go cutting edge [DEPEND #4106Fix future warning from GSDReader, go cutting edge Jun 23, 2023
@IAlibay IAlibay changed the title [DEPEND #4106Fix future warning from GSDReader, go cutting edge [DEPEND #4106] Fix future warning from GSDReader, go cutting edge Jun 23, 2023
@IAlibay IAlibay changed the title [DEPEND #4106] Fix future warning from GSDReader, go cutting edge [DEPEND #4160] Fix future warning from GSDReader, go cutting edge Jun 23, 2023
@IAlibay
Copy link
Member

IAlibay commented Jun 23, 2023

Of course, NEP29...

@IAlibay IAlibay changed the title [DEPEND #4160] Fix future warning from GSDReader, go cutting edge Fix future warning from GSDReader, go cutting edge Jun 23, 2023
@IAlibay IAlibay dismissed their stale review June 23, 2023 19:53

can't self approve

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.

Generally ok with the approach.

If we normally announce version bumps in deps then we should add a note to CHANGELOG but I leave this to you.

@orbeckst
Copy link
Member

Failures such as

E       AttributeError: module 'gsd' has no attribute '__version__'

I am going to revoke my unconditional "approve" until these issues are addressed.

@orbeckst orbeckst self-requested a review June 23, 2023 23:03
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.

I am cool with gsd > 3.0.0 but the test failures need to be fixed first.

@IAlibay IAlibay requested a review from orbeckst June 27, 2023 09:44
@IAlibay IAlibay merged commit 6532f7b into develop Jun 28, 2023
24 checks passed
@RMeli RMeli deleted the issue-4152-gsd_rb_mode branch August 16, 2023 07:00
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.

gsd FutureWarning
3 participants