Skip to content
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

add relprop for atom selection and corresponding UT #4841

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions package/MDAnalysis/core/selection.py
Original file line number Diff line number Diff line change
Expand Up @@ -1353,6 +1353,51 @@
return group[mask]


class RelPropertySelection(PropertySelection):
"""Some of the possible properties:
x, y, z,

ChiahsinChu marked this conversation as resolved.
Show resolved Hide resolved
.. versionadded:: 2.9.0
"""

token = "relprop"
precedence = 1

def __init__(self, parser, tokens):
super().__init__(parser, tokens)
self.sel = parser.parse_expression(self.precedence)
# self.ori_value = self.value
ChiahsinChu marked this conversation as resolved.
Show resolved Hide resolved

def _apply(self, group):
try:
values = getattr(group, self.props[self.prop])
except KeyError:
errmsg = f"Expected one of {list(self.props.keys())}"
raise SelectionError(errmsg) from None
except NoDataError:
attr = self.props[self.prop]
errmsg = f"This Universe does not contain {attr} information"
raise SelectionError(errmsg) from None

Check warning on line 1380 in package/MDAnalysis/core/selection.py

View check run for this annotation

Codecov / codecov/patch

package/MDAnalysis/core/selection.py#L1377-L1380

Added lines #L1377 - L1380 were not covered by tests
ChiahsinChu marked this conversation as resolved.
Show resolved Hide resolved

try:
col = {"x": 0, "y": 1, "z": 2}[self.prop]
except KeyError:
pass

Check warning on line 1385 in package/MDAnalysis/core/selection.py

View check run for this annotation

Codecov / codecov/patch

package/MDAnalysis/core/selection.py#L1384-L1385

Added lines #L1384 - L1385 were not covered by tests
ChiahsinChu marked this conversation as resolved.
Show resolved Hide resolved
else:
values = values[:, col]
sel = self.sel.apply(group)
rel_value = (
sel.center_of_geometry().reshape(3).astype(np.float32)[col]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@orbeckst orbeckst Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ChiahsinChu ChiahsinChu Dec 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. COG (not COM) is what I want to implement. In my cases, the planar surfaces are used, and taking the average positions of the surfaces is better than the minimal distances as reference. But I can see the case when the minimal distance would be a better choice, for example, for the curved surfaces. I suggest to have a new selecton class. What is your opinion?

)
values -= rel_value
ChiahsinChu marked this conversation as resolved.
Show resolved Hide resolved

if self.absolute:
values = np.abs(values)
mask = self.operator(values, self.value)

return group[mask]


class SameSelection(Selection):
"""
Selects all atoms that have the same subkeyword value as any atom in selection
Expand Down
17 changes: 17 additions & 0 deletions testsuite/MDAnalysisTests/core/test_atomselections.py
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,17 @@ def test_prop(self, universe):
assert_equal(len(sel), 3194)
assert_equal(len(sel2), 2001)

def test_relprop(self, universe):
sel1 = universe.select_atoms("relprop z <= 1 index 0")
sel2 = universe.select_atoms("relprop abs z <= 1 index 0")

positions = universe.trajectory[0].positions
ref = positions[0, 2]
mask_1 = (positions[:, 2] - ref) <= 1
assert_equal(len(sel1), np.count_nonzero(mask_1))
mask_2 = np.abs(positions[:, 2] - ref) <= 1
assert_equal(len(sel2), np.count_nonzero(mask_2))

def test_bynum(self, universe):
"Tests the bynum selection, also from AtomGroup instances (Issue 275)"
sel = universe.select_atoms('bynum 5')
Expand Down Expand Up @@ -1076,6 +1087,12 @@ def test_invalid_prop_selection(self, universe):
with pytest.raises(SelectionError, match="Expected one of"):
universe.select_atoms("prop parsnip < 2")

def test_invalid_relprop_selection(self, universe):
with pytest.raises(SelectionError, match="Expected one of"):
universe.select_atoms("relprop parsnip < 2 index 0")
with pytest.raises(SelectionError, match="Unknown selection token"):
universe.select_atoms("relprop z < 2")


def test_segid_and_resid():
u = make_Universe(('segids', 'resids'))
Expand Down
Loading