BUG: use base units for TPR x/v parsing#5374
BUG: use base units for TPR x/v parsing#5374tylerjereddy wants to merge 1 commit intoMDAnalysis:developfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5374 +/- ##
========================================
Coverage 93.85% 93.85%
========================================
Files 182 182
Lines 22505 22507 +2
Branches 3200 3200
========================================
+ Hits 21121 21123 +2
Misses 922 922
Partials 462 462 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
* Fixes MDAnalysisgh-5373. * TPR position/velocity parsing was accidentally using native instead of base units--regression testing and source code was adjusted accordingly.
tylerjereddy
force-pushed
the
treddy_issue_5373
branch
from
5dafb12 to
c620345
Compare
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Thanks so much for doing this @tylerjereddy ! Couple of things.
| self.ts._pos = np.asarray( | ||
| tpr_utils.ndo_rvec(data, th.natoms), dtype=np.float32 | ||
| ) | ||
| self.convert_pos_from_native(self.ts._pos) |
There was a problem hiding this comment.
I think you need to wrap it around self.convert_units like we do here https://github.com/MDAnalysis/mdanalysis/blob/develop/package%2FMDAnalysis%2Fcoordinates%2FGRO.py#L255
There was a problem hiding this comment.
I noticed that. However, its use does need motivating with a regression test that fails without it, which wasn't immediately obvious to me since we don't write TPR files.
There was a problem hiding this comment.
Also, GROWriter __init__ has:
convert_units : bool (optional)
units are converted to the MDAnalysis base format; [``True``]
but farther down it has:
if self.convert_units:
# Convert back to nm from Angstroms,
# Not inplace because AtomGroup is not a copy
positions = self.convert_pos_to_native(positions, inplace=False)so while the default behavior makes sense, the docs are garbled--basically wrong, right? Should it even be "allowed" to write with A units for GRO?
There was a problem hiding this comment.
Do we know if dimensions setting also needs fixing?
There was a problem hiding this comment.
On the latest version of this feature branch,
import MDAnalysis as mda
from MDAnalysisTests.datafiles import TPR, XTC
u = mda.Universe(TPR, XTC)
print(u.dimensions)
u = mda.Universe(TPR)
print(u.dimensions)
u = mda.Universe(TPR, TPR)
print(u.dimensions)produces:
[80.017006 80.017006 80.017006 60. 60. 90. ]
None
None
That looks like more like a feature request than the same category of bug, so scope might be argued. I'd probably suggest a separate issue for that one.
There was a problem hiding this comment.
That sounds good - I was mostly just looking at other single file readers and checking what they were doing. If we don't have to, that works.
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
I'll put the convert unit thing as a request.
3 participants
Fixes BUG: TPR coordinate/velocity parsing uses wrong units #5373.
TPR position/velocity parsing was accidentally using native instead of base units--regression testing and source code was adjusted accordingly.
LLM / AI generated code disclosure
No AI/LLMs used here.
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--5374.org.readthedocs.build/en/5374/