Residue & Segment slicing. Residue.select_atoms?

[richardjgowers]

I'm going over the old tests in test_atomgroup and found some differences in behaviour between the old and new systems. Because Residue and Segment used to be derived from AtomGroup, these could be sliced to give atoms.

u = mda.Universe()

res = u.residues[0]

# Previously
res[2]  # would return second Atom in Residue

# Now have to go via atoms attribute
res.atoms[2]

This also applies to things like __contains__, should atom in res work?

Also, select_atoms now has to be called off either a Universe or AtomGroup object, but because it explicitly says _atoms in the method name, shouldn't this always work off any object? (ie with SegmentGroup.select_atoms it's obvious what is meant, so allow it?)

I'm in favour of changing these things back to how they used to work, but just checking this seems better with everyone else.

poking @dotsdl in particular

[kain88-de]

For me.

1. res.atoms[2] is better then the old way
2. select_atoms should be callable from any group object.

[orbeckst]

I agree with @kain88-de 👍

1. slicing a XGroup should give an X (this needs to be documented clearly in CHANGELOG and docs as something new! It will break old code.)
2. select_atoms() needs to work from anywhere

EDIT: I think select_atoms on all of these things should be implemented as

class XGroup():
   def select_atoms(....):
        return self.atoms.select_atoms(...)

so that it works as expected, i.e., it selects from within the atoms unless globals are selected.

[mnmelo]

Another vote for @kain88-de's choice.

[dotsdl]

I'm in favor of @kain88-de's proposed approach as well. I don't think Residue and Segment objects should be sliceable, to keep things explicit and clean (not so many ways to do the same thing). Having select_atoms work from any group is fine; perhaps we should leave it off of Residue and Segment objects, though, since these technically aren't groups?

[orbeckst]

since these technically aren't groups

"Technically" is not really important from a user's point of view. I liked Residue = group of atoms (and Segment = group of residues) because it makes sense semantically.

Could we have Residues and Segments behave like such groups?

[dotsdl]

@orbeckst the problem is then that if you want a Residue to behave both as an AtomGroup and a Residue, then it will have attributes like charge (for the Residue's charge) and charges (for the charge on each atom). I think there were other cases where basically merging the API of a Residue and an AtomGroup under the new scheme causes clashes in expected functionality like this, and so then some things will work like an AtomGroup while others will not. That is more confusing for users in my view than just being explicit with e.g. Residue.atoms.

I'm looking for the thread where we discussed this, because we definitely settled this question a long time ago IIRC.

[richardjgowers]

Yeah I think David is right about attribute access having to refer to the Residue and never the atoms. My question is with the special getitem and contains methods, isn't the intent clear that we're trying to use the Residue as a group?

…

On Mon, 16 Jan 2017, 7:20 p.m. David Dotson, ***@***.***> wrote: @orbeckst <https://github.com/orbeckst> the problem is then that if you want a Residue to behave both as an AtomGroup *and* a Residue, then it will have attributes like charge (for the Residue's charge) and charges (for the charge on each atom). I think there were other cases where basically merging the API of a Residue and an AtomGroup under the new scheme causes clashes in expected functionality like this, and so then some things will work like an AtomGroup while others will not. *That* is more confusing for users in my view than just being explicit with e.g. Residue.atoms. I'm looking for the thread where we discussed this, because we definitely settled this question a long time ago IIRC. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1163 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI0jB-lsjtxDR5QF0kEUMAzE1KfNfgpbks5rS8MCgaJpZM4LkyQ-> .

[orbeckst]

@dotsdl yes, if you could dig out that thread.

I would expect Residue.charge to be the total charge and Residue.charges an array of the charges of the constituents, i.e., the atoms. Similarly, Segment.charges should be charges per residue, at least that's what the Segment/Residue/Atom hierarchy means. And if this were true then slicing should again give lists of the elements, i.e., either residues or atoms.

If this is technically hard to do – fine, we have to figure out where to spend developer time – but from a semantic perspective it would be meaningful and unambiguous. I would argue that not having this behavior introduces some metaphor shear into the UI.

[dotsdl]

@orbeckst see here for your own conclusion and here for some extended discussion.

My worry is that Residue objects will behave as AtomGroups in some ways and not in others (and same for Segments with regard to ResidueGroups).

@richardjgowers those mechanisms were things I was resistant to adding back in, but they were added back to maintain things that were present in the old system. I don't like them, since I think they cause us metaphor shear as @orbeckst says.

[orbeckst]

@dotsdl : I agreed that X.atoms (and X.residues) is a semantically good way to access properties belonging to atoms and residues.

I guess we had the discussion on Segment and Residue a long time ago #599 (comment)

So in general, Residue and Segment only give information about the singular object, rather than the plural contents.

I'll stop flogging a dead horse ... (even though I am not convinced that it is ambiguous as to what should happen when referring downward: in my mind a Residue should behave like an AtomGroup and a Segment should behave like a ResidueGroup).

The hard line view would be that neither Residue nor Segment should be iterable or slicable (to be consistent with "Residue and Segment are 'singular objects'"), even though from an oo/semantic point of view if atom in residue would be nice to have. However, if atom in residue.atoms is perhaps even clearer so yes, fine, #1163 (comment) the whole way:

1. no res[:]: TypeError
2. no atom in res: TypeError
3. have Residue.select_atoms == Residue.atoms.select_atoms

[richardjgowers]

Cool thanks for the quick consensus