Hotfix: ResidueGroup of an AtomGroup now updated on resname or resnum set

[dotsdl]

Setting the resnums or resnames directly of an AtomGroup failed to update the resnums/resnames of that AtomGroups's ResidueGroup. This has been fixed. This should be included as a hotfix, since it fixes what amounts to a severe break of resnum behavior.

[kain88-de]

Could you also add a unit-test that fails without the fix and passes with it?

[richardjgowers]

Ch Ch Ch Ch Changelog?

[orbeckst]

This should go into a 0.11.1.

Do we need a milestone 0.11-fixes in the issue tracker for these fixes?

[dotsdl]

Perhaps a discussion is worth having here. Currently one can set the resnums of an AtomGroup with, say:

ag.set_resnums(3)
print ag.resnums

and this will work even if the atoms in the AtomGroup come from many different residues or if there are other atoms in the same residues that are not in the AtomGroup. The question in these cases, then, is what one should see with:

print ag.residues.resnums

Should each residue give as their resnum the resnum of the first atom in that residue (in topology order)? The atoms in a given residue may all have different resnums. The same problem occurs for resnames, too.

[richardjgowers]

I was going to put all 0.11.0 -> 0.12.0 stuff into the 0.12 milestone. Once enough features stacked up I'd release and iterate the minor number (Z in X.Y.Z). Then once all the 0.12 stuff was done I'd iterate Y. I think the milestone system is useful for longer term planning than Zs.

[orbeckst]

That should work, given that everything that matters is in CHANGELOG.

[richardjgowers]

And re: the issue, I think the problem is it's over defined initially, (negative degrees of freedom if we want to be fancy) so editing things is always going to end in tears. Ways to fix this...

• have Atom.resname point to Atom.residue.resname (reduce degrees of freedom)
• have Residue check all resnames and issue a warning if inconsistency found (but still return [0])

def resname(self):
    resname = self[0].resname
    if not all(self.resnames == resname):
        warn people
    return resname

I think I'm more in favour of option 2

[orbeckst]

@dotsdl : maybe we should simply restrict setters for some of these to only work at the appropriate level??

It would be useful to get an idea of how people are actually using the setters.

[hainm]

just curious, why not using name for all. atom.name; residue.name.

residue.resname is seem to be duplicated.

[dotsdl]

Whatever the solution, it needs to come fast because at the moment using:

import MDAnalysis as mda
from MDAnalysisTests.datafiles import GRO, XTC

u = mda.Universe(GRO, XTC)
protein = u.select_atoms('protein')

resids = protein.resids
protein.set_resnums(resids + 3)

gives then

>>> print protein.resnums
[  4   4   4 ..., 217 217 217]
>>> print protein.residues.resnums
[  1   2   3   4   5   6   7   8   9  10  11  12  13  14  15  16  17  18
  19  20  21  22  23  24  25  26  27  28  29  30  31  32  33  34  35  36
  37  38  39  40  41  42  43  44  45  46  47  48  49  50  51  52  53  54
  55  56  57  58  59  60  61  62  63  64  65  66  67  68  69  70  71  72
  73  74  75  76  77  78  79  80  81  82  83  84  85  86  87  88  89  90
  91  92  93  94  95  96  97  98  99 100 101 102 103 104 105 106 107 108
 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126
 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144
 145 146 147 148 149 150 151 152 153 154 155 156 157 158 159 160 161 162
 163 164 165 166 167 168 169 170 171 172 173 174 175 176 177 178 179 180
 181 182 183 184 185 186 187 188 189 190 191 192 193 194 195 196 197 198
 199 200 201 202 203 204 205 206 207 208 209 210 211 212 213 214]

doing instead

resids = protein.residues.resids
protein.residues.set_resnums(resids + 3)

does produce the correct result, however, for protein.resnums and protein.residues.resnums. I think I'm in favor of removing setting resnums, resnames, or segids at levels lower than those properties make sense for, since that would remove the possibility for this kind of weirdness. Setting resids and segids at the atom level can still work for merging residues and segments, however. Likewise for setting segids at the residue level.

[dotsdl]

@hainm because a Residue object is itself an AtomGroup. Although the discussion has been broached as to whether the "everything is an AtomGroup" object model should be re-evaluated (see #411), for the moment using .name might be a bit ambiguous.

[hainm]

I see. thanks.

[richardjgowers]

@dotsdl I think what you've posted is the expected behaviour. If you set at the atom level then it will be able to diverge from the Residue.

If you wanted to pull out a big hammer, you could remove .resnum from Atom and make it only exist in Residue, so AtomGroup.set_resnums didn't make sense. Then if you wanted to change a resnum, you'd need to first change the resid to move it into a new Residue, then change the resnum of the Residue. Is that what you were meaning before?

[dotsdl]

@richardjgowers If the resnums of individual atoms are expected to be independent of the resnums of their residues when set at the level of the atoms (but not at the level of the residues), then I suppose this behavior is fine. I still think it's a bit confusing, and it is actually different behavior than what was the case previously.

If we don't want resnums to work this way, the only reasonable alternative I see removing the ability to set resnums at the atom level (but still get them on a per-atom basis, as we worked hard on in #385). Same for resnames. Where setting would still work is for resids and segids, since these work as primary keys for determining what is a residue and what is a segment.

[richardjgowers]

If you put in the warnings I suggested above would it catch this?

[dotsdl]

@richardjgowers I think this is still a problem even with warnings because it almost certainly will result in unexpected behavior that might not be caught. This can be disastrous for code that is doing something complicated if the resnums are set at the atom level but accessed at the residue level. Plus, the instances in which this warning would not apply would be instances in which AtomGroup.residues.set_resids would work just as well without any possibility for problems.

[dotsdl]

I created an issue for this. I'm closing the PR for now until we settle on the solution. I suggest continuing the discussion there.