AtomGroup API: convert some methods to properties

[orbeckst]

We should discuss if we also want to break the AtomGroup API with 0.11.0, as discussed on and off on the developer list (@richardjgowers , @mnmelo ): request for discussion: changing parts of AtomGroup API. A number of methods of AtomGroup are really conceptually more attributes (typically, quantities derived from the topology) so a good case can be made to turn them into managed properties. (I cannot remember if there was a specific rationale in the early days why they were methods – maybe @nmichaud remembers...?)

The 0.11.0 release would be a (comparatively) good time because other stuff changes, too, and if users need to rework their code then they should really only have to do it once. (Users will hate the changes outlined here because they will break code in cryptic ways and I don't see a way to have the same method and property co-exist for a while.)

I'd like to open a discussion with a focus on

1. Do we want to change this? Is this really necessary?
2. What is the full list of changes needed? (We are unlikely to do a second major API break before 1.0 so this should be it so if we want to do this we will set a new target date for 0.11.0.)
3. How can we mitigate the pain for users? Can we write a script that helps with the code transformation?

Proposal

Turn methods that mainly report on topology properties into managed attributes.
For example:

ag.resids()    --->   ag.resids

Provide setters when possible and replace the corresponding set_* methods:

# old (currently)
ag.set_resname(['ALA', 'ALA'])

# new (proposed)
ag.resnames = ['ALA', 'ALA']

AtomGroup methods to be changed

I suggest the following methods to be converted to managed attributes:

•   bfactors  (currently attribute but scheduled to become a method, so actually keep as attribute )
  

•   charges
  

•   indices
  

•   masses
  

•   names
  

•   radii
  

•   resids
  

•   resnames
  

•   resnums
  

•   segids
  

[hainm]

Do we want to change this? Is this really necessary?

yes, much less typing; cleaner code, more Pythonic ... and because all other packages use python property.

PS: summer is almost gone and I am still waiting for @richardjgowers for his py3. ha ha.

[hainm]

and numberOfAtoms() should just be n_atoms or natom

[richardjgowers]

So I'm in favour of properties for 0.11.0. Letting the setters also be callable does let you specify extra arguments.. so we could do

atomgroup.set_resid([0, 1, 2, 3], fancy_option=True)

But currently I think it's only set_segid that would use this. It's very easy to include set_x and have that use the property (or vice versa).

It's difficult to mitigate pain, but a script should be easy enough.

With resids and friends, there's some odd behaviour there. While masses returns an array of len(atomgroup), resids returns an array of len(atomgroup.residues). I don't use the residue system for biological simulations though, so is this behaviour as expected for this?

I would expect:

# This returns len(atomgroup)
# because I'm working on atoms
atomgroup.resids = [0, 0, 1, 1, 1, 2, 2]

# This returns len(residuegroup)
# because I'm working on residues
residuegroup.resids = [0, 1, 2]

There's some Atom properties that aren't accessible via AtomGroup, I think off the top of my head it's altLoc and serial. But basically any Atom.property should exist as AtomGroup.properties

The other thing we had briefly discussed was moving away from camelCase. This would then entail

atomgroup.selectAtoms -> atomgroup.select_atoms

This is big, but it's painless as we can keep selectAtoms as deprecated.

[mnmelo]

I'd like to add a request for an Atom-property-to-AtomGroup-properties dictionary, so that selections for Atom.property know where to look for in AtomGroup's attributes.

It should be easy, and it's mostly the plural of names (mass to masses). And it'd be great if it could be automatically done for new attributes, to lessen the burden on the dev (though I'm not really sure how).

Maybe with @richardjgowers's new AtomGroups as structured arrays in 1.0 this will come for free.

[richardjgowers]

So you mean somewhere there's a dict like

PLURALS = {'radius':'radii', etc

SINGLES = {'radii':'radius' etc (can do this as a dict inversion of previous

[mnmelo]

Exactly. Then in the Selection module we don't even need explicit lists of selectable attributes. We simply try getattr(AtomGroup, PLURALS["searched_term"]).

It's not as straightforward, since different attributes will need different selection comparison methods (less so when they all become fields in a structured array), but it's a start to making selections more general and clean up code duplication.

[richardjgowers]

So another thing I've found, atoms have the number property, and the plural of this is indices. So should Atoms have index, or AtomGroup have numbers?

[orbeckst]

I'd vote for introducing Atom.index and deprecating number (which probably comes from CHARMM historically and should also be reflected in the selection syntax).

Oliver Beckstein
email: [email protected]
skype: orbeckst

Am Jul 29, 2015 um 13:37 schrieb Richard Gowers [email protected]:

So another thing I've found, atoms have the number property, and the plural of this is indices. So should Atoms have index, or AtomGroup have numbers?

—
Reply to this email directly or view it on GitHub.

[richardjgowers]

I've pushed a branch that does everything proposed so far.

TODO:

• address atom.index question
• tests for residue related things via properties
• tests for using property setters directly
• check docs updated everywhere

@mnmelo I've added your dict to core/AtomGroup as below. If it prevents a circular import this can be moved to somewhere else.

+# Constant to translate the name of an Atom's property
+# to the plural version, as found in AtomGroup
+_PLURAL_PROPERTIES = {'number': 'indices',
+                      'name': 'names',
+                      'type': 'types',
+                      'resname': 'resnames',
+                      'resid': 'resids',
+                      'segid': 'segids',
+                      'mass': 'masses',
+                      'charge': 'charges',
+                      'radius': 'radii',
+                      'bfactor': 'bfactors',
+                      'resnum': 'resnums',
+                      'altLoc': 'altLocs',
+                      'serial': 'serials'}
+# And the return route
+_SINGULAR_PROPERTIES = {v: k for k, v in _PLURAL_PROPERTIES.items()}

[dotsdl]

I'm very much in favor of this, especially making a one-to-one correspondence between AtomGroup and Atom properties and ResidueGroup and Residue properties. One thing I would change from its current behavior is AtomGroup.resids, which currently tries to be smarter than I think it should be for the sake of consistency. As @richardjgowers suggested, it should return an array of equal size to the AtomGroup.

[orbeckst]

On 31 Jul, 2015, at 15:50, David Dotson wrote:

One thing I would change from its current behavior is AtomGroup.resids, which currently tries to be smarter than I think it should be for the sake of consistency. As @richardjgowers suggested, it should return an array of equal size to the AtomGroup.

Personally, I would find it very annoying of AtomGroup.resids gave me one resid per atom. Most of the time I use it to figure out the resids in the group. Having to do a np.unique(AtomGroup.resids) or AtomGroup.residues.resids (?) every time seems a bit cumbersome (and I am not even sure if the latter does what I'd want it to do). The same goes for AtomGroup.segids, by the way. So in that sense there's more than one property that will not return the size of the AG.

On the other hand, there's something to be said for consistency, so any idea how to have the cake and eat it, too? Or does no-one else need the convenience?

[dotsdl]

Doesn't AtomGroup.segments.segids get the same thing as AtomGroup.segids at the moment? I think consistency is better here, since there certainly have been cases where I needed to get the resid for every atom in an atomgroup and had to do something like

[atom.resid for atom in atomgroup]

to get them. That's what I'd call cumbersome. Since the current behavior can be obtained with AtomGroup.residues.resids anyhow, I think this is worth changing to make it consistent and predictable.

[dotsdl]

Not to appeal to the Zen of Python, but I think:

There should be one-- and preferably only one --obvious way to do it.

[orbeckst]

@dotsdl , please open a new issue for sorting out the size of the returned arrays for ag.resids and friends.

Just summarize (copy & paste...) the state of the discussion.

[orbeckst]

@hainm , please open (or re-open if there was alreay an issue) your suggestion to change AtomGroup.numberOfAtoms() into something else. I'd personally favor AtomGroup.numatoms (property and sounds similar to trajectory.numframes) but we can discuss this in the separate issue.

[orbeckst]

@richardjgowers , I merged #374 because the sooner we start switching over the better (if the developers cannot deal with the change then users will have a really hard time..) but I think not everything from you checklist was completed, in particular using the property setters is not being tested for and the docs have not been adjusted.

We will close this issue when these points have been addressed.

[orbeckst]

@richardjgowers , please open a new issue for changing selectAtoms() to select_atoms():

The other thing we had briefly discussed was moving away from camelCase. This would then entail

atomgroup.selectAtoms -> atomgroup.select_atoms

This is big, but it's painless as we can keep selectAtoms() as deprecated.

[hainm]

Sure. I will.

Hai

On Aug 3, 2015, at 7:21 PM, Oliver Beckstein [email protected] wrote:

@hainm , please open (or re-open if there was alreay an issue) your suggestion to change AtomGroup.numberOfAtoms() into something else. I'd personally favor AtomGroup.numatoms (property and sounds similar to trajectory.numframes but we can discuss this in the separate issue.

—
Reply to this email directly or view it on GitHub.

[richardjgowers]

Ok will do.

[orbeckst]

@richardjgowers , can you close this issue?

Did you address the remaining points from your checklist, in particular testing the property setters and have the docs been adjusted (perhaps @tylerjereddy knows this, i.e. #382 ).

[richardjgowers]

I think the only thing holding this up is @dotsdl with #385. I think tests and docs are all ok.

[dotsdl]

@richardjgowers finishing this up today, by hook or by crook. :D