fixes #1795

[richardjgowers]

Fixes #1795

Changes made in this Pull Request:

• SphericalZone and SphericalLayer no longer shift atoms to inside
  primary unit cell when calculating center of reference group

PR Checklist

• Tests?
• Docs?
• CHANGELOG updated?
• Issue raised/referenced?

[richardjgowers]

I think this finally fixes #1795. center_of_geometry(pbc=True) doesn't really make sense for these selections, if anything we need center(unwrap=True) ie #2012 but that's not merged yet. The test that is added is an unwrapped system, so shouldn't change before/after this change.

[tylerjereddy]

I went back to the code reported as problematic in the original issue and confirmed that the lastest develop (259f778) gives 40 residues on that selection and @richardjgowers feature branch here (e8a7863) gives 5, which is what the issue reporter expects / wants. So, I think mechanically this is fine & we should perhaps merge soon-ish to fix that mechanical issue.

I'd probably feel a little safer if we were to use the exact reported issue example as a regression test, but I trust that the test used here is similar enough & avoids the additional test data file burden.

I do have a few suggestions / comments, that are perhaps better suited to follow-up PRs or at least issues so we don't forget.

• the docs for sphzone / sphlayer still indicate that the COG / centroid of the ref group is used -- is this still true? If so, do we now gracefully handle the scenarios I was confused about in my diagram?
• the linked issue / PR discussions for this problem also raised some of my confusion about pbc-related flags in our code base (there's a few of them, it is confusing, etc.); does that persist?
• I don't know if some of the behavior adjustment here warrants i.e., versionchanged directives in the docs; if a temporary bug just crept in maybe not I suppose

[tylerjereddy]

The Travis failure is likely unrelated -- we should maybe go ahead and do a crude pinning fix to the NumPy version there I suppose so we don't block PRs & then rebase here maybe.

[richardjgowers]

@tylerjereddy the centroid is still used, but it always assumes that all atoms are in the correct image to calculate this. WRT your sketch, if you had protein/X atoms in either side of the box this selection would fail because it would calculate the centroid as roughly the center of the box. This is exactly what unwrap would fix.

In general pbc=True needs to get replaced with wrap=True (ie consider all atoms in the primary unit cell), and unwrap=True (make sure bonds aren't broken across periodic boundaries) which is what #2012 is about.

[zemanj]

In general pbc=True needs to get replaced with wrap=True (ie consider all atoms in the primary unit cell), and unwrap=True (make sure bonds aren't broken across periodic boundaries) which is what #2012 is about.

I agree with @richardjgowers that this can only be fixed for the general case (atom coordinates in arbitrary images) by first wrapping and then unwrapping the system. But even then, PBC have to be taken into account when calculating the COG of the reference group (and afterwards when computing distances, but that's already handled correctly I think).

If I'm not mistaken, this PR currently only works correctly for already unwrapped systems without PBC.

[richardjgowers]

@zemanj yep we need proper unwrap support. But this PR fixes the regression that happened when we started to use pbc=True inside the COG calculation.

[orbeckst]

Could someone take the responsibility and merge? (I haven't really followed the conversation in depth but nothing has happened over the last 2d and the status is green so naively I'd think one could hit the button...)

[richardjgowers]

I think I addressed @tylerjereddy 's comments, so I'm merging this

tl;dr there's still wrap/unwrap issues, but this fixes the regression that happened