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

Add warning for future change of argument name for timeseries from asel -> atomgroup #4343

Merged
merged 12 commits into from
Dec 3, 2023

Conversation

hmacdope
Copy link
Member

@hmacdope hmacdope commented Nov 11, 2023

Fixes #3911

Changes made in this Pull Request:

  • Adds warning for future change of name in timeseries from asel to atomgroup.

PR Checklist

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

Developers certificate of origin


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

@pep8speaks
Copy link

pep8speaks commented Nov 11, 2023

Hello @hmacdope! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 294:1: W293 blank line contains whitespace
Line 331:80: E501 line too long (81 > 79 characters)

Line 990:52: E252 missing whitespace around parameter equals
Line 990:53: E252 missing whitespace around parameter equals
Line 1039:80: E501 line too long (81 > 79 characters)

Line 491:80: E501 line too long (91 > 79 characters)
Line 540:80: E501 line too long (81 > 79 characters)

Line 506:80: E501 line too long (87 > 79 characters)
Line 523:80: E501 line too long (84 > 79 characters)

Line 209:1: W293 blank line contains whitespace
Line 213:1: W293 blank line contains whitespace

Comment last updated at 2023-12-03 04:13:44 UTC

Copy link

github-actions bot commented Nov 11, 2023

Linter Bot Results:

Hi @hmacdope! 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/7074594385/job/19255806059


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

Copy link

codecov bot commented Nov 11, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f4005db) 93.36% compared to head (2954931) 93.37%.

Additional details and impacted files
@@            Coverage Diff             @@
##           develop    #4343     +/-   ##
==========================================
  Coverage    93.36%   93.37%             
==========================================
  Files          170      184     +14     
  Lines        22325    23453   +1128     
  Branches      4079     4085      +6     
==========================================
+ Hits         20844    21899   +1055     
- Misses         963     1036     +73     
  Partials       518      518             

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

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.

Cheers, couple of things though.

testsuite/MDAnalysisTests/coordinates/test_dcd.py Outdated Show resolved Hide resolved
testsuite/MDAnalysisTests/coordinates/test_dcd.py Outdated Show resolved Hide resolved
package/MDAnalysis/coordinates/DCD.py Outdated Show resolved Hide resolved
@@ -1023,6 +1023,12 @@ def timeseries(self, asel: Optional['AtomGroup']=None,

.. versionadded:: 2.4.0
"""
if asel:
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, we should support both keyword arguments and docs should reflect this.

@@ -525,6 +525,12 @@ def timeseries(self, asel=None, start=0, stop=-1, step=1, order='afc'):
ValueError now raised instead of NoDataError for empty input
AtomGroup
"""
if asel:
Copy link
Member

Choose a reason for hiding this comment

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

As above

Copy link
Member Author

Choose a reason for hiding this comment

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

Done :)

@hmacdope
Copy link
Member Author

@IAlibay still WIP ill ping for review when ready

@RMeli RMeli marked this pull request as draft November 17, 2023 21:34
@RMeli
Copy link
Member

RMeli commented Nov 17, 2023

@hmacdope given your comment above I converted it to a draft for now.

@hmacdope hmacdope marked this pull request as ready for review December 3, 2023 04:35
@hmacdope hmacdope requested a review from IAlibay December 3, 2023 04:36
@hmacdope
Copy link
Member Author

hmacdope commented Dec 3, 2023

@IAlibay @RMeli assuming tests pass I think this is ready for another review.

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.

Cheers

@hmacdope hmacdope merged commit a474dd3 into MDAnalysis:develop Dec 3, 2023
23 checks passed
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.

Change name of asel in timeseries to something more comprehensible.
4 participants