Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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 @@ -214,7 +214,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
24 changes: 21 additions & 3 deletions package/MDAnalysis/coordinates/DCD.py
Original file line number Diff line number Diff line change
Expand Up @@ -390,17 +390,35 @@ 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.

"""Write timestep object into trajectory.
def _write_next_frame(self, ag):
"""Write information associated with ``obj`` at current frame into trajectory

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.
Renamed from `write_next_timestep` to `_write_next_frame`.
"""
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:
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, '
'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: 3 additions & 8 deletions package/MDAnalysis/coordinates/MOL2.py
Original file line number Diff line number Diff line change
Expand Up @@ -373,21 +373,16 @@ def encode_block(self, obj):
molecule[1] = molecule_1_store
return return_val

def write(self, obj):
def _write_next_frame(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):
"""Write a new frame to the MOL2 file.

Parameters
----------
obj : AtomGroup or Universe
.. versionchanged:: 1.0.0
Renamed from `write_next_timestep` to `_write_next_frame`.
"""
block = self.encode_block(obj)
self.file.writelines(block)
18 changes: 13 additions & 5 deletions package/MDAnalysis/coordinates/NAMDBIN.py
Original file line number Diff line number Diff line change
Expand Up @@ -113,15 +113,23 @@ def __init__(self, filename, n_atoms=None, **kwargs):
"""
self.filename = util.filename(filename)

def write(self, obj):
"""Write obj at current trajectory frame to file.
def _write_next_frame(self, obj):
"""Write information associated with ``obj`` at current frame into trajectory


Parameters
----------
obj : :class:`~MDAnalysis.core.groups.AtomGroup` or :class:`~MDAnalysis.core.universe.Universe` or a :class:`Timestep`
write coordinate information associate with `obj`
"""
obj : :class:`~MDAnalysis.core.groups.AtomGroup` or :class:`~MDAnalysis.core.universe.Universe`
write coordinate information associated with `obj`


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?


.. versionchanged:: 1.0.0
Renamed from `write` to `_write_next_frame`.
.. 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):
n_atoms = obj.n_atoms
coor = obj.positions.reshape(n_atoms*3)
Expand Down
12 changes: 8 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 All @@ -931,6 +931,10 @@ def write_next_timestep(self, ts=None, **kwargs):
argument, :meth:`PDBWriter._update_frame` *must* be called
with the :class:`~MDAnalysis.core.groups.AtomGroup.Universe` as
its argument so that topology information can be gathered.


.. versionchanged:: 1.0.0
Renamed from `write_next_timestep` to `_write_next_frame`.
'''
if ts is None:
try:
Expand Down
44 changes: 26 additions & 18 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,48 @@ def _init_netcdf(self, periodic=True):
self._first_frame = False
self.trjfile = ncfile

def is_periodic(self, ts=None):
"""Test if `Timestep` contains a periodic trajectory.
def is_periodic(self, ts):
"""Test if timestep ``ts`` contains a periodic box.

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):
"""write a new timestep to the trj file
def _write_next_frame(self, ag):
"""Write information associated with ``ag`` at current frame into trajectory

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
Added ability to use either AtomGroup or Universe.
Renamed from `write_next_timestep` to `_write_next_frame`.
"""
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 +1028,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
26 changes: 23 additions & 3 deletions package/MDAnalysis/coordinates/TRR.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
"""
from __future__ import absolute_import

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 +59,37 @@ class TRRWriter(XDRBaseWriter):
'force': 'kJ/(mol*nm)'}
_file = TRRFile

def write_next_timestep(self, ts):
"""Write timestep object into trajectory.
def _write_next_frame(self, ag):
"""Write information associated with ``ag`` at current frame into trajectory

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


.. versionchanged:: 1.0.0
Renamed from `write_next_timestep` to `_write_next_frame`.
.. 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
26 changes: 23 additions & 3 deletions package/MDAnalysis/coordinates/TRZ.py
Original file line number Diff line number Diff line change
Expand Up @@ -530,10 +530,30 @@ def _writeheader(self, title):
out['nrec'] = 10
out.tofile(self.trzfile)

def write_next_timestep(self, ts):
def _write_next_frame(self, obj):
"""Write information associated with ``obj`` at current frame into trajectory

Parameters
----------
ag : AtomGroup or Universe


.. versionchanged:: 1.0.0
Renamed from `write_next_timestep` to `_write_next_frame`.
"""
# 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
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
Loading