WIP Issue 2043 deprecate ts write #2110
Conversation
|
Turns out a fair few Writers didn't even accept a Timestep, so this helps tighten up the API too #206 I want to go through and get rid of the old usage of |
|
Oh joy, parallel testing is broken. |
Codecov Report
@@ Coverage Diff @@
## develop #2110 +/- ##
===========================================
+ Coverage 91.23% 91.28% +0.05%
===========================================
Files 176 176
Lines 24084 24555 +471
Branches 3156 3160 +4
===========================================
+ Hits 21972 22416 +444
- Misses 1489 1516 +27
Partials 623 623
Continue to review full report at Codecov.
|
| @@ -410,17 +410,36 @@ def __init__(self, | |||
| is_periodic=1, | |||
| istart=istart) | |||
|
|
|||
| def write_next_timestep(self, ts): | |||
There was a problem hiding this comment.
I feel that it might be useful to leave one entry point open for handing over custom-timesteps. Perhaps not deprecate here?
Or are there better alternative ways to do non-standard things (such as writing a "trajectory" of principal components... someone did this a while ago), e.g., using Universe.empty() and friends?
There was a problem hiding this comment.
are the low-level functions in lib.formats good for that?
There was a problem hiding this comment.
why is this changed in the first place? This is an internal API method to me. The write method in our base class used to handle this just fine. It also allowed us to have the code for unpacking the atomgroup into a timestep just once. I would rather vote for making this a private method and leaving childclasses as they are otherwise.
There was a problem hiding this comment.
The problem was Writer.write(obj) then calls Writer.write_next_timestep(ts) (which is what is implemented in child classes). But really the argument for W.write_next_timestep() should still be an AtomGroup (else you end up with car crash code like this)
So basically more of #206 where it's not really clear what a Writer is meant to implement
I guess we could make it so that:
- Child classes implement
write_next_timestep()which must accept AG/Universe - Child classes can optionally accept a
Timestepin the above method?
Currently the (very loose) default is that Timestep is accepted, and AG is optional. (But there are a few dissenters to this)
There was a problem hiding this comment.
I accept consistency as reasoning. I would still like that this code of unpacking an atomgroup exists only once as a helper function. We can properly test it.
orbeckst
force-pushed
the
issue-2043-deprecate_ts_write
branch
from
a3beb57 to
01016df
Compare
|
I squshed and rebased against develope and force pushed. |
|
I am not sure where Travis CI went – any ideas? I cannot see it as one of the running checks. |
|
@kain88-de said in #2110 (comment)
That would be good. Is there any way that the copy&paste code if isinstance(ag, base.Timestep):
warnings.warn(
'Passing a Timestep to write is deprecated, '
'use either an AtomGroup or Universe',
DeprecationWarning)
ts = ag
else:
try:
ts = ag.ts
except AttributeError:
try:
# Universe?
ts = ag.trajectory.ts
except AttributeError:
raise TypeError("No Timestep found in ag argument")could be a private method in the baseclass or something like that? |
|
Merged develop to get the travis fix from PR #2234. |
|
@richardjgowers is this still WIP? Don't we need the removal of |
|
@orbeckst Yeah I guess the deprecation will get pushed back to 2.0 or something. This isn't super critical, it's just a tightening of APIs |
richardjgowers
force-pushed
the
issue-2043-deprecate_ts_write
branch
from
8841106 to
46c15af
Compare
|
Ok I've raised this from the dead, hopefully the last blocker to 1.0 @MDAnalysis/coredevs |
There was a problem hiding this comment.
Tests are failing with some chemfiles related issues.
Can you briefly remind me why you renamed some write() methods to write_next_timestep()? Just asking because the name "write_next_timestep" seems antithetical to the intent. Is this just a semantic mishap on our part?
| write coordinate information associate with `obj` | ||
| """ | ||
|
|
There was a problem hiding this comment.
need a deprecation warning here when using a ts?
write_next_timestep() is this the name we're using even though we don't really want to use timestep?
|
@orbeckst I guess really we shouldn't have two entrypoints, so maybe |
- identified which Writers can currently use a ts (lots don't anyway) - deprecated Timestep as argument to Writer.write - Update package/MDAnalysis/coordinates/base.py - Update package/MDAnalysis/coordinates/XYZ.py - Update package/MDAnalysis/coordinates/GRO.py - add test_writer_api.py - removed obsolete test
removed chemfiles.Writer.write (to use base API method) and put logic into write_next_timestep
now started in 1.0 and targets v2.0
CI seems to get 0.7.4 for some builds
richardjgowers
force-pushed
the
issue-2043-deprecate_ts_write
branch
from
b9a3633 to
cf4e3a8
Compare
There was a problem hiding this comment.
Overall lgtm, might want to switch to .. deprecated instead of .. versionchanged in some places.
Just to check am I correct in assuming that the logic here is that we only have deprecation warnings for all the writers that existed pre-1.0 (i.e. why FHIAIMS and Chemfiles don't raise a warning)?
It looks like FHIAIMS and Chemfiles still accept ts, so we'll probably want at the very least some kind of comment somewhere that reminds us to clean up the logic some v2.0?
added TODO 2.0 for removing timestep logic
There was a problem hiding this comment.
lgtm, just two small changes, in the interest of getting 1.0 out, I can push them through if you want @richardjgowers
added tests for each Writer to make them accept AtomGroup or Universe
There was a problem hiding this comment.
So testing the warnings, we currently get the following:
GRO, TRZ, NAMDBIN, and XYZ give 1 deprecation warning, everything else give two.
GRO is a write method so that's expected, but the rest are write_next_timestep. We probably want to have the same behaviour across all ts-supporting writers. My opinion here is that the easiest thing would be two put warnings in all the write_next_timestep methods that support ts. It means an extra warning for users, but it's probably not too intrusive.
use write() instead renamed many functions to _write_next_frame for the internal workings of how to write a frame
|
|
||
| self._write_pdb_bonds() | ||
| self.close() | ||
|
|
||
| # Set the trajectory to the starting position | ||
| traj[start] | ||
|
|
||
| def write_next_timestep(self, ts=None, **kwargs): | ||
| def _write_next_frame(self, ts=None, **kwargs): |
There was a problem hiding this comment.
Is this the only _write_next_frame that takes a Timestep?
There was a problem hiding this comment.
I think so yes, it's because PDB also defines its own write() method
|
As discussed privately, I've added some "dev docs" for the |
There was a problem hiding this comment.
The code looks good to me; just a few doc suggestions. Thanks @richardjgowers and @IAlibay!
Co-authored-by: Lily Wang <31115101+lilyminium@users.noreply.github.com>
Co-authored-by: Lily Wang <31115101+lilyminium@users.noreply.github.com>
Co-authored-by: Lily Wang <31115101+lilyminium@users.noreply.github.com>
richardjgowers
dismissed stale reviews from lilyminium and orbeckst
changes added
Starts #2043
Changes made in this Pull Request:
Writer.write(Timestep)is deprecatedPR Checklist