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

Fixed high dimensional GroupBase indexing. #4656

Merged
merged 3 commits into from
Aug 22, 2024
Merged
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
2 changes: 1 addition & 1 deletion package/AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ Chronological list of authors
- Kurt McKee
- Fabian Zills
- Laksh Krishna Sharma

- Matthew Davies


External code
Expand Down
1 change: 1 addition & 0 deletions package/CHANGELOG
Original file line number Diff line number Diff line change
Expand Up @@ -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/<particle>/<property>` (part of Issue #4598, PR #4615)
* Fix failure in double-serialization of TextIOPicklable file reader.
Expand Down
10 changes: 10 additions & 0 deletions package/MDAnalysis/core/groups.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
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')

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure (?) we have to allow ndim>1, so we can probably just raise an Error inside this branch without the np.squeeze step.

Is there a code path that currently uses the np.arange(5).reshape(-1, 1) route?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I can tell. The only tests that fail with the sanitisation removed are the newly added tests, to test the sanitisation is working.

Copy link
Member

Choose a reason for hiding this comment

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

Ok great then, I couldn't remember if something like Residue access was going via this route.

# We specify _derived_class instead of self.__class__ to allow
# subclasses, such as UpdatingAtomGroup, to control the class
# resulting from slicing.
Expand Down
19 changes: 19 additions & 0 deletions testsuite/MDAnalysisTests/core/test_atomgroup.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
Loading