-
Notifications
You must be signed in to change notification settings - Fork 661
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: Pca.py doctest error #4377
Fix: Pca.py doctest error #4377
Conversation
I will update the change log when reviewed by maintainers. |
Linter Bot Results:Hi @HeetVekariya! Thanks for making this PR. We linted your code and found the following: There are currently no issues detected! 🎉 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4377 +/- ##
===========================================
- Coverage 93.66% 93.64% -0.03%
===========================================
Files 168 180 +12
Lines 21248 22327 +1079
Branches 3917 3917
===========================================
+ Hits 19902 20908 +1006
- Misses 888 961 +73
Partials 458 458 ☔ View full report in Codecov by Sentry. |
@lilyminium could please handle this PR? I think rmsip was your code, wasn't it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening this PR, @HeetVekariya -- however, I'm not sure that changing the output is the best solution to the failing doctest. The differences are somewhere around the ~15th decimal place, which seems to me to simply arise from precision error. IMO a more permanent solution to this issue might be to change it into a format that only prints out maybe the first 6 decimal places, using something like string formatting (with print
) or rounding -- what do you think?
|
Yes sure please do, rounding seems like the best solution here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM but I'm confused why CI is failing -- restarting now.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Sorry for the delay -- thanks for fixing this @HeetVekariya! Could you please update the CHANGELOG? |
…into fix/pca-doctest-errors
@lilyminium sorry for late response, I have updated the change log. |
@lilyminium could you please check in on this PR? You approved it a while back but it hasn't been merged. Thanks! |
Thanks @HeetVekariya -- merging now! |
Thank you for merging 🙏 |
Partially address #3925
Changes made in this Pull Request:
0.38147609331128324
0.17478244043688052
0.3814760933112831
0.17478244043688054
PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4377.org.readthedocs.build/en/4377/