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

AlignTraj Writer kwargs fix #4565

Merged
merged 9 commits into from
May 26, 2024
Merged

Conversation

ljwoods2
Copy link
Contributor

@ljwoods2 ljwoods2 commented Apr 12, 2024

Fixes #4564

Changes made in this Pull Request:

  • Updated AlignTraj's writer constructor to take kwargs
  • Wrote test in test_align.py using xtc writer with "precision" kwarg

PR Checklist

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

Developers certificate of origin


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

@pep8speaks
Copy link

pep8speaks commented Apr 12, 2024

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

Line 1672:20: W292 no newline at end of file

Comment last updated at 2024-05-26 22:47:41 UTC

Copy link

github-actions bot commented Apr 12, 2024

Linter Bot Results:

Hi @ljwoods2! 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/9246734685/job/25434607268


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 Apr 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.41%. Comparing base (2c6e21f) to head (52aaedb).
Report is 18 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4565      +/-   ##
===========================================
- Coverage    93.64%   93.41%   -0.24%     
===========================================
  Files          168      180      +12     
  Lines        21254    22167     +913     
  Branches      3918     3919       +1     
===========================================
+ Hits         19904    20707     +803     
- Misses         892     1000     +108     
- Partials       458      460       +2     

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

package/CHANGELOG Outdated Show resolved Hide resolved
package/MDAnalysis/analysis/align.py Outdated Show resolved Hide resolved
@yuxuanzhuang yuxuanzhuang self-assigned this Apr 27, 2024
Copy link
Contributor

@yuxuanzhuang yuxuanzhuang left a comment

Choose a reason for hiding this comment

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

Thanks so much! The functional parts all look good to me!

Regarding your final black modification, please see #2450 for details but you should only format the lines you change to avoid complaints from #4565 (comment).

@ljwoods2
Copy link
Contributor Author

My apologies, I forgot to turn off my autoformatter

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.

LGTM thanks @ljwoods2

@hmacdope hmacdope merged commit 81062f3 into MDAnalysis:develop May 26, 2024
13 of 21 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.

AlignTraj doesn't accept writer kwargs
4 participants