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.

Sign up for GitHub

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

Jump to bottom

AlignTraj Writer kwargs fix #4565

Merged
merged 9 commits into from
May 26, 2024
Merged

AlignTraj Writer kwargs fix #4565

merged 9 commits into from
May 26, 2024

Conversation

ljwoods2
Copy link
Contributor

@ljwoods2 ljwoods2 commented Apr 12, 2024 •
edited

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 •
edited

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

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 12, 2024 •
edited

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!

@codecov Codecov
Copy link

codecov bot commented Apr 12, 2024 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.23%. Comparing base (2c6e21f) to head (405f7e8).

❗ Current head 405f7e8 differs from pull request most recent head 52aaedb

Please upload reports for the commit 52aaedb to get more accurate results.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #4565      +/-   ##
===========================================
- Coverage    93.64%   93.23%   -0.42%     
===========================================
  Files          168       12     -156     
  Lines        21254     1079   -20175     
  Branches      3918        0    -3918     
===========================================
- Hits         19904     1006   -18898     
+ Misses         892       73     -819     
+ Partials       458        0     -458

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

yuxuanzhuang
yuxuanzhuang requested changes Apr 27, 2024
View reviewed changes
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
yuxuanzhuang
yuxuanzhuang approved these changes May 20, 2024
View reviewed changes
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

hmacdope
hmacdope approved these changes May 26, 2024
View reviewed changes
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
Reviewers

@hmacdope hmacdope hmacdope approved these changes

@yuxuanzhuang yuxuanzhuang yuxuanzhuang approved these changes

Assignees

@yuxuanzhuang yuxuanzhuang

Labels
Component-Analysis
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AlignTraj doesn't accept writer kwargs
4 participants
@ljwoods2 @pep8speaks @hmacdope @yuxuanzhuang