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

frame Writers' write() method does not accept a Timestep #49

Closed
GoogleCodeExporter opened this Issue Apr 4, 2015 · 7 comments

Comments

Projects
None yet
1 participant
@GoogleCodeExporter

GoogleCodeExporter commented Apr 4, 2015

What steps will reproduce the problem?

u = Universe(PSF,DCD)
W = Writer('foo.pdb')
W.write(u.trajectory.ts)


What is the expected output? What do you see instead?

The Trajectory API Specification currently says that this should work but 
instead I get

--> 343         u = selection.universe
    344         if frame is not None:
    345             u.trajectory[frame]  # advance to frame

AttributeError: 'Universe' object has no attribute 'universe'

The way it is implemented at the moment it cannot work because FrameWriter also 
need atom type information that is not in the Timestep but only accessible via 
a universe.

Either change API definition before 0.7.0 is released or somehow hack around 
– maybe add Universe to Timestep??



Original issue reported on code.google.com by orbeckst on 25 Oct 2010 at 12:15

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 4, 2015

Those steps don't produce the AttributeError you have quoted. I get something 
that is more consistent with feeding in a Timestep object (below). I can 
reproduce your error by feeding in a raw trajectory (i.e., just the universe 
object without timestep specification), and that would make more sense for a 
'Universe' object-related complaint in the stack trace.

AttributeError: 'Timestep' object has no attribute 'universe'

Looking at the method for PDB writing it's clear that if you feed in a single 
argument the assumption is that this is an AtomGroup and nothing else:

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

        write(selection,frame=FRAME)

        selection         MDAnalysis AtomGroup
        frame             optionally move to frame FRAME
        """

So, you want the user to be able to feed in a trajectory time step or even just 
a raw universe object? There would certainly be ugly ways to parse out the 
universe object from a timestep specification and physically select all atoms 
in the code (presumably the user wants all atoms if they aren't specifying) so 
that the function effectively gets an atom selection. Then you could feed the 
frame number of the timestep through as if the user had simply provided the 
frame=number argument.

I was about to test this when I realized that code indentation in MDAnalysis 
source is still a problem when using gedit as a text editor.

Original comment by tyler.je.reddy@gmail.com on 26 Oct 2010 at 10:15

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 4, 2015

Sorry, apparently I pasted the wrong output. Now again:

import MDAnalysis; from MDAnalysis.tests.datafiles import *; 
u = MDAnalysis.Universe(PSF,DCD)
W = MDAnalysis.Writer('foo.pdb')

W.write(u.trajectory.ts)
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)

/Users/oliver/tmp/<ipython console> in <module>()

/Users/oliver/.local/lib/python2.6/site-packages/MDAnalysis-0.7.0_rc1-py2.6-maco
sx-10.6-universal.egg/MDAnalysis/coordinates/PDB.pyc in write(self, selection, 
frame)
    341         frame             optionally move to frame FRAME
    342         """
--> 343         u = selection.universe
    344         if frame is not None:
    345             u.trajectory[frame]  # advance to frame

AttributeError: 'Timestep' object has no attribute 'universe'

 W.write(u)
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)

/Users/oliver/tmp/<ipython console> in <module>()

/Users/oliver/.local/lib/python2.6/site-packages/MDAnalysis-0.7.0_rc1-py2.6-maco
sx-10.6-universal.egg/MDAnalysis/coordinates/PDB.pyc in write(self, selection, 
frame)
    341         frame             optionally move to frame FRAME
    342         """
--> 343         u = selection.universe
    344         if frame is not None:
    345             u.trajectory[frame]  # advance to frame

AttributeError: 'Universe' object has no attribute 'universe'


Original comment by orbeckst on 26 Oct 2010 at 12:23

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 4, 2015

* Accepting a Universe is simple: we just add a universe attribute to Universe 
that points to self (or we fake it with properties/fiddling with 
__getattribute__)

* Accepting a Timestep is harder because we do need extra information beyond 
the Timestep to write a PDB or GRO; it is a not a problem for trajectory 
writers like DCD or XTC which only care about how many coordinates there are.

We could add a universe attribute to Timestep as well but I am not 100% sure if 
I fully understand the implications of doing so (although it *should* be safe, 
given that one typically has a a one-to-one correspondence between a Universe 
and a Reader).

We could also require a Universe when setting up the Writer; that's a bit ugly 
because it would actually be necessary for "single frame" writers (GRO, PDB, 
...) but superfluous for DCD/XTC/...

What I want is that the user does not notice a difference when using

  Writer.write(...)

between writing a trajectory and writing a "single-frame" format.

Original comment by orbeckst on 26 Oct 2010 at 12:30

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 4, 2015

Suggestion (that does not solve the problem...):

For the future: We could change the trajectory writers so that they do not 
actually open the file during __init__() but do this in a lazy manner on the 
first call to write(). 

This will require some minor book-keeping behind the scenes but would have the 
great advantage that at the writing stage we know everything we need to know to 
set up the trajectory.

All we have to do is to delay writing header information and set a flag 
reminding us once we've done that.

This would definitely improve user experience and make scripts even simpler to 
write:

  W = Writer(filename)
  for ts in other.trajectory:
    W.write(selection)


Original comment by orbeckst on 27 Oct 2010 at 1:44

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 4, 2015

I think I'll close the issue with Won't Fix and change the API specs:

write() requires a Universe or a Selection

write_next_timestep() requires a Timestep


I don't think that there is another good way to solve this unless we require a 
Universe for setting up the Writer:

W = Writer(Universe, filename)

In this case we should probably add Writer as a method to Universe anyway:

W = universe.Writer(filename)

... which might not be such a bad solution --- comments?

Original comment by orbeckst on 27 Oct 2010 at 1:48

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 4, 2015

I intend to change the specs so that accepting a Timestep is optional for 
write(), at least for 0.7.0 and resolve this with "Won't fix".

Feel free to re-open the issue. 

Original comment by orbeckst on 30 Oct 2010 at 11:28

  • Changed state: WontFix
@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Apr 4, 2015

.. and made single frame writer work with universes in r528

Original comment by orbeckst on 31 Oct 2010 at 12:30

orbeckst referenced this issue Jun 22, 2015

Lightweight serialization of AtomGroups by reutilization of built Uni…
…verses

AtomGroups can now be pickled/unpickled (closes #293)
A weakref system was implemented so that Universes can have a __del__
method without memleaking (closes #297)
ChainReaders no longer leak (and also no longer have a __del__  method)
(closes #312)
Tests now specifically look for uncollectable objects at the end. Breaks
parallelization, though.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment