Skip to content

Add test to cover unit conversion path in DMSReader - #5180

Closed
tani-dubey wants to merge 5 commits into
MDAnalysis:developfrom
tani-dubey:test-DMSReader
Closed

Add test to cover unit conversion path in DMSReader#5180
tani-dubey wants to merge 5 commits into
MDAnalysis:developfrom
tani-dubey:test-DMSReader

Conversation

@tani-dubey

@tani-dubey tani-dubey commented Dec 18, 2025

Copy link
Copy Markdown
Contributor

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

codecov Bot commented Dec 18, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.72%. Comparing base (9fb8b0a) to head (7882b3b).
⚠️ Report is 60 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.

@IAlibay IAlibay left a comment

Copy link
Copy Markdown
Member

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.

@tani-dubey

Copy link
Copy Markdown
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.

@tani-dubey
tani-dubey requested a review from IAlibay January 5, 2026 15:07

@RMeli RMeli left a comment

Copy link
Copy Markdown
Member

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,

@RMeli RMeli Jan 15, 2026

Copy link
Copy Markdown
Member

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

IAlibay commented Jan 15, 2026

Copy link
Copy Markdown
Member

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.

@tani-dubey
tani-dubey requested a review from RMeli January 17, 2026 09:12
@orbeckst

orbeckst commented Feb 6, 2026

Copy link
Copy Markdown
Member

@tani-dubey do you have experience with desmond and can you create a suitable test file?

@tani-dubey

Copy link
Copy Markdown
Contributor Author

@tani-dubey do you have experience with desmond and can you create a suitable test file?

I don’t have prior hands-on experience with Desmond, and I don’t currently have an environment set up to generate DMS files. I’m happy to help investigate options, but creating a new, scientifically sound reference DMS file might be challenging without access to Desmond itself.

@IAlibay

IAlibay commented Feb 8, 2026

Copy link
Copy Markdown
Member

@tani-dubey I just don't see how this issue can be fixed without new data files. Given your response about the difficulty in generating new files, I will close this PR for now.

@IAlibay IAlibay closed this Feb 8, 2026
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

4 participants