-
Notifications
You must be signed in to change notification settings - Fork 768
Add test to cover unit conversion path in DMSReader #5180
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
base: develop
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5180 +/- ##
===========================================
- Coverage 92.72% 92.72% -0.01%
===========================================
Files 180 180
Lines 22472 22475 +3
Branches 3188 3190 +2
===========================================
+ Hits 20838 20840 +2
- Misses 1176 1177 +1
Partials 458 458 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
IAlibay
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.
This test is insufficient, it only flushes the code (and actually doesn't increase coverage for the dimensions path). Checking the size of the array doesn't do much to solve the initial issue.
A proper test here would include actual checks on the numbers to make sure all the results pre and post conversion are as expected. This would include tests for positions, velocities, and dimensions.
|
Thanks for the detailed feedback |
RMeli
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.
@IAlibay concerning velocities I think the DMS files we currently have in MDAnalysisTests have only one frame and no velocities. Do you know of a file with velocities that we have easily available?
| np.testing.assert_allclose( | ||
| u_conv.atoms.positions, | ||
| u_native.atoms.positions, | ||
| rtol=1e-6, |
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.
Does the default rtol fails here?
Maybe also worth adding a comment that we expect this to be the same since DMS and MDAnalysis use the same units?
I don't unfortunately, I'm not even sure the current DMS reference file has dimensions, so it's likely the current file isn't testing anything important. |
Summary
Fixes : #3304
This PR adds a small test for
DMSReaderthat exercises the unit-conversion code path whenconvert_units=True.Details
convert_pos_from_nativeby triggering position accessTest verified locally with:
Notes
ts.dimensionsis intentionallyNonefor DMS files, so this test does not attempt to assert or modify box dimensions.📚 Documentation preview 📚: https://mdanalysis--5180.org.readthedocs.build/en/5180/