From 7a57af161b6a636cea8a778ca6a5a8927f483d45 Mon Sep 17 00:00:00 2001 From: orioncohen Date: Fri, 2 Apr 2021 16:49:23 -0700 Subject: [PATCH 01/11] Fixed issue #2915 and added a pytest to demonstrate the fix. Sphzone operating on an empty selection now returns an empty atom group. --- package/MDAnalysis/core/selection.py | 4 +++- testsuite/MDAnalysisTests/core/test_atomselections.py | 5 +++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/package/MDAnalysis/core/selection.py b/package/MDAnalysis/core/selection.py index adff7ab3eeb..feeb2144396 100644 --- a/package/MDAnalysis/core/selection.py +++ b/package/MDAnalysis/core/selection.py @@ -375,11 +375,13 @@ def __init__(self, parser, tokens): self.sel = parser.parse_expression(self.precedence) @return_empty_on_apply - def apply(self, group): + def apply(self, group): # TODO fix atom selection indices = [] sel = self.sel.apply(group) box = self.validate_dimensions(group.dimensions) periodic = box is not None + if len(sel) == 0: + return group[[]] ref = sel.center_of_geometry().reshape(1, 3).astype(np.float32) pairs = distances.capped_distance(ref, group.positions, self.cutoff, box=box, diff --git a/testsuite/MDAnalysisTests/core/test_atomselections.py b/testsuite/MDAnalysisTests/core/test_atomselections.py index 6d52ccf0a8f..8c04925e02d 100644 --- a/testsuite/MDAnalysisTests/core/test_atomselections.py +++ b/testsuite/MDAnalysisTests/core/test_atomselections.py @@ -730,6 +730,11 @@ def test_sphzone(self, u, periodic, expected): assert len(sel) == expected + @pytest.mark.parametrize('periodic,expected', ([True, 33], [False, 25])) + def test_sphzone(self, u, periodic, expected): + sel = u.select_atoms('sphzone 5.0 name NOT_A_NAME', periodic=periodic) + assert len(sel) == 0 + class TestTriclinicDistanceSelections(BaseDistanceSelection): @pytest.fixture() From 89a38d341d8e8d545f62ee963a34bfb8f1abd63d Mon Sep 17 00:00:00 2001 From: orioncohen Date: Fri, 2 Apr 2021 17:18:29 -0700 Subject: [PATCH 02/11] removed a unncesessary TODO statement --- package/MDAnalysis/core/selection.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package/MDAnalysis/core/selection.py b/package/MDAnalysis/core/selection.py index feeb2144396..31e536e6ede 100644 --- a/package/MDAnalysis/core/selection.py +++ b/package/MDAnalysis/core/selection.py @@ -375,7 +375,7 @@ def __init__(self, parser, tokens): self.sel = parser.parse_expression(self.precedence) @return_empty_on_apply - def apply(self, group): # TODO fix atom selection + def apply(self, group): indices = [] sel = self.sel.apply(group) box = self.validate_dimensions(group.dimensions) From a9194a3f6e0fca8365098724c37beef41078d0df Mon Sep 17 00:00:00 2001 From: orioncohen Date: Fri, 2 Apr 2021 17:22:04 -0700 Subject: [PATCH 03/11] added issue #2915 fix to CHANGELOG --- package/CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/package/CHANGELOG b/package/CHANGELOG index bf10ef51bc4..163d32afc9e 100644 --- a/package/CHANGELOG +++ b/package/CHANGELOG @@ -22,6 +22,7 @@ The rules for this file: * 2.0.0 Fixes + * Fixed 'sphzone' behavior to be consistent with 'around' (Issue #2915) * A Universe created from an ROMol with no atoms returns now a Universe with 0 atoms (Issue #3142) * ValueError raised when empty atomgroup is given to DensityAnalysis From dcb23b54eef029759d99f355b2a8b2f3843cdc35 Mon Sep 17 00:00:00 2001 From: orioncohen Date: Fri, 2 Apr 2021 21:52:19 -0700 Subject: [PATCH 04/11] added self to AUTHORS and CHANGELOG, moved lines in selection --- package/AUTHORS | 1 + package/CHANGELOG | 2 +- package/MDAnalysis/core/selection.py | 4 ++-- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/package/AUTHORS b/package/AUTHORS index 6a32bc9ce33..00076883aa4 100644 --- a/package/AUTHORS +++ b/package/AUTHORS @@ -157,6 +157,7 @@ Chronological list of authors - Leonardo Barneschi - Henrik Jäger - Jan Stevens + - Orion Cohen External code ------------- diff --git a/package/CHANGELOG b/package/CHANGELOG index 163d32afc9e..79ba91d1eed 100644 --- a/package/CHANGELOG +++ b/package/CHANGELOG @@ -17,7 +17,7 @@ The rules for this file: lilyminium, daveminh, jbarnoud, yuxuanzhuang, VOD555, ianmkenney, calcraven，xiki-tempula, mieczyslaw, manuel.nuno.melo, PicoCentauri, hanatok, rmeli, aditya-kamath, tirkarthi, LeonardoBarneschi, hejamu, - biogen98 + biogen98, orioncohen * 2.0.0 diff --git a/package/MDAnalysis/core/selection.py b/package/MDAnalysis/core/selection.py index 31e536e6ede..d0d0fc93bf3 100644 --- a/package/MDAnalysis/core/selection.py +++ b/package/MDAnalysis/core/selection.py @@ -378,10 +378,10 @@ def __init__(self, parser, tokens): def apply(self, group): indices = [] sel = self.sel.apply(group) - box = self.validate_dimensions(group.dimensions) - periodic = box is not None if len(sel) == 0: return group[[]] + box = self.validate_dimensions(group.dimensions) + periodic = box is not None ref = sel.center_of_geometry().reshape(1, 3).astype(np.float32) pairs = distances.capped_distance(ref, group.positions, self.cutoff, box=box, From 889b28d395717330e7d38f7e1bf87a3e0361edd0 Mon Sep 17 00:00:00 2001 From: orioncohen Date: Sat, 3 Apr 2021 08:13:06 -0700 Subject: [PATCH 05/11] fixed issue #2915 for cylayer, cyzone, and sphlayer. added testing to confirm. --- package/MDAnalysis/core/selection.py | 5 ++++- testsuite/MDAnalysisTests/core/test_atomselections.py | 7 +++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/package/MDAnalysis/core/selection.py b/package/MDAnalysis/core/selection.py index d0d0fc93bf3..93dc56e8f3a 100644 --- a/package/MDAnalysis/core/selection.py +++ b/package/MDAnalysis/core/selection.py @@ -352,6 +352,8 @@ def __init__(self, parser, tokens): def apply(self, group): indices = [] sel = self.sel.apply(group) + if len(sel) == 0: + return group[[]] box = self.validate_dimensions(group.dimensions) periodic = box is not None ref = sel.center_of_geometry().reshape(1, 3).astype(np.float32) @@ -396,7 +398,8 @@ class CylindricalSelection(Selection): @return_empty_on_apply def apply(self, group): sel = self.sel.apply(group) - + if len(sel) == 0: + return group[[]] # Calculate vectors between point of interest and our group vecs = group.positions - sel.center_of_geometry() diff --git a/testsuite/MDAnalysisTests/core/test_atomselections.py b/testsuite/MDAnalysisTests/core/test_atomselections.py index 8c04925e02d..b5120ac8328 100644 --- a/testsuite/MDAnalysisTests/core/test_atomselections.py +++ b/testsuite/MDAnalysisTests/core/test_atomselections.py @@ -209,6 +209,8 @@ def test_sphzone(self, universe, selstr): def test_cylayer(self, universe, selstr): sel = universe.select_atoms(selstr) assert_equal(len(sel), 88) + empty = universe.select_atoms('cylayer 4.0 6.0 10 -10 name NOT_A_NAME') + assert_equal(len(empty), 0) @pytest.mark.parametrize('selstr', [ 'cyzone 6.0 10 -10 bynum 1281', @@ -217,6 +219,8 @@ def test_cylayer(self, universe, selstr): def test_cyzone(self, universe, selstr): sel = universe.select_atoms(selstr) assert_equal(len(sel), 166) + empty = universe.select_atoms('cyzone 6.0 10 -10 name NOT_A_NAME') + assert_equal(len(empty), 0) def test_point(self, universe): ag = universe.select_atoms('point 5.0 5.0 5.0 3.5') @@ -775,6 +779,9 @@ def test_sphlayer(self, u): assert idx == set(ag.indices) + empty = u.select_atoms('sphlayer 2.4 6.0 name NOT_A_NAME') + assert len(empty) == 0 + def test_sphzone(self, u): r1 = u.select_atoms('resid 1') cog = r1.center_of_geometry().reshape(1, 3) From 96b63e3e47c9508753e06024f7928e1d4803a4a6 Mon Sep 17 00:00:00 2001 From: orioncohen Date: Sat, 3 Apr 2021 09:15:33 -0700 Subject: [PATCH 06/11] created decorator to test for empty selections. moved around testing to be more consistent. --- package/MDAnalysis/core/selection.py | 26 ++++++++++++------- .../core/test_atomselections.py | 8 +++--- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/package/MDAnalysis/core/selection.py b/package/MDAnalysis/core/selection.py index 93dc56e8f3a..f883229b547 100644 --- a/package/MDAnalysis/core/selection.py +++ b/package/MDAnalysis/core/selection.py @@ -217,6 +217,20 @@ def apply(self, group): return func(self, group) return apply +def return_empty_on_empty_selection(func): + """ + Decorator to return empty AtomGroups from the apply() function + if entire the selection is empty or a sub-expression is empty. + """ + @functools.wraps(func) + def apply(self, group): + if len(group) == 0: + return group + if len(self.sel.apply(group)) == 0: + return group[[]] + return func(self, group) + return apply + class _Selectionmeta(type): def __init__(cls, name, bases, classdict): type.__init__(type, name, bases, classdict) @@ -348,12 +362,10 @@ def __init__(self, parser, tokens): self.exRadius = float(tokens.popleft()) self.sel = parser.parse_expression(self.precedence) - @return_empty_on_apply + @return_empty_on_empty_selection def apply(self, group): indices = [] sel = self.sel.apply(group) - if len(sel) == 0: - return group[[]] box = self.validate_dimensions(group.dimensions) periodic = box is not None ref = sel.center_of_geometry().reshape(1, 3).astype(np.float32) @@ -376,12 +388,10 @@ def __init__(self, parser, tokens): self.cutoff = float(tokens.popleft()) self.sel = parser.parse_expression(self.precedence) - @return_empty_on_apply + @return_empty_on_empty_selection def apply(self, group): indices = [] sel = self.sel.apply(group) - if len(sel) == 0: - return group[[]] box = self.validate_dimensions(group.dimensions) periodic = box is not None ref = sel.center_of_geometry().reshape(1, 3).astype(np.float32) @@ -395,11 +405,9 @@ def apply(self, group): class CylindricalSelection(Selection): - @return_empty_on_apply + @return_empty_on_empty_selection def apply(self, group): sel = self.sel.apply(group) - if len(sel) == 0: - return group[[]] # Calculate vectors between point of interest and our group vecs = group.positions - sel.center_of_geometry() diff --git a/testsuite/MDAnalysisTests/core/test_atomselections.py b/testsuite/MDAnalysisTests/core/test_atomselections.py index b5120ac8328..0b8ba83d2b9 100644 --- a/testsuite/MDAnalysisTests/core/test_atomselections.py +++ b/testsuite/MDAnalysisTests/core/test_atomselections.py @@ -734,11 +734,6 @@ def test_sphzone(self, u, periodic, expected): assert len(sel) == expected - @pytest.mark.parametrize('periodic,expected', ([True, 33], [False, 25])) - def test_sphzone(self, u, periodic, expected): - sel = u.select_atoms('sphzone 5.0 name NOT_A_NAME', periodic=periodic) - assert len(sel) == 0 - class TestTriclinicDistanceSelections(BaseDistanceSelection): @pytest.fixture() @@ -793,6 +788,9 @@ def test_sphzone(self, u): assert idx == set(ag.indices) + empty = u.select_atoms('sphzone 5.0 name NOT_A_NAME') + assert len(empty) == 0 + def test_point_1(self, u): # The example selection ag = u.select_atoms('point 5.0 5.0 5.0 3.5') From d5fde7d90c3f7cc7dff2917bb257b71ba76ddba2 Mon Sep 17 00:00:00 2001 From: orioncohen Date: Sat, 3 Apr 2021 09:20:15 -0700 Subject: [PATCH 07/11] added another blank line to be consistent with PEP-8 --- package/MDAnalysis/core/selection.py | 1 + 1 file changed, 1 insertion(+) diff --git a/package/MDAnalysis/core/selection.py b/package/MDAnalysis/core/selection.py index f883229b547..756d7c3ebf6 100644 --- a/package/MDAnalysis/core/selection.py +++ b/package/MDAnalysis/core/selection.py @@ -217,6 +217,7 @@ def apply(self, group): return func(self, group) return apply + def return_empty_on_empty_selection(func): """ Decorator to return empty AtomGroups from the apply() function From 8a81f5485d7709b439a0de1d60749be8b6740c59 Mon Sep 17 00:00:00 2001 From: orioncohen Date: Sat, 3 Apr 2021 17:06:40 -0700 Subject: [PATCH 08/11] seperated testing for empty atom selection for sph* and cy* selection tokens --- testsuite/MDAnalysisTests/core/test_atomselections.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/testsuite/MDAnalysisTests/core/test_atomselections.py b/testsuite/MDAnalysisTests/core/test_atomselections.py index 0b8ba83d2b9..eb02538e7b0 100644 --- a/testsuite/MDAnalysisTests/core/test_atomselections.py +++ b/testsuite/MDAnalysisTests/core/test_atomselections.py @@ -209,6 +209,8 @@ def test_sphzone(self, universe, selstr): def test_cylayer(self, universe, selstr): sel = universe.select_atoms(selstr) assert_equal(len(sel), 88) + + def test_empty_cylayer(self, universe): empty = universe.select_atoms('cylayer 4.0 6.0 10 -10 name NOT_A_NAME') assert_equal(len(empty), 0) @@ -219,6 +221,8 @@ def test_cylayer(self, universe, selstr): def test_cyzone(self, universe, selstr): sel = universe.select_atoms(selstr) assert_equal(len(sel), 166) + + def test_empty_cyzone(self, universe): empty = universe.select_atoms('cyzone 6.0 10 -10 name NOT_A_NAME') assert_equal(len(empty), 0) @@ -774,6 +778,7 @@ def test_sphlayer(self, u): assert idx == set(ag.indices) + def test_empty_sphlayer(self, u): empty = u.select_atoms('sphlayer 2.4 6.0 name NOT_A_NAME') assert len(empty) == 0 @@ -788,6 +793,7 @@ def test_sphzone(self, u): assert idx == set(ag.indices) + def test_empty_sphzone(self, u): empty = u.select_atoms('sphzone 5.0 name NOT_A_NAME') assert len(empty) == 0 From a4c7e41ea868f710b3e25005d1f25c26458d09c3 Mon Sep 17 00:00:00 2001 From: orioncohen Date: Sat, 3 Apr 2021 17:14:55 -0700 Subject: [PATCH 09/11] added stacked decorators instead of repeated functionality in one decorator --- package/MDAnalysis/core/selection.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/package/MDAnalysis/core/selection.py b/package/MDAnalysis/core/selection.py index 756d7c3ebf6..7035743fe50 100644 --- a/package/MDAnalysis/core/selection.py +++ b/package/MDAnalysis/core/selection.py @@ -225,8 +225,6 @@ def return_empty_on_empty_selection(func): """ @functools.wraps(func) def apply(self, group): - if len(group) == 0: - return group if len(self.sel.apply(group)) == 0: return group[[]] return func(self, group) @@ -363,6 +361,7 @@ def __init__(self, parser, tokens): self.exRadius = float(tokens.popleft()) self.sel = parser.parse_expression(self.precedence) + @return_empty_on_apply @return_empty_on_empty_selection def apply(self, group): indices = [] @@ -389,6 +388,7 @@ def __init__(self, parser, tokens): self.cutoff = float(tokens.popleft()) self.sel = parser.parse_expression(self.precedence) + @return_empty_on_apply @return_empty_on_empty_selection def apply(self, group): indices = [] @@ -406,6 +406,7 @@ def apply(self, group): class CylindricalSelection(Selection): + @return_empty_on_apply @return_empty_on_empty_selection def apply(self, group): sel = self.sel.apply(group) From b6dfd9a64a12c97ceaa525e95cf73ae3f8da8151 Mon Sep 17 00:00:00 2001 From: orioncohen Date: Sun, 4 Apr 2021 12:24:43 -0700 Subject: [PATCH 10/11] removed the decorator implementation and added code directly into the functions --- package/MDAnalysis/core/selection.py | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/package/MDAnalysis/core/selection.py b/package/MDAnalysis/core/selection.py index 7035743fe50..d72ec4f6c03 100644 --- a/package/MDAnalysis/core/selection.py +++ b/package/MDAnalysis/core/selection.py @@ -218,18 +218,6 @@ def apply(self, group): return apply -def return_empty_on_empty_selection(func): - """ - Decorator to return empty AtomGroups from the apply() function - if entire the selection is empty or a sub-expression is empty. - """ - @functools.wraps(func) - def apply(self, group): - if len(self.sel.apply(group)) == 0: - return group[[]] - return func(self, group) - return apply - class _Selectionmeta(type): def __init__(cls, name, bases, classdict): type.__init__(type, name, bases, classdict) @@ -362,10 +350,11 @@ def __init__(self, parser, tokens): self.sel = parser.parse_expression(self.precedence) @return_empty_on_apply - @return_empty_on_empty_selection def apply(self, group): indices = [] sel = self.sel.apply(group) + if len(sel) == 0: + return group[[]] box = self.validate_dimensions(group.dimensions) periodic = box is not None ref = sel.center_of_geometry().reshape(1, 3).astype(np.float32) @@ -389,10 +378,11 @@ def __init__(self, parser, tokens): self.sel = parser.parse_expression(self.precedence) @return_empty_on_apply - @return_empty_on_empty_selection def apply(self, group): indices = [] sel = self.sel.apply(group) + if len(sel) == 0: + return group[[]] box = self.validate_dimensions(group.dimensions) periodic = box is not None ref = sel.center_of_geometry().reshape(1, 3).astype(np.float32) @@ -407,9 +397,10 @@ def apply(self, group): class CylindricalSelection(Selection): @return_empty_on_apply - @return_empty_on_empty_selection def apply(self, group): sel = self.sel.apply(group) + if len(sel) == 0: + return group[[]] # Calculate vectors between point of interest and our group vecs = group.positions - sel.center_of_geometry() From 0cc38ad9c51c9b50a482c91c9054f6386db72157 Mon Sep 17 00:00:00 2001 From: orioncohen Date: Mon, 5 Apr 2021 12:46:56 -0700 Subject: [PATCH 11/11] updated CHANGELOG with more detail --- package/CHANGELOG | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/package/CHANGELOG b/package/CHANGELOG index 79ba91d1eed..0de5b4a35a4 100644 --- a/package/CHANGELOG +++ b/package/CHANGELOG @@ -22,7 +22,8 @@ The rules for this file: * 2.0.0 Fixes - * Fixed 'sphzone' behavior to be consistent with 'around' (Issue #2915) + * Fixed 'sphzone', 'sphlayer', 'cyzone', and 'cylayer' to return empty if the + zone/layer is empty, consistent with 'around' (Issue #2915) * A Universe created from an ROMol with no atoms returns now a Universe with 0 atoms (Issue #3142) * ValueError raised when empty atomgroup is given to DensityAnalysis