Fixed high dimensional GroupBase indexing. #4656
Conversation
|
Hello @MattTDavies! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2024-08-21 16:33:21 UTC |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on GitHub Discussions so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS as part of this PR.
Linter Bot Results:Hi @MattTDavies! Thanks for making this PR. We linted your code and found the following: Some issues were found with the formatting of your code.
Please have a look at the Please note: The |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4656 +/- ##
===========================================
- Coverage 93.61% 93.59% -0.03%
===========================================
Files 171 185 +14
Lines 21254 22487 +1233
Branches 3937 3979 +42
===========================================
+ Hits 19897 21046 +1149
- Misses 898 977 +79
- Partials 459 464 +5 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
package/MDAnalysis/core/groups.py
Outdated
| 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') | ||
|
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok great then, I couldn't remember if something like Residue access was going via this route.
Fixes #4647
This does not fix the underlying "Frankenatoms" issue, but stops it from occuring due to GroupBase indexing by raising an error if improperly shaped indices are provided.
Changes made in this Pull Request:
PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4656.org.readthedocs.build/en/4656/