Added dt support for xtc and trr trajectories.
#4908
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on GitHub Discussions so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS as part of this PR.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4908 +/- ##
===========================================
- Coverage 93.42% 93.41% -0.01%
===========================================
Files 177 189 +12
Lines 21859 22942 +1083
Branches 3078 3085 +7
===========================================
+ Hits 20422 21432 +1010
- Misses 986 1059 +73
Partials 451 451 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -148,6 +149,8 @@ def __init__( | |||
| itself is that of the sub system. | |||
| refresh_offsets : bool (optional) | |||
| force refresh of offsets | |||
| dt : float (optional) | |||
| timestep in MDAnalysis units to load trajectory with | |||
There was a problem hiding this comment.
Clearly document what happens here.
State that None reads the time as stored in the trajectory. Choosing dt overrides the time that is stored in the trajectory and assigns frame * dt as time.
| elif self._dt == ts.dt: | ||
| time = ts.time |
There was a problem hiding this comment.
Delete this case: this leads to weird behavior when the initial frame is at 0 but the time is not. In this case, setting dt will behave differently from what the user would expect, namely that time = frame * dt. Make sure that the code exactly implements what you document.
There was a problem hiding this comment.
My thinking here, was that if dt is set, and it is exactly equal to one of the trajectory, I don't want to touch the time in the frame.
There was a problem hiding this comment.
I understand but that's not how it's documented. If the user does not want to change anything then they should simply not set dt. Less magic is better.
| @@ -327,13 +335,16 @@ def __init__(self, filename, n_atoms, convert_units=True, **kwargs): | |||
| number of atoms to be written | |||
| convert_units : bool (optional) | |||
| convert from MDAnalysis units to format specific units | |||
| dt : float (optional) | |||
| timestep in MDAnalysis units to write trajectory with | |||
There was a problem hiding this comment.
document exactly what dt does and what the default means
| elif self._dt == ts.dt: | ||
| time = ts.time |
There was a problem hiding this comment.
as in TRR – remove this case because it leads to ambiguous behavior
| time = ts.time | ||
| else: | ||
| time = self._dt * ts.frame | ||
| step = ts.data.get('step', ts.frame) |
| @@ -64,10 +64,12 @@ def __init__(self, filename, n_atoms, convert_units=True, | |||
| number of atoms to write | |||
| convert_units : bool (optional) | |||
| convert into MDAnalysis units | |||
| dt : float (optional) | |||
| timestep in MDAnalysis units to load trajectory with | |||
There was a problem hiding this comment.
document exact behavior, emphasize that None is the default that reads time from trajectory and that dt as a float is for special cases when one wants to set a different time step from what is stored
| precision : float (optional) | ||
| set precision of saved trjactory to this number of decimal places. | ||
| """ | ||
| super(XTCWriter, self).__init__(filename, n_atoms, convert_units, | ||
| super(XTCWriter, self).__init__(filename, n_atoms, convert_units, dt, |
There was a problem hiding this comment.
use kwargs for the super call (sorry that convert_units was not done)
| super(XTCWriter, self).__init__(filename, n_atoms, convert_units, dt, | |
| super(XTCWriter, self).__init__(filename, n_atoms, convert_units=convert_units, dt=dt, |
(you probably need to line-break ... actually, just run through black)
| @@ -164,7 +171,11 @@ def _read_next_timestep(self, ts=None): | |||
| def _frame_to_ts(self, frame, ts): | |||
| """convert a xtc-frame to a mda TimeStep""" | |||
| ts.frame = self._frame | |||
| ts.time = frame.time | |||
| dt = self._kwargs["dt"] | |||
| if dt is not None: | |||
There was a problem hiding this comment.
Move the default behavior first in if block
| def test_Writer_with_dt_setting(self, tmpdir): | ||
| universe = mda.Universe(GRO, self.filename, convert_units=True) | ||
| ext = os.path.splitext(self.filename)[1] | ||
| outfile = str(tmpdir.join("/xdr-reader-test" + ext)) |
There was a problem hiding this comment.
Do you have to convert to string? Is Writer not able to use the Path instance?
There was a problem hiding this comment.
(or is tmpdir something else .... some py.path?)
There was a problem hiding this comment.
(And yes .... you took the old code, it's just possible that this code is so old that it predates the ability of readers/writers to work with Path objects.)
| @@ -245,6 +278,31 @@ def test_Writer(self, tmpdir): | |||
| u.atoms.positions, universe.atoms.positions, self.prec | |||
| ) | |||
|
|
|||
| def test_Writer_with_dt_setting(self, tmpdir): | |||
| universe = mda.Universe(GRO, self.filename, convert_units=True) | |||
There was a problem hiding this comment.
I don't like replicating the code of test_Writer, less copy&paste makes for more maintainable code. Can you please refactor so that the two methods share most of their code? Perhaps there's even a way to make it a parametrized test.
orbeckst
changed the title
xtc and trr trajectories.xtc and trr trajectories.
Fixes #4905
Changes made in this Pull Request:
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--4908.org.readthedocs.build/en/4908/