Skip to content
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

WIP Issue 2043 deprecate ts write #2110

Merged
merged 22 commits into from
Jun 8, 2020
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
5c6f4b3
deprecate Timestep argument to Writers (issue #2043)
richardjgowers Oct 16, 2018
926212e
made AtomGroup/Universe default argument to write_next_timestep
richardjgowers Oct 17, 2018
2e7e164
pushed chemfiles into line
richardjgowers Jun 5, 2020
937c215
make NAMDBINWriter use write_next_timestep method
richardjgowers Jun 5, 2020
5590b9a
updated deprecation warnings for Writer.write(timestep)
richardjgowers Jun 5, 2020
7173865
fix up chemfiles test
richardjgowers Jun 6, 2020
0377dfc
fixup FHAIMS to raise deprecation warning on timestep input
richardjgowers Jun 6, 2020
cf4e3a8
fixed chemfiles test with old version of chemfiles
richardjgowers Jun 6, 2020
d19fbd8
changed versionchanged to deprecated
richardjgowers Jun 7, 2020
2b034a4
deprecation docstring changes
IAlibay Jun 7, 2020
fac4510
fixed up TRZ to accept non Timestep arguments
richardjgowers Jun 7, 2020
cbc8a0a
deprecated write_next_timestep
richardjgowers Jun 7, 2020
8f1be73
removed testsuit usage of write_next_timestep
richardjgowers Jun 7, 2020
2d1f27b
i'm such a genius
richardjgowers Jun 7, 2020
15e5374
Merge branch 'develop' into issue-2043-deprecate_ts_write
IAlibay Jun 7, 2020
b39aa24
fixed up test_chemfiles
richardjgowers Jun 8, 2020
288b2ec
added test for DATAWriter with Universe input
richardjgowers Jun 8, 2020
3a96141
Update test_lammps.py
richardjgowers Jun 8, 2020
0d35fc6
Fixes warning typo and docs (including dev docs)
IAlibay Jun 8, 2020
cd0c209
Apply suggestions from code review
richardjgowers Jun 8, 2020
205c402
Apply suggestions from code review
richardjgowers Jun 8, 2020
e3ffe4c
Update package/MDAnalysis/coordinates/chemfiles.py
richardjgowers Jun 8, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion package/CHANGELOG
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,8 @@ Deprecations
* analysis.density.density_from_Universe() (remove in 2.0)
* analysis.density.notwithin_coordinates_factory() (remove in 2.0)
* analysis.density.density_from_PDB and BfactorDensityCreator (remove in 2.0)

* Writer.write_next_timestep is deprecated, use write() instead (remove in 2.0)
* Writer.write(Timestep) is deprecated, use either a Universe or AtomGroup

09/05/19 IAlibay, richardjgowers

Expand Down
21 changes: 19 additions & 2 deletions package/MDAnalysis/coordinates/DCD.py
Original file line number Diff line number Diff line change
Expand Up @@ -390,17 +390,34 @@ def __init__(self,
is_periodic=1,
istart=istart)

def write_next_timestep(self, ts):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are the low-level functions in lib.formats good for that?

Copy link
Member

@kain88-de kain88-de Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Timestep in 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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

def _write_next_frame(self, ag):
"""Write timestep object into trajectory.
richardjgowers marked this conversation as resolved.
Show resolved Hide resolved

Parameters
----------
ts: TimeStep
ag : AtomGroup or Universe

See Also
--------
:meth:`DCDWriter.write` takes a more general input


.. deprecated:: 1.0.0
Deprecated use of Timestep as argument. To be removed in version 2.0
.. versionchanged:: 1.0.0
Added ability to pass AtomGroup or Universe.
IAlibay marked this conversation as resolved.
Show resolved Hide resolved
"""
if isinstance(ag, base.Timestep):
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")
xyz = ts.positions.copy()
dimensions = ts.dimensions.copy()

Expand Down
9 changes: 3 additions & 6 deletions package/MDAnalysis/coordinates/FHIAIMS.py
Original file line number Diff line number Diff line change
Expand Up @@ -267,22 +267,19 @@ def __init__(self, filename, convert_units=True, n_atoms=None, **kwargs):
self.filename = util.filename(filename, ext='.in', keep=True)
self.n_atoms = n_atoms

def write(self, obj):
def _write_next_frame(self, obj):
"""Write selection at current trajectory frame to file.

Parameters
-----------
obj : AtomGroup or Universe or :class:`Timestep`

obj : AtomGroup or Universe
"""
# write() method that complies with the Trajectory API

# TODO 2.0: Remove timestep logic
try:

# make sure to use atoms (Issue 46)
ag_or_ts = obj.atoms
# can write from selection == Universe (Issue 49)

except AttributeError:
IAlibay marked this conversation as resolved.
Show resolved Hide resolved
if isinstance(obj, base.Timestep):
ag_or_ts = obj.copy()
Expand Down
10 changes: 9 additions & 1 deletion package/MDAnalysis/coordinates/GRO.py
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ def write(self, obj):

Parameters
-----------
obj : AtomGroup or Universe or :class:`Timestep`
obj : AtomGroup or Universe

Note
----
Expand All @@ -357,6 +357,9 @@ def write(self, obj):
*resName* and *atomName* are truncated to a maximum of 5 characters
.. versionchanged:: 0.16.0
`frame` kwarg has been removed
.. deprecated:: 1.0.0
Deprecated calling with Timestep, use AtomGroup or Universe.
To be removed in version 2.0.
"""
# write() method that complies with the Trajectory API

Expand All @@ -368,6 +371,11 @@ def write(self, obj):

except AttributeError:
if isinstance(obj, base.Timestep):
warnings.warn(
'Passing a Timestep to write is deprecated, '
richardjgowers marked this conversation as resolved.
Show resolved Hide resolved
'and will be removed in 2.0; '
'use either an AtomGroup or Universe',
DeprecationWarning)
ag_or_ts = obj.copy()
else:
raise_from(TypeError("No Timestep found in obj argument"), None)
Expand Down
11 changes: 1 addition & 10 deletions package/MDAnalysis/coordinates/MOL2.py
Original file line number Diff line number Diff line change
Expand Up @@ -373,16 +373,7 @@ def encode_block(self, obj):
molecule[1] = molecule_1_store
return return_val

def write(self, obj):
"""Write a new frame to the MOL2 file.

Parameters
----------
obj : AtomGroup or Universe
"""
self.write_next_timestep(obj)

def write_next_timestep(self, obj):
def _write_next_frame(self, obj):
"""Write a new frame to the MOL2 file.

Parameters
IAlibay marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
10 changes: 7 additions & 3 deletions package/MDAnalysis/coordinates/NAMDBIN.py
Original file line number Diff line number Diff line change
Expand Up @@ -113,15 +113,19 @@ def __init__(self, filename, n_atoms=None, **kwargs):
"""
self.filename = util.filename(filename)

def write(self, obj):
def _write_next_frame(self, obj):
"""Write obj at current trajectory frame to file.
richardjgowers marked this conversation as resolved.
Show resolved Hide resolved

Parameters
----------
obj : :class:`~MDAnalysis.core.groups.AtomGroup` or :class:`~MDAnalysis.core.universe.Universe` or a :class:`Timestep`
obj : :class:`~MDAnalysis.core.groups.AtomGroup` or :class:`~MDAnalysis.core.universe.Universe`
write coordinate information associate with `obj`
richardjgowers marked this conversation as resolved.
Show resolved Hide resolved
"""

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


.. deprecated:: 1.0.0
Passing a Timestep is deprecated for removal in version 2.0
"""
# TODO 2.0: Remove Timestep logic
if isinstance(obj, base.Timestep):
IAlibay marked this conversation as resolved.
Show resolved Hide resolved
n_atoms = obj.n_atoms
coor = obj.positions.reshape(n_atoms*3)
Expand Down
8 changes: 4 additions & 4 deletions package/MDAnalysis/coordinates/PDB.py
Original file line number Diff line number Diff line change
Expand Up @@ -825,7 +825,7 @@ def _update_frame(self, obj):
* :attr:`PDBWriter.timestep` (the underlying trajectory
:class:`~MDAnalysis.coordinates.base.Timestep`)

Before calling :meth:`write_next_timestep` this method **must** be
Before calling :meth:`_write_next_frame` this method **must** be
called at least once to enable extracting topology information from the
current frame.
"""
Expand Down Expand Up @@ -864,7 +864,7 @@ def write(self, obj):
# Issue 105: with write() ONLY write a single frame; use
# write_all_timesteps() to dump everything in one go, or do the
# traditional loop over frames
self.write_next_timestep(self.ts, multiframe=self._multiframe)
self._write_next_frame(self.ts, multiframe=self._multiframe)
self._write_pdb_bonds()
# END record is written when file is being close()d

Expand Down Expand Up @@ -907,15 +907,15 @@ def write_all_timesteps(self, obj):

for framenumber in range(start, len(traj), step):
traj[framenumber]
self.write_next_timestep(self.ts, multiframe=True)
self._write_next_frame(self.ts, multiframe=True)

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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the only _write_next_frame that takes a Timestep?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so yes, it's because PDB also defines its own write() method

'''write a new timestep to the PDB file

:Keywords:
Expand Down
39 changes: 23 additions & 16 deletions package/MDAnalysis/coordinates/TRJ.py
Original file line number Diff line number Diff line change
Expand Up @@ -869,7 +869,6 @@ def __init__(self,
self.dt = dt
self.remarks = remarks or "AMBER NetCDF format (MDAnalysis.coordinates.trj.NCDFWriter)"

self.ts = None # when/why would this be assigned??
self._first_frame = True # signals to open trajectory
self.trjfile = None # open on first write with _init_netcdf()
self.periodic = None # detect on first write
Expand Down Expand Up @@ -972,39 +971,47 @@ def _init_netcdf(self, periodic=True):
self._first_frame = False
self.trjfile = ncfile

def is_periodic(self, ts=None):
def is_periodic(self, ts):
"""Test if `Timestep` contains a periodic trajectory.
richardjgowers marked this conversation as resolved.
Show resolved Hide resolved

Parameters
----------
ts : :class:`Timestep`
:class:`Timestep` instance containing coordinates to
be written to trajectory file; default is the current
timestep
be written to trajectory file

Returns
-------
bool
Return ``True`` if `ts` contains a valid simulation box
"""
ts = ts if ts is not None else self.ts
return np.all(ts.dimensions > 0)

def write_next_timestep(self, ts=None):
def _write_next_frame(self, ag):
"""write a new timestep to the trj file
richardjgowers marked this conversation as resolved.
Show resolved Hide resolved

Parameters
----------
ts : :class:`Timestep`
:class:`Timestep` instance containing coordinates to
be written to trajectory file; default is the current
timestep
ag : AtomGroup or Universe


.. deprecated:: 1.0.0
Deprecated using Timestep. To be removed in version 2.0.
.. versionchanged:: 1.0.0
IAlibay marked this conversation as resolved.
Show resolved Hide resolved
Added ability to use either AtomGroup or Universe.
"""
if ts is None:
ts = self.ts
if ts is None:
raise IOError(
"NCDFWriter: no coordinate data to write to trajectory file")
if isinstance(ag, base.Timestep):
ts = ag
else:
try:
# Atomgroup?
ts = ag.ts
except AttributeError:
try:
# Universe?
ts = ag.trajectory.ts
except AttributeError:
raise TypeError("No Timestep found in ag argument")

if ts.n_atoms != self.n_atoms:
raise IOError(
Expand All @@ -1020,7 +1027,7 @@ def _write_next_timestep(self, ts):
"""Write coordinates and unitcell information to NCDF file.

Do not call this method directly; instead use
:meth:`write_next_timestep` because some essential setup is done
:meth:`write` because some essential setup is done
there before writing the first frame.

Based on Joshua Adelman's `netcdf4storage.py`_ in `Issue 109`_.
Expand Down
24 changes: 22 additions & 2 deletions package/MDAnalysis/coordinates/TRR.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@
"""
from __future__ import absolute_import

import warnings

richardjgowers marked this conversation as resolved.
Show resolved Hide resolved
from . import base
from .XDR import XDRBaseReader, XDRBaseWriter
from ..lib.formats.libmdaxdr import TRRFile
from ..lib.mdamath import triclinic_vectors, triclinic_box
Expand All @@ -58,18 +61,35 @@ class TRRWriter(XDRBaseWriter):
'force': 'kJ/(mol*nm)'}
_file = TRRFile

def write_next_timestep(self, ts):
def _write_next_frame(self, ag):
"""Write timestep object into trajectory.
richardjgowers marked this conversation as resolved.
Show resolved Hide resolved

Parameters
----------
ts : :class:`~base.Timestep`
ag : AtomGroup or Universe

See Also
--------
<FormatWriter>.write(AtomGroup/Universe/TimeStep)
The normal write() method takes a more general input


.. deprecated:: 1.0.0
Deprecated the use of Timestep as arguments to write. Use either
an AtomGroup or Universe. To be removed in version 2.0.
"""
if isinstance(ag, base.Timestep):
ts = ag
else:
try:
ts = ag.ts
except AttributeError:
try:
# special case: can supply a Universe, too...
ts = ag.trajectory.ts
except AttributeError:
raise TypeError("No Timestep found in ag argument")

xyz = None
if ts.has_positions:
xyz = ts.positions.copy()
Expand Down
16 changes: 13 additions & 3 deletions package/MDAnalysis/coordinates/TRZ.py
Original file line number Diff line number Diff line change
Expand Up @@ -530,10 +530,20 @@ def _writeheader(self, title):
out['nrec'] = 10
out.tofile(self.trzfile)

def write_next_timestep(self, ts):
def _write_next_frame(self, obj):
# Check size of ts is same as initial
if not ts.n_atoms == self.n_atoms:
raise ValueError("Number of atoms in ts different to initialisation")
# TODO: Remove Timestep logic in 2.0
IAlibay marked this conversation as resolved.
Show resolved Hide resolved
if isinstance(obj, base.Timestep):
ts = obj
if not ts.n_atoms == self.n_atoms:
raise ValueError("Number of atoms in ts different to initialisation")
else:
try: # atomgroup?
ts = obj.ts
except AttributeError: # universe?
ts = obj.trajectory.ts
if not obj.atoms.n_atoms == self.n_atoms:
raise ValueError("Number of atoms in ts different to initialisation")

# Gather data, faking it when unavailable
data = {}
Expand Down
2 changes: 1 addition & 1 deletion package/MDAnalysis/coordinates/XDR.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ class XDRBaseReader(base.ReaderBase):
"""Base class for libmdaxdr file formats xtc and trr

This class handles integration of XDR based formats into MDAnalysis. The
XTC and TRR classes only implement `write_next_timestep` and
XTC and TRR classes only implement `_write_next_frame` and
`_frame_to_ts`.

.. _offsets-label:
Expand Down
26 changes: 24 additions & 2 deletions package/MDAnalysis/coordinates/XTC.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@
"""
from __future__ import absolute_import

import warnings

richardjgowers marked this conversation as resolved.
Show resolved Hide resolved
from . import base
from .XDR import XDRBaseReader, XDRBaseWriter
from ..lib.formats.libmdaxdr import XTCFile
from ..lib.mdamath import triclinic_vectors, triclinic_box
Expand Down Expand Up @@ -70,18 +73,37 @@ def __init__(self, filename, n_atoms, convert_units=True,
**kwargs)
self.precision = precision

def write_next_timestep(self, ts):
def _write_next_frame(self, ag):
"""Write timestep object into trajectory.
richardjgowers marked this conversation as resolved.
Show resolved Hide resolved

Parameters
----------
ts : :class:`~base.Timestep`
ag : AtomGroup or Universe

See Also
--------
<FormatWriter>.write(AtomGroup/Universe/TimeStep)
richardjgowers marked this conversation as resolved.
Show resolved Hide resolved
The normal write() method takes a more general input


.. deprecated:: 1.0.0
Deprecated using Timestep. To be removed in version 2.0.
.. versionchanged:: 1.0.0
IAlibay marked this conversation as resolved.
Show resolved Hide resolved
Added ability to use either AtomGroup or Universe.
"""
if isinstance(ag, base.Timestep):
ts = ag
else:
try:
# Atomgroup?
ts = ag.ts
except AttributeError:
try:
# Universe?
ts = ag.trajectory.ts
except AttributeError:
raise TypeError("No Timestep found in ag argument")

xyz = ts.positions.copy()
time = ts.time
step = ts.frame
Expand Down
Loading