-
Notifications
You must be signed in to change notification settings - Fork 832
ENH: expose TPR box dimensions through timestep #5377
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
Changes from 2 commits
798bf7a
6405f33
4792dcd
cd6ca2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,6 +51,7 @@ | |
|
|
||
| """ | ||
|
|
||
| from ..lib.mdamath import triclinic_box | ||
| from . import base | ||
| from ..lib import util | ||
| from .timestep import Timestep | ||
|
|
@@ -110,7 +111,15 @@ def _read_first_frame(self): | |
|
|
||
| state_ngtc = th.ngtc # done init_state() in src/gmxlib/tpxio.c | ||
| if th.bBox: | ||
| 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I reviewed the discussion in #5374. The conversion * 10.0 from nm to Angstroms follows the standard MDAnalysis convention where dimensions are always in Angstroms. Happy to update if the team decides on a different approach in the new issue you mentioned. |
||
|
|
||
| ts.dimensions = triclinic_box( | ||
| box_angstrom[0], | ||
| box_angstrom[1], | ||
| box_angstrom[2], | ||
| ) | ||
|
|
||
| if state_ngtc > 0: | ||
| if th.fver < 69: # redundancy due to different versions | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -438,3 +438,23 @@ def test_resids(resid_from_one, resid_addition): | |
| resids, | ||
| err_msg="tpr_resid_from_one kwarg not switching resids", | ||
| ) | ||
|
|
||
|
|
||
| class TestTPRBoxVectors: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't this testing live in the coordinate namespace since the "coordinates-only Universe" is emphasized and the changes are made in |
||
| """Tests for TPRParser box vector support.""" | ||
|
|
||
| def test_tpr_only_dimensions_not_none(self): | ||
| import MDAnalysis as mda | ||
| from MDAnalysisTests.datafiles import TPR | ||
|
|
||
| u = mda.Universe(TPR) | ||
|
|
||
| assert u.dimensions is not None | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test case is not well thought out -- it serves no value whatsoever alongside the one below, since they use the same
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated — removed the redundant None and shape-only checks. Tests are now parametrized across multiple TPR files (TPR, TPR2024_4, TPR2016, TPR455Double) with actual dimension values verified via gmx dump / mda.Universe. |
||
|
|
||
| def test_tpr_only_dimensions_shape(self): | ||
| import MDAnalysis as mda | ||
| from MDAnalysisTests.datafiles import TPR | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed — removed the scoped imports from inside test functions. All imports are now at the top of the file. |
||
|
|
||
| u = mda.Universe(TPR) | ||
|
|
||
| assert u.dimensions.shape == (6,) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We expect testing to be a bit more thorough than this:
|
||
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.
what is PR 27?