-
Notifications
You must be signed in to change notification settings - Fork 664
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding a ref point support for atom selection #4348
Conversation
Linter Bot Results:Hi @abdulasiraj! Thanks for making this PR. We linted your code and found the following: Some issues were found with the formatting of your code.
Please have a look at the Please note: The |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4348 +/- ##
===========================================
- Coverage 93.66% 93.23% -0.44%
===========================================
Files 168 12 -156
Lines 21248 1079 -20169
Branches 3917 0 -3917
===========================================
- Hits 19902 1006 -18896
+ Misses 888 73 -815
+ Partials 458 0 -458 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a good start, this is trying to make refpoint x y z
a possible selection as part of a command like sphlayer
in the selection language.
The return from _apply
here needs to usually be an AtomGroup, as other selections will expected it to provide things like center_of_geometry: https://github.com/MDAnalysis/mdanalysis/blob/develop/package/MDAnalysis/core/selection.py#L348
There's probably two ways to make this work:
- you could make the
_apply
method here return a AtomGroup-like object, so define a PseudoAtomGroup class here that duck types (imitates) all the required methods (like center_of_geometry etc). - instead, make all geometric selections that require a selection, instead have a branch where they can instead parse the
refpoint
here. This would change the signature ofsphlayer 1.0 2.0 selection OR refpoint
Option 2 here is probably more lines of code and seems less elegant, but option 1 seems a little fragile as the pseudo AtomGroup class would require a lot of maintenance to keep it behaving like a real AtomGroup
Thanks for the contribution @abdulasiraj and thanks for the initial round of feedback @richardjgowers . As there's still a WIP-label in the title I assume that you still want to do a fair amount of work by yourself @abdulasiraj . Once you think it's really ready for review, please change the name and then we can have closer look. |
@abdulasiraj I removed the |
That's make more sense. I'll update for sure. Thanks @RMeli |
@abdulasiraj are you still working on this PR? Otherwise we will close it. |
Hi @orbeckst, I'll complete it in few days hopefully. just got time to work on it. Thanks! |
Fixes #4344
Changes made in this Pull Request:
PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4348.org.readthedocs.build/en/4348/