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

Handling AtomGroup arguments in analysis functions/classes #1074

Open
kain88-de opened this issue Nov 14, 2016 · 5 comments
Open

Handling AtomGroup arguments in analysis functions/classes #1074

kain88-de opened this issue Nov 14, 2016 · 5 comments

Comments

@kain88-de
Copy link
Member

See PR #1073 Issues: #888, #719
Connected #1029

@orbeckst asked if a AtomGroup is a valid argument to have for an analysis. I would say yes and I write most of my own analysis in terms of aomgroups. But there is also old code and some newer that works with a universe and selection strings.

Some example classes
Accepting Universe: AlignTraj, Contacts (That one is a mixed bag with the reference though).
Accepting Atomgroup: InterRDF

I'm personally in favor of giving using an atomgroup as a parameter. I'm actually writing all my own analysis classes like this. This also makes it easy to use atomgroups of the same system from different simulations in another universe. It also feels like more natural way to use a python library like this. The universe and selection string version is more along using a CLI tool.

I've actually written a small argument parsing function in #888 that would be able to allow both approaches. See #888 but that is connected to #1029 with the new system.

The question is how should we handle atomgroup arguments in the analysis classes. Do we want to allow them or should it be a strict universe/selection-string option?

@orbeckst
Copy link
Member

Hbond analysis needs selection strings so that the selection can be recomputed for each frame.

I agree that using AtomGroups instead of selection strings feels more pythonic.

In principle the Universe can always be obtained from an AtomGroup so there is not really any need to provide a Universe except in order to make clear that an explicit selection is needed.

Not sure yet what the cleanest approach is or if it is even sensible to try a one size fits all approach.

@kain88-de
Copy link
Member Author

one size fits all approach

How about a one size fits 90% approach? That's what my argument parsing in #888 is trying to do. For the other 10% like Hbond analysis we can still use explicit selection strings as they are the only viable option and throw a sensible error message explaining that a string is needed for this analysis in contrast to the common case.

This goes well along with having a consistent API for analysis classes #719.

@richardjgowers
Copy link
Member

We also had some discussion in #175 about this, mostly about creating an automatically updating AtomSelection. I think the tl;dr of that idea is:

# needs some kwarg/different method to define that we don't want a regular AtomGroup
atom_sel = u.select_atoms('around 5.0 protein and resname SOL', dynamic_selection=True)

# find how many water atoms within 5.0 of protein 
for ts in u.trajectory:
    print len(atom_sel)

It's maybe a little complicated, and could just be replaced by calling select_atoms manually with a selection string.

We should probably just accept either strings or AtomGroup interchangeably.

@mnmelo
Copy link
Member

mnmelo commented Nov 15, 2016

Hey guys, I'm still working on this dynamic selection idea (it got held back because it depended heavily on the new topology system).

I should have it out soon.

mnmelo added a commit that referenced this issue Nov 21, 2016
Cleaned up some selection docs

Updated some uses of itertools.izip to six.zip

Cleaned up some variable names in groups.py that could eventually clash with
the six module.
mnmelo added a commit that referenced this issue Nov 23, 2016
Cleaned up some selection docs

Updated some uses of itertools.izip to six.zip

Cleaned up some variable names in groups.py that could eventually clash with
the six module.
mnmelo added a commit that referenced this issue Nov 30, 2016
Cleaned up some selection docs

Updated some uses of itertools.izip to six.zip

Cleaned up some variable names in groups.py that could eventually clash with
the six module.
mnmelo added a commit that referenced this issue Dec 13, 2016
Cleaned up some selection docs

Updated some uses of itertools.izip to six.zip

Cleaned up some variable names in groups.py that could eventually clash with
the six module.
mnmelo added a commit that referenced this issue Dec 28, 2016
* Added dynamically updating atomgroup selections (#175 and #1074)

Cleaned up some selection docs

Updated some uses of itertools.izip to six.zip

Cleaned up some variable names in groups.py that could eventually clash with
the six module.

Made AtomGroup __repr__ fancier by inflecting with the number of
elements.
@richardjgowers
Copy link
Member

So with UpdatingAtomGroups now a thing, is there anything a string argument can do that an AtomGroup (possibly updating) can't do? If not, is it a good idea to try and push towards going fully OO and recommend using AG as arguments rather than strings?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants