select_atoms accepts atomgroups

[lilyminium]

Implements #2369

Changes made in this Pull Request:

• If u.select_atoms is given an Updating/AtomGroup, it returns that Updating/AtomGroup

@orbeckst commented on the need for careful testing:

I thought the same. Raise a separate issue. It looks like a simple change but this is such a fundamental method that it will require some careful testing. If it works consistently then it would be great. I think there was an old issue related to the "use AG where strings are allowed".

There is a simple test to check that it does return an AtomGroup, but otherwise I haven't got many ideas for others. Suggestions are welcome.

PR Checklist

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

[richardjgowers]

Looks good barring some quibbles

[codecov]

Codecov Report

Merging #2375 into develop will increase coverage by 0.02%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #2375      +/-   ##
===========================================
+ Coverage    89.89%   89.92%   +0.02%     
===========================================
  Files          173      173              
  Lines        21792    21808      +16     
  Branches      2862     2866       +4     
===========================================
+ Hits         19591    19610      +19     
+ Misses        1609     1607       -2     
+ Partials       592      591       -1

Impacted Files	Coverage Δ
package/MDAnalysis/core/groups.py	98.3% <100%> (+0.01%)	⬆️
package/MDAnalysis/analysis/rms.py	92.99% <100%> (+2.32%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9372d09...a739c44. Read the comment docs.

[orbeckst]

The macOS job ran into the 50min time limit. I restarted it but we might be back in the bad, old days where we have to run our tests faster...

[orbeckst]

Please don't forget docs and CHANGELOG!

[orbeckst]

In terms of tests I was thinking about MDAnalysis.analysis.rms.RMSD, ... such tests would be added to the tests of the analysis code to make sure it works with selection strings and AGs.

[orbeckst]

Re: CHANGELOG: has to go into a 0.21.0

[lilyminium]

This definitely requires a bit more thought that I've currently put into it.

Do we want select_atoms to return copies of AtomGroups?

What should happen when:

>>> ag = u.select_atoms('around 2 protein')
>>> u.select_atoms(ag, updating=True)

Should this generate an error or should the static AtomGroup be turned into a selection string of indices?

>>> ag2 = u.select_atoms('resid 1')
>>> ag.select_atoms(ag2)

Should this return the intersection of ag and ag2?

[lilyminium]

u.select_atoms(ag) now returns the intersection of u.atoms and ag. I've added an apply method to AtomGroup that should probably be renamed to something private like _apply_selection, but for now it's the same as Selection.apply.

I modified rms.RMSD to accept AtomGroups and added a couple tests. I'll probably need to go through all the analysis classes individually.

An UpdatingAtomGroup created from an AtomGroup uses repr(ag) as its selection string because the other options seemed very long...

[richardjgowers]

Yeah the updating case is weird because it won’t work as maybe it looks like at first. Once the ‘around ...’ selection is done, the resulting AG has no memory of how/why it was made, so can’t then retroactively be made updating.

I’m not super sold on the new way to perform intersections but I’ll think on it.

This whole patch is getting messy and I’m wondering if it’s better that analysis classes learn to accept either strings or (U)AGs.

[orbeckst]

I agree that switching from selection strings to only (U)AGs is the cleaner way to do this. Given that this might be more complicated than anticipated then let's put this on hold for 2.0. There's no good reason to break lots of user code just because we find this to be cleaner/neater... at least not in the short term.

I see 1.0 as the "stable/old" MDAnalysis release (minus crud like flags #782 ...).

For 2.0 we can apply our lessons learned.

[orbeckst]

I can be persuaded to change my opinion, of course.

But I learned that a good way to save time is to say "no" and stop work while you can. @lilyminium sorry to vote for killing the PR at this stage after you put the work in but this work was necessary to show potential problems. And I think that other things have higher priority.

Again, I am happy to be disagreed with.

[lilyminium]

Definitely happy to focus on other things. Not much work put in 😄 Analyses like rms.RMSD already accept both Universes and AtomGroups anyway, so it did feel a little unnecessary there. Should this be closed or left open?

[orbeckst]

I’d close the PR (but don’t delete it) and leave the issue open. Add a quick note to the issue summarizing the discussion for future generations/future us, please. Thanks!

…

Am 29.10.2019 um 00:28 schrieb Lily Wang ***@***.***>:  Definitely happy to focus on other things. Not much work put in 😄 Analyses like rms.RMSD already accept both Universes and AtomGroups anyway, so it did feel a little unnecessary there. Should this be closed or left open? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.