Add no selection option to RMSD analysis class#5296
Add no selection option to RMSD analysis class#5296ollyfutur wants to merge 11 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 22487 +4
Branches 3195 3198 +3
========================================
+ Hits 21095 21101 +6
+ 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 :)
| "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.
There was a problem hiding this comment.
Reflecting on this, this is how alignto handles it:
mdanalysis/package/MDAnalysis/analysis/align.py
Lines 409 to 413 in 9531c6e
So there is a legitimate debate on whether RMSD should or should not adopt a similar behavior.
There was a problem hiding this comment.
None provides a unique functionality, i.e., leaving mobile and ref unchanged, so I see much more value in letting them do what @ollyfutur is intending here.
I'd say, the fact that align chooses "all" when selecting None is more a historical accident where None was just used as "default" and an argument could be made to change its behavior for "all". It would be more consistent if "all" behaved like a selection.
(by the way, "all" is not properly documented in https://docs.mdanalysis.org/2.10.0/documentation_pages/selections.html ... )
|
@ollyfutur could you please raise an issue that we can then reference? I think you're making a good point here. Given that this has some implications for our API and the discussion on behavior of None vs all we should have an issue for it. This PR can then "fix the issue". |
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
I like the change and the code. Please
- raise an issue that we can reference; describe the problem and the solution in the issue
- add a bit more documentation
package/CHANGELOG
Outdated
| DSSP by porting upstream PyDSSP 0.9.1 fix (Issue #4913) | ||
|
|
||
| Enhancements | ||
| * Allow `select=None` in `MDAnalysis.analysis.rms.RMSD` |
There was a problem hiding this comment.
describe succinctly what None does, reference Issue and PR
| - `str` -> Any valid string selection | ||
| - `dict` -> ``{'mobile':sel1, 'reference':sel2}`` | ||
| - `tuple` -> ``(sel1, sel2)`` | ||
| - ``None`` |
| "select dictionary must contain entries for keys " | ||
| "'mobile' and 'reference'." | ||
| ) from None | ||
| elif select is None: |
There was a problem hiding this comment.
None provides a unique functionality, i.e., leaving mobile and ref unchanged, so I see much more value in letting them do what @ollyfutur is intending here.
I'd say, the fact that align chooses "all" when selecting None is more a historical accident where None was just used as "default" and an argument could be made to change its behavior for "all". It would be more consistent if "all" behaved like a selection.
(by the way, "all" is not properly documented in https://docs.mdanalysis.org/2.10.0/documentation_pages/selections.html ... )
3 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/