mdanalysis icon indicating copy to clipboard operation
mdanalysis copied to clipboard

use AtomGroup in place of selection strings

Open orbeckst opened this issue 6 years ago • 4 comments
trafficstars

Is your feature request related to a problem? Please describe.

Many analysis methods (Contacts, old and new H-bond analysis PR #2237, ...) require the user to provide selection strings and then internally call u.select_atoms(sel). Given the goal of MDAnalysis to enable object-oriented programming, it should be possible to provide AtomGroups directly instead of selections.

Describe the solution you'd like

As discussed by @richardjgowers @p-j-smith @bieniekmateusz in https://github.com/MDAnalysis/mdanalysis/pull/2237#discussion_r272833451

This could be neatly solved with Universe.selectAtoms(AG) returning AG. This way all interfaces would automatically accept str and AG selections.

Describe alternatives you've considered A clear and concise description of any alternative solutions or features you've considered.

  • We can leave everything as is, but this presents a bit of a hodge-podge of user interface choices (probably does not make @lilyminium happy, either, who would likely prefer to have a general way).
  • Some cases are difficult where selection strings are actually necessary because the selection has to be recreated at each step (waterdynamics is an example, H-bond analysis is also a common case). This could be solved with updating (dynamics) AtomGroups but the user needs to be aware.

orbeckst avatar Oct 22 '19 17:10 orbeckst

#2375 implemented the following, where type(ag) == AtomGroup:

  • Universe.select_atoms(ag) returns Universe.atoms.intersection(ag), i.e. copy of ag with any duplicates and ordering lost.
  • AtomGroup.select_atoms(ag) returns AtomGroup.intersection(ag).
  • Universe.select_atoms(ag, updating=True) returns UpdatingAtomGroup, but the actual behaviour over frames is to return the same static atoms in the AtomGroup. Again, I believe ordering/duplicates are lost.

This was closed due to being more complicated than anticipated. The last point in particular is quite misleading, and users could lose ordered AtomGroups easily. It was also most likely that I would have to look at every analysis class and change their selection behaviour. This issue is probably on hold until 2.0.

lilyminium avatar Oct 31 '19 01:10 lilyminium

To add more notes from the discussion: In 2.0 we could also simply abandon selection strings for analysis user interfaces and rewrite them to just use AtomGroups. That would create a cleaner API anyway.

orbeckst avatar Oct 31 '19 01:10 orbeckst

I am also going to revive the spectre of this for 3.0 ? I think it would be good to have a more consistent interface.

We could also consider using a decorator where one can provide a selstring OR atomgroup and they get converted to an atomgroup consistently. Then users can keep the flexibility of but analysis function only need to provide an interface for atomgroup. This is similar to what is done in check_coords now as of #3730 and related.

What do people think?

hmacdope avatar Sep 08 '22 23:09 hmacdope

There was a whole discussion of why using both is really not that straightforward. Details elude me, unfortunately.

However, only using AtomGroups would be feasible, I think, but a big jump.

orbeckst avatar Sep 09 '22 00:09 orbeckst