Add no selection option to RMSD analysis class#5296
Add no selection option to RMSD analysis class#5296ollyfutur wants to merge 10 commits intoMDAnalysis:developfrom
Conversation
RMSD analysis class now accepts `select=None` to prevent any selection on the provided `atomgroup` and `reference`.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5296 +/- ##
========================================
Coverage 93.82% 93.83%
========================================
Files 182 182
Lines 22483 22485 +2
Branches 3195 3196 +1
========================================
+ Hits 21095 21099 +4
+ Misses 925 924 -1
+ Partials 463 462 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jeremyleung521
left a comment
jeremyleung521
left a comment
There was a problem hiding this comment.
Just some ideas/suggestions on the implementation. Probably need someone more experienced to approve :)
package/MDAnalysis/analysis/rms.py
Outdated
| self.mobile_atoms = self.atomgroup.select_atoms(*select["mobile"]) | ||
| self.ref_atoms = ( | ||
| self.reference | ||
| if select["reference"] is None |
There was a problem hiding this comment.
I would do b if c is not None else a (like L542~545) instead of what we have here a if c is None else b.
Makes it easier to read (the default is more common) and the code more uniform.
Also if you're already doing checks here, why are we doing extra checks/processing in process_selection? We could just query directly if select is not None...
There was a problem hiding this comment.
Makes it easier to read (the default is more common) and the code more uniform.
I agree that the default should come first, I’ll update (though L542~545 you’ll notice that the default groupselections=None comes second!)
Also if you're already doing checks here, why are we doing extra checks/processing in
process_selection?
The current implementation handles cases such as passing select={"reference": None, "mobile": "some selection string"}, which feels consistent with the behavior of passing a single selection string to be applied to both reference and mobile. It can be handled in another way though…
There was a problem hiding this comment.
Whoops. Anyhow, definitely easier to read with this change though :)
Just exploring the alternatives here. I can see why we want select consistently as a dictionary with two keys too, so maybe I'm just overthinking it.
| "select dictionary must contain entries for keys " | ||
| "'mobile' and 'reference'." | ||
| ) from None | ||
| elif select is None: |
There was a problem hiding this comment.
One thing that I keep going back and forth in my head is that 'all' and None have so much overlap I'm wondering if it's better to modify 'all' behavior to be like None (no sorting). Maybe someone more experienced should comment on this.
There was a problem hiding this comment.
The problem is that it would be inconsistent with the usual atomgroup.select_atoms("all"), which is expected to sort… I think that conceptually None translates better the idea that no operation changes the provided groups.
There was a problem hiding this comment.
Just read the docs and it's clear that sorting is intended and not just a side effect. So yea, I think None probably is the best way here.
https://docs.mdanalysis.org/2.9.0/documentation_pages/selections.html
Selections always return an [AtomGroup](https://docs.mdanalysis.org/2.9.0/documentation_pages/core/groups.html#MDAnalysis.core.groups.AtomGroup) with atoms sorted according to their index in the topology (this is to ensure that there are not any duplicates, which can happen with complicated selections).
LGTM now.
2 participants
Changes made in this Pull Request:
MDAnalysis.analysis.rms.RMSDnow acceptsselect=Noneto prevent any further selection onatomgroupandreference. This is useful e.g. to avoid reordering of the atomgroups.LLM / AI generated code disclosure
LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: no
PR Checklist
package/CHANGELOGfile updated?package/AUTHORS? (If it is not, add it!)Developers Certificate of Origin
I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.
📚 Documentation preview 📚: https://mdanalysis--5296.org.readthedocs.build/en/5296/