add relprop for atom selection and corresponding UT
#4841
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4841 +/- ##
===========================================
- Coverage 93.65% 93.60% -0.06%
===========================================
Files 177 189 +12
Lines 21774 22868 +1094
Branches 3064 3065 +1
===========================================
+ Hits 20393 21405 +1012
- Misses 929 1010 +81
- Partials 452 453 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Looks sensible at first glance.
In addition to the inline comments, could you please also add documentation to core.groups.AtomGroup.select_atoms() after prop
mdanalysis/package/MDAnalysis/core/groups.py
Line 2995 in ac145ec
prop 's docs) so that we can see how users would use it.
UPDATE: I added a few more comments after submitting my review. Please look at these, too. In particular, can you please explain the rationale for comparing to the COM of the reference group instead of calculating all distances and then comparing the min?
Additionally, the comparison needs to be able to take into account PBC (periodic=True).
| try: | ||
| col = {"x": 0, "y": 1, "z": 2}[self.prop] | ||
| except KeyError: | ||
| pass |
There was a problem hiding this comment.
It looks to me that you shouldn't pass here as there's no meaningful way to return results. Please look into the case when line 1385 is executed. A test for this case would probably reveal the issue.
| """Some of the possible properties: | ||
| x, y, z, | ||
|
|
There was a problem hiding this comment.
Please add more documentation, explaining what this operator is doing.
| except NoDataError: | ||
| attr = self.props[self.prop] | ||
| errmsg = f"This Universe does not contain {attr} information" | ||
| raise SelectionError(errmsg) from None |
There was a problem hiding this comment.
ultimately we will also need a test for this failure case
| values = values[:, col] | ||
| sel = self.sel.apply(group) | ||
| rel_value = ( | ||
| sel.center_of_geometry().reshape(3).astype(np.float32)[col] |
There was a problem hiding this comment.
Your docs should make clear that the COM of the second group is used.
Is this what this selection should do? Or should it calculate all-vs-all distances and then compute the min over all distances and then compare this minimal distance to the value?
There was a problem hiding this comment.
When using center_of_geometry you also need to think carefully to what value wrap and unwrap needs to be set, especially if the user does not get a choice in setting it.
| def __init__(self, parser, tokens): | ||
| super().__init__(parser, tokens) | ||
| self.sel = parser.parse_expression(self.precedence) | ||
| # self.ori_value = self.value |
| rel_value = ( | ||
| sel.center_of_geometry().reshape(3).astype(np.float32)[col] | ||
| ) | ||
| values -= rel_value |
There was a problem hiding this comment.
This calculation may not be correct under PBC.
Look into MDAnalysis.lib.distances.distance_array (or probably better MDAnalysis.lib.distances.capped_distance) and MDAnalysis.lib.distances.minimize_vectors for fast functions that can calculate distances under PBC.
Implementaton of #4838
Changes made in this Pull Request:
relpropkeyword for atom selectionI will update the User Guide once this implementation (especially the interface to users) is accepted.
PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4841.org.readthedocs.build/en/4841/