ENH: expose TPR box dimensions through timestep#5377
ENH: expose TPR box dimensions through timestep#5377iamtanmaybaranwal wants to merge 2 commits intoMDAnalysis:developfrom
Conversation
When a Universe is created directly from a TPR file, u.dimensions previously returned None because box information was extracted but discarded during TPR reading. This change reads box vectors from the TPRReader, converts them from nanometres to Angstroms, converts triclinic vectors into MDAnalysis dimensions format, and stores them in ts.dimensions. Tests were added to verify dimensions are populated correctly. Closes MDAnalysis#5375
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5377 +/- ##
========================================
Coverage 93.85% 93.85%
========================================
Files 182 182
Lines 22505 22508 +3
Branches 3200 3200
========================================
+ Hits 21121 21124 +3
Misses 922 922
Partials 462 462 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tylerjereddy
left a comment
tylerjereddy
left a comment
There was a problem hiding this comment.
I added some initial comments. Some more info on the LLM/AI usage would be good, currently it just reads:
LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: yes
and my review comments suggest that things haven't been checked carefully by a human yet.
| Fixes | ||
| * TPRReader now exposes box dimensions when loading TPR files | ||
| as coordinates-only Universes, fixing missing `Universe.dimensions` | ||
| values. (Issue #5375, PR #27) |
| ) | ||
|
|
||
|
|
||
| class TestTPRBoxVectors: |
There was a problem hiding this comment.
shouldn't this testing live in the coordinate namespace since the "coordinates-only Universe" is emphasized and the changes are made in package/MDAnalysis/coordinates/TPR.py?
| tpr_utils.extract_box_info(data, th.fver) | ||
| box_info = tpr_utils.extract_box_info(data, th.fver) | ||
| # box vectors are stored in nm | ||
| box_angstrom = np.array(box_info.size) * 10.0 |
There was a problem hiding this comment.
the unit handling needs some discussion/consideration, per Irfan's comments at: #5374 (comment)
I think I may open yet another issue about this, because the matter is actually a bit complex/confusing at the moment
|
|
||
| u = mda.Universe(TPR) | ||
|
|
||
| assert u.dimensions is not None |
There was a problem hiding this comment.
This test case is not well thought out -- it serves no value whatsoever alongside the one below, since they use the same TPR and this just checks that the attribute is not None...
|
|
||
| def test_tpr_only_dimensions_shape(self): | ||
| import MDAnalysis as mda | ||
| from MDAnalysisTests.datafiles import TPR |
There was a problem hiding this comment.
This pattern of scoping imports inside functions like this is a hallmark of LLM/AI usage, and not really checking the work very carefully.
We do generally expect contributions to review work carefully, whether they use an LLM or not.
|
|
||
| u = mda.Universe(TPR) | ||
|
|
||
| assert u.dimensions.shape == (6,) |
There was a problem hiding this comment.
We expect testing to be a bit more thorough than this:
- check the actual values make sense--could easily break things while retaining the correct shape
- probably check some different TPR files/box types in a parametrized test
2 participants
When a Universe is created directly from a TPR file, u.dimensions previously returned None because box information was extracted but discarded during TPR reading.
This change reads box vectors from the TPRReader, converts them from nanometres to Angstroms, converts triclinic vectors into MDAnalysis dimensions format, and stores them in ts.dimensions.
Tests were added to verify dimensions are populated correctly.
Closes #5375
Fixes #5375
Changes made in this Pull Request:
TPRReader.ts.dimensions.LLM / AI generated code disclosure
LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: yes
PR Checklist
package/CHANGELOGfile updated?package/AUTHORS? (If it is not, add it!)Developers Certificate of Origin
I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.
📚 Documentation preview 📚: https://mdanalysis--5377.org.readthedocs.build/en/5377/