MDAnalysis / mdanalysis

MDAnalysis is a Python library to analyze molecular dynamics simulations.
https://mdanalysis.org
Other
1.32k stars 652 forks source link

Writers should use a more general `atoms` argument instead of `n_atoms` #517

Open kain88-de opened 9 years ago

kain88-de commented 9 years ago

In #511 i noticed that a more natural way of using the writers would be to have an atoms arguments which can be of type (Universe, AtomGroup, int). Allowing for usages like this

universe = mda.Universe(PDB, RAW_XTC)
w = mda.Writer(fname, universe)
w = mda.Writer(fname, universe.select_atoms('protein')

Implementing this is easy for each writer class.

if isinstance(atoms, (Universe, AtomGroup):
    n_atoms = len(atoms)
elif isinstance(atoms, int):
    n_atoms = atoms
else:
    raise ValueError('Can\'t build Writer from atoms ({})'.format(atoms))

But since this is an API change it is better if we discuss is first. And I think we should wait with it until #516 is completed, then we can make the change on all writers and the same time.

richardjgowers commented 9 years ago

Just fyi, Universe doesn't currently have a len (it would be unclear if it meant len(trajectory) or len(atoms))

I also don't like putting type checks on things like this. Ideally it should work with any object that has all the required attributes, AtomGroup or not. Duck typing etc etc

orbeckst commented 9 years ago

Using atoms=atomgroup saves a wee bit of typing (instead of n_atoms=len(ag)) and is conceptually nice. So yes, why not? (We'll just have to keep n_atoms around for a while in parallel until we get to the next API break...)

I think in this instance one cannot get away with simple duck typing (i.e. you can't just ask for len(obj)). However, len(obj.atoms) should always give you the right answer (unless it's an int).

hainm commented 8 years ago

it seems that mda dev prefer to use OO. I myself prefer to use verb to perform action.

In pytraj

import pytraj as pt
traj = pt.load('traj.nc', 'prmtop')
pt.save('new_name.dcd', traj)
# with mask
pt.save('new_name.dcd', traj(mask='@CA'))

(disclaimer: my first language is PASCAL :D).

dotsdl commented 8 years ago

it seems that mda dev prefer to use OO.

@hainm Yes. :)

orbeckst commented 8 years ago

@kain88-de I'm in favor for having the proposed functionality. Let's come to a conclusion here to either implement in the future or close the issue.

kain88-de commented 8 years ago

OK then we can go for this in the future.

kash1102 commented 7 years ago

hello all, I am interested in solving this issue as much as i understood we need to take atoms as argument in coordinates/core.py int function writer instead of n_atoms and this atom argument can be of 3 type 1. atomgroup 2. universe 3. int. and in writer class we have to extract n_atoms by code given by @kain88-de or slight modfication of code. Thanks any inputs on this will be appreciated :)

kain88-de commented 7 years ago

First we need to make sure that all writers use the same API. This means they all should use the same base tests see, #516.