Skip to content

Conversation

@tanii1125
Copy link
Contributor

@tanii1125 tanii1125 commented Dec 18, 2025

Summary

Fixes : #3304
This PR adds a small test for DMSReader that exercises the unit-conversion code path when convert_units=True.

Details

  • Adds coverage for convert_pos_from_native by triggering position access
  • Does not alter reader behavior or logic
  • No changes to existing tests

Test verified locally with:

pytest testsuite/MDAnalysisTests/coordinates/test_dms.py::TestDMSReader::test_convert_pos_from_native

Notes

ts.dimensions is intentionally None for DMS files, so this test does not attempt to assert or modify box dimensions.


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

@codecov
Copy link

codecov bot commented Dec 18, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.72%. Comparing base (9fb8b0a) to head (7882b3b).
⚠️ Report is 9 commits behind head on develop.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@IAlibay IAlibay left a 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.

@tanii1125
Copy link
Contributor Author

Thanks for the detailed feedback
I’ll update the test to assert numerical correctness (pre- and post-conversion) for positions, velocities, and dimensions, rather than just triggering the code path.

@tanii1125 tanii1125 requested a review from IAlibay January 5, 2026 15:07
Copy link
Member

@RMeli RMeli left a 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,
Copy link
Member

@RMeli RMeli Jan 15, 2026

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?

@IAlibay
Copy link
Member

IAlibay commented Jan 15, 2026

Do you know of a file with velocities that we have easily available?

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.

@tanii1125 tanii1125 requested a review from RMeli January 17, 2026 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DMSReader: convert_pos_from_native coverage

3 participants