-
Notifications
You must be signed in to change notification settings - Fork 646
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
feat: add zone
selection to support a box like selection from given selection
#4324
base: develop
Are you sure you want to change the base?
Conversation
Hello @Cloudac7! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-07-25 04:03:36 UTC |
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.
Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on the developer mailing list so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS
as part of this PR.
Linter Bot Results:Hi @Cloudac7! 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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4324 +/- ##
===========================================
- Coverage 93.61% 93.60% -0.02%
===========================================
Files 171 183 +12
Lines 21250 22369 +1119
Branches 3936 3951 +15
===========================================
+ Hits 19893 20938 +1045
- Misses 898 971 +73
- Partials 459 460 +1 ☔ 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.
Apologies for the slow reviews @Cloudac7
I've added some initial comments.
@MDAnalysis/coredevs could someone please take on the responsibility of reviewing this PR?
package/doc/sphinx/source/conf.py
Outdated
@@ -14,6 +14,8 @@ | |||
import sys | |||
import os | |||
import datetime | |||
sys.path.insert(0, os.path.abspath('../../..')) |
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.
Could you explain why this is being added?
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.
Sorry for forgetting remove the line.
I am developing on MacOS platform and thus the compiling of documentation with original code would raise an ModuleNotFoundError: No module named 'MDAnalysis'
. My practice followed the instruction of latest documentation for developing, and no solution found. After adding the line in more upper position, sphinx-build
could work.
@@ -277,6 +277,23 @@ cyzone *externalRadius* *zMax* *zMin* *selection* | |||
relative to the COG of *selection*, instead of absolute z-values | |||
in the box. | |||
|
|||
box *dimension(s)* *d1_Max* *d1_Min* (*d2_Max* *d2_Min*) (*d3_Max* *d3_Min*) *selection* |
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.
I'm very likely missing something, but I don't know if I fully understand how this differs from using prop
- could you maybe explain the benefits of this new selection over using a succession of prop
?
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.
Well, prop
is used to define a static zone in each axes, in my understanding. The new defined box
selection do similar thing like which sphzone
, cyzone
or cylayer
do, thus could define a zone according to another selection which could also be dynamical when updating=True
.
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.
See @Cloudac7 's description in #4323 (comment) , which looks sensible to me. If we have cyzone then we can also have box.
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 would make more sense to me as min
to max
values rather than the opposite. E.g. box z 0 8 bilayer
to select a slice of water above a bilayer and box z -8 0
to get the other half.
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.
@richardjgowers I had the same thoughts but unfortunately cyzone and friends started this inverse "max min" syntax. I feel consistency across the *zone
and *layer
selections is more important here than purity.
(I mean I am really looking for someone to convince me we should do min max
here ;-) because I know that I am going to mistype it...)
package/MDAnalysis/core/selection.py
Outdated
super().__init__(parser, tokens) | ||
self.periodic = parser.periodic | ||
self.direction = tokens.popleft() | ||
if self.direction not in self.combination: |
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.
can we set xmin
xmax
etc here to None
then optionally set them to something else in the else:
branch below? I think this would simplify the __getattribute__
usage below
@@ -277,6 +277,23 @@ cyzone *externalRadius* *zMax* *zMin* *selection* | |||
relative to the COG of *selection*, instead of absolute z-values | |||
in the box. | |||
|
|||
box *dimension(s)* *d1_Max* *d1_Min* (*d2_Max* *d2_Min*) (*d3_Max* *d3_Min*) *selection* |
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 would make more sense to me as min
to max
values rather than the opposite. E.g. box z 0 8 bilayer
to select a slice of water above a bilayer and box z -8 0
to get the other half.
bf93731
to
036eea5
Compare
package/MDAnalysis/core/selection.py
Outdated
combination = [ | ||
"x", | ||
"y", | ||
"z", | ||
"xy", | ||
"xz", | ||
"yz", | ||
"yx", | ||
"zx", | ||
"zy", | ||
"xyz", | ||
"xzy", | ||
"yxz", | ||
"yzx", | ||
"zxy", | ||
"zyx", | ||
] |
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.
combination = [ | |
"x", | |
"y", | |
"z", | |
"xy", | |
"xz", | |
"yz", | |
"yx", | |
"zx", | |
"zy", | |
"xyz", | |
"xzy", | |
"yxz", | |
"yzx", | |
"zxy", | |
"zyx", | |
] | |
combination = [''.join(s) for i in range(1, 4) for s in itertools.permutations("xyz", i)] |
Just for fun. Probably less readable than what you wrote.
But the question is, do we really need both xy
and yx
? What's the use case of having all permutations as opposed to only combinations?
combination = [ | |
"x", | |
"y", | |
"z", | |
"xy", | |
"xz", | |
"yz", | |
"yx", | |
"zx", | |
"zy", | |
"xyz", | |
"xzy", | |
"yxz", | |
"yzx", | |
"zxy", | |
"zyx", | |
] | |
combination = [''.join(s) for i in range(1, 4) for s in itertools.combinations("xyz", i)] |
Nitpick: the variable is called "combination", so the name might have to change to "permutation" ("permutations"?) if that's the actual use case.
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.
The extensive list of all permutations of x
, y
, and z
is designed to avoid the need to calculate the O(3!) = O(1) permutations each time a selection is executed. This has been reformatted by darker
to comply with PEP8 standards. It's a minor optimization that can be overlooked, though. 🤣
Regarding the latter point, my original intention was to enhance the flexibility of the selection by making both yx
and xy
acceptable. However, this inadvertently added extra complexity. You're correct in suggesting that it would be better to have users write in the sequence of x -> y -> z
. This approach would eliminate the need for additional processing.
package/MDAnalysis/core/selection.py
Outdated
"The direction '{}' is not valid. Must be combination of {}" | ||
"".format(self.direction, ["x", "y", "z"]) |
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.
"The direction '{}' is not valid. Must be combination of {}" | |
"".format(self.direction, ["x", "y", "z"]) | |
f"The direction '{self.direction}' is not valid. Must be combination of {axis_map}" |
8d7f2ce
to
0090016
Compare
doc: add doc about box selection fix: reformat with darker and flake8
style: reformat as pep8
test: add error catching for box selection fix: prevent abnormal input in direction
style: reformat CHANGELOG
commit 8d7f2ce Author: Futaki Haduki <[email protected]> Date: Thu Nov 9 17:23:57 2023 +0800 Update package/MDAnalysis/core/groups.py Co-authored-by: Rocco Meli <[email protected]> commit cfdec5b Author: Futaki Haduki <[email protected]> Date: Thu Nov 9 17:23:44 2023 +0800 Update package/MDAnalysis/core/groups.py Co-authored-by: Rocco Meli <[email protected]>
0090016
to
773ef5a
Compare
@richardjgowers I am tentatively assigning the PR to you for shepherding it through. If it's too much then please unassign yourself and ping me. Thanks. |
@richardjgowers could you have a look if @Cloudac7 addressed the reviewers' comments? Thanks! |
@richardjgowers could you please have another look? today on discord someone asked for this feature so I was reminded of this PR… |
@Cloudac7 could you please resolve the conflicts? Sorry that this has been very slow from our end. |
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.
Looks good, couple tweaks to go
package/MDAnalysis/core/selection.py
Outdated
} | ||
|
||
if self.periodic and group.dimensions is not None: | ||
box = group.dimensions[:3] |
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.
I'm not sure this is correct for triclinic boxes, I think instead you want the trace of the 3x3 vector representation
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.
In fact, I'm not sure if the check necessary. In my opinion, it might be nature to select a zone a little larger than the unit cell, yielding all of the atoms in the unit cell on the corresponding direction. Although it would be misleading because of PBC.
feat: remove check on unit cell for `zone`
zone
selection to support a box like selection from given selection
Description:
It introduces a new geometry selection, as described in #4323.
A new selection language is defined in format:
It could pick a zone from the center of geometry from selection.
Changes made in this Pull Request:
PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4324.org.readthedocs.build/en/4324/