-
Notifications
You must be signed in to change notification settings - Fork 769
Docs: Fix doctest failure in rms.py #5205
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
Docs: Fix doctest failure in rms.py #5205
Conversation
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.
Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on GitHub Discussions so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS as part of this PR.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5205 +/- ##
========================================
Coverage 92.72% 92.73%
========================================
Files 180 180
Lines 22475 22475
Branches 3190 3190
========================================
+ Hits 20841 20843 +2
+ Misses 1177 1176 -1
+ Partials 457 456 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Can you show the output of the doc testing before and after your fix? |
orbeckst
left a comment
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.
See comments inline.
Please also show explicitly doc tests output before/after your change.
package/MDAnalysis/analysis/rms.py
Outdated
| >>> float(rmsd(A, B, center=True)) | ||
| 6.838544558398293 |
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.
Nobody would normally case the output with float. First and foremost, the docs must be human-readable/human-centered. If the output is different then you could show the different output.
Find a way to make the doc-tests work without introducing code in the example that only serves the doc tests.
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 pointing that out, @orbeckst You’re right the float() cast was making the example look a bit off.
I’ve switched the example back to rmsd(A, B, center=True) and updated the expected output to np.float64(6.838544558398293) so it lines up with what NumPy actually returns , I tested it locally and the doctests are passing now.
|
Can you please copy and paste how you ran the local test and the relevant output? Thanks. |
yeah sure @orbeckst , here are the local doctest outputs as requested: Before (Failing): |
orbeckst
left a comment
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.
Thank you for the fix! LGTM
Contributes to #3925
Changes made in this Pull Request:
rms.pydoctest failure by updated the expected output to np.float64(6.838544558398293) so it lines up with what NumPy actually returnspytest --doctest-modules.LLM / AI generated code disclosure
PR Checklist
📚 Documentation preview 📚: https://mdanalysis--5205.org.readthedocs.build/en/5205/