From d6295275341a7e7c02a8551bd1e456e79144af20 Mon Sep 17 00:00:00 2001 From: Matthew Davies <128810112+MattTDavies@users.noreply.github.com> Date: Thu, 15 Aug 2024 09:42:21 -0400 Subject: [PATCH 1/3] Fixed high dimensional GroupBase indexing. --- package/AUTHORS | 2 +- package/CHANGELOG | 1 + package/MDAnalysis/core/groups.py | 10 ++++++++++ .../MDAnalysisTests/core/test_atomgroup.py | 19 +++++++++++++++++++ 4 files changed, 31 insertions(+), 1 deletion(-) diff --git a/package/AUTHORS b/package/AUTHORS index 52e17f7c20d..9728e7ac531 100644 --- a/package/AUTHORS +++ b/package/AUTHORS @@ -243,7 +243,7 @@ Chronological list of authors - Kurt McKee - Fabian Zills - Laksh Krishna Sharma - + - Matthew Davies External code diff --git a/package/CHANGELOG b/package/CHANGELOG index 750ddcf1cba..ceba46e3fa5 100644 --- a/package/CHANGELOG +++ b/package/CHANGELOG @@ -22,6 +22,7 @@ The rules for this file: * 2.8.0 Fixes + * Sanitise and catch higher dimensional indexing in GroupBase (Issue #4647) * Do not raise an Error reading H5MD files with datasets like `observables//` (part of Issue #4598, PR #4615) * Fix failure in double-serialization of TextIOPicklable file reader. diff --git a/package/MDAnalysis/core/groups.py b/package/MDAnalysis/core/groups.py index c272f96f1e4..4f541d0bb3d 100644 --- a/package/MDAnalysis/core/groups.py +++ b/package/MDAnalysis/core/groups.py @@ -597,6 +597,16 @@ def __getitem__(self, item): # hack to make lists into numpy arrays # important for boolean slicing item = np.array(item) + + # correctly flatten oddly shaped, but 1d arrays + # raises issues if flattening already suitable arrays + if isinstance(item, np.ndarray) and item.ndim > 1: # ignore slices + item = np.squeeze(item) + # disallow high dimensional indexing. + # this doesnt stop the underlying issue + if item.ndim > 1: + raise IndexError('Group index must be 1d') + # We specify _derived_class instead of self.__class__ to allow # subclasses, such as UpdatingAtomGroup, to control the class # resulting from slicing. diff --git a/testsuite/MDAnalysisTests/core/test_atomgroup.py b/testsuite/MDAnalysisTests/core/test_atomgroup.py index 6173ea76f5c..8a3f4625cfb 100644 --- a/testsuite/MDAnalysisTests/core/test_atomgroup.py +++ b/testsuite/MDAnalysisTests/core/test_atomgroup.py @@ -1592,6 +1592,25 @@ def test_index_advancedslice(self, universe): "an AtomGroup") assert_equal(ag[1], ag[-1], "advanced slicing does not preserve order") + @pytest.mark.parametrize('shape', [(-1, 1), (1, -1)]) + def test_1d_index_invariance(self, universe, shape): + u = universe + ag1 = u.atoms[np.arange(5)] + ag2 = u.atoms[np.arange(5).reshape(shape)] + + # without correct sanitising, n_atoms is 1 when reshaped 'incorrectly' + assert ag2.n_atoms == 5, ("indexing with a 1d array was not " + "invarient of row vs column order") + assert_equal(ag1, ag2), ("indexing with a 1d array was not " + "invarient of row vs column order") + + def test_2d_indexing_caught(self, universe): + u = universe + index_2d = [[1, 2, 3], + [4, 5, 6]] + with pytest.raises(IndexError): + u.atoms[index_2d] + @pytest.mark.parametrize('sel', (np.array([True, False, True]), [True, False, True])) def test_boolean_indexing_2(self, universe, sel): From 5ab605aad50ac051f25aa1ccfd80029c4fbb1e1b Mon Sep 17 00:00:00 2001 From: Matthew Davies <128810112+MattTDavies@users.noreply.github.com> Date: Thu, 15 Aug 2024 11:26:49 -0400 Subject: [PATCH 2/3] fixed pep8 issues --- package/MDAnalysis/core/groups.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/package/MDAnalysis/core/groups.py b/package/MDAnalysis/core/groups.py index 4f541d0bb3d..f8c2444ad03 100644 --- a/package/MDAnalysis/core/groups.py +++ b/package/MDAnalysis/core/groups.py @@ -597,10 +597,10 @@ def __getitem__(self, item): # hack to make lists into numpy arrays # important for boolean slicing item = np.array(item) - + # correctly flatten oddly shaped, but 1d arrays # raises issues if flattening already suitable arrays - if isinstance(item, np.ndarray) and item.ndim > 1: # ignore slices + if isinstance(item, np.ndarray) and item.ndim > 1: item = np.squeeze(item) # disallow high dimensional indexing. # this doesnt stop the underlying issue From 9b83ae6bad7f4b425af0512a9129600e0b6a6568 Mon Sep 17 00:00:00 2001 From: Matthew Davies <128810112+MattTDavies@users.noreply.github.com> Date: Wed, 21 Aug 2024 12:31:02 -0400 Subject: [PATCH 3/3] Removed sanitisation --- package/CHANGELOG | 2 +- package/MDAnalysis/core/groups.py | 6 +----- testsuite/MDAnalysisTests/core/test_atomgroup.py | 12 ------------ 3 files changed, 2 insertions(+), 18 deletions(-) diff --git a/package/CHANGELOG b/package/CHANGELOG index ceba46e3fa5..14fcc7648b8 100644 --- a/package/CHANGELOG +++ b/package/CHANGELOG @@ -22,7 +22,7 @@ The rules for this file: * 2.8.0 Fixes - * Sanitise and catch higher dimensional indexing in GroupBase (Issue #4647) + * Catch higher dimensional indexing in GroupBase (Issue #4647) * Do not raise an Error reading H5MD files with datasets like `observables//` (part of Issue #4598, PR #4615) * Fix failure in double-serialization of TextIOPicklable file reader. diff --git a/package/MDAnalysis/core/groups.py b/package/MDAnalysis/core/groups.py index f8c2444ad03..e8304d3b042 100644 --- a/package/MDAnalysis/core/groups.py +++ b/package/MDAnalysis/core/groups.py @@ -598,14 +598,10 @@ def __getitem__(self, item): # important for boolean slicing item = np.array(item) - # correctly flatten oddly shaped, but 1d arrays - # raises issues if flattening already suitable arrays if isinstance(item, np.ndarray) and item.ndim > 1: - item = np.squeeze(item) # disallow high dimensional indexing. # this doesnt stop the underlying issue - if item.ndim > 1: - raise IndexError('Group index must be 1d') + raise IndexError('Group index must be 1d') # We specify _derived_class instead of self.__class__ to allow # subclasses, such as UpdatingAtomGroup, to control the class diff --git a/testsuite/MDAnalysisTests/core/test_atomgroup.py b/testsuite/MDAnalysisTests/core/test_atomgroup.py index 8a3f4625cfb..1497456e2da 100644 --- a/testsuite/MDAnalysisTests/core/test_atomgroup.py +++ b/testsuite/MDAnalysisTests/core/test_atomgroup.py @@ -1592,18 +1592,6 @@ def test_index_advancedslice(self, universe): "an AtomGroup") assert_equal(ag[1], ag[-1], "advanced slicing does not preserve order") - @pytest.mark.parametrize('shape', [(-1, 1), (1, -1)]) - def test_1d_index_invariance(self, universe, shape): - u = universe - ag1 = u.atoms[np.arange(5)] - ag2 = u.atoms[np.arange(5).reshape(shape)] - - # without correct sanitising, n_atoms is 1 when reshaped 'incorrectly' - assert ag2.n_atoms == 5, ("indexing with a 1d array was not " - "invarient of row vs column order") - assert_equal(ag1, ag2), ("indexing with a 1d array was not " - "invarient of row vs column order") - def test_2d_indexing_caught(self, universe): u = universe index_2d = [[1, 2, 3],