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

Adjust num of parts to accomodate number of frames #4710

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

yuxuanzhuang
Copy link
Contributor

@yuxuanzhuang yuxuanzhuang commented Sep 20, 2024

Fixes #4685

Changes made in this Pull Request:

  • set n_parts to the total number of frames being analyzed if
    n_parts is bigger.

PR Checklist

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

Developers certificate of origin


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

Copy link

Linter Bot Results:

Hi @yuxuanzhuang! 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/10954201085/job/30415731957


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

@yuxuanzhuang
Copy link
Contributor Author

#4682 will pass with this fix.

Copy link

codecov bot commented Sep 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.53%. Comparing base (4fafd51) to head (3359bbd).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4710      +/-   ##
===========================================
- Coverage    93.54%   93.53%   -0.02%     
===========================================
  Files          173      185      +12     
  Lines        21434    22505    +1071     
  Branches      3981     3983       +2     
===========================================
+ Hits         20051    21049     +998     
- Misses         929     1002      +73     
  Partials       454      454              

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

Comment on lines +585 to +586
warnings.warn(f"Set to {n_parts} parts to match the total number "
"of frames being analyzed")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
warnings.warn(f"Set to {n_parts} parts to match the total number "
"of frames being analyzed")
warnings.warn(f"Set 'n_parts' to {n_parts} to match the total number "
"of frames being analyzed")

I think it's a bit clearer what is being tone this way? Feel free to ignore if you disagree.

Comment on lines +201 to +203
def test_reset_n_parts_to_n_frames(u):
# Issue #4685
a = FrameAnalysis(u.trajectory)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def test_reset_n_parts_to_n_frames(u):
# Issue #4685
a = FrameAnalysis(u.trajectory)
def test_reset_n_parts_to_n_frames(u):
"""
Issue #4685
https://github.com/MDAnalysis/mdanalysis/issues/4685
"""
a = FrameAnalysis(u.trajectory)

def test_reset_n_parts_to_n_frames(u):
# Issue #4685
a = FrameAnalysis(u.trajectory)
with pytest.warns(UserWarning, match='Set to'):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
with pytest.warns(UserWarning, match='Set to'):
with pytest.warns(UserWarning, match="Set 'n_parts' to"):

The change suggested above makes the test also a bit more specific.

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.

Array dimensions error during parallelization testing
6 participants