-
Notifications
You must be signed in to change notification settings - Fork 781
Fix GSDReader TypeError when indexing with NumPy scalar integers #5233
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
Conversation
…; add test for GSDReader numpy int indexing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a TypeError that occurred when indexing GSD trajectory files with NumPy scalar integers (e.g., np.int64). The issue manifested when using Analysis classes with HOOMD GSD trajectories because MDAnalysis's parallelization framework generates frame indices as numpy arrays with dtype np.int64, but the underlying GSD library's HOOMDTrajectory only accepts Python int types for indexing.
Changes:
- Added conversion of numpy integer types to Python int in
GSDReader._read_frame()before indexing the GSD file - Added regression test covering
np.int64,np.int32, and negative indexing with numpy integers - Updated AUTHORS file with contributor name
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| package/MDAnalysis/coordinates/GSD.py | Added int(frame) conversion in _read_frame() to handle numpy scalar integers before accessing the underlying GSD file object |
| testsuite/MDAnalysisTests/coordinates/test_gsd.py | Added comprehensive regression test for numpy integer indexing including negative indices and multiple numpy integer types |
| package/AUTHORS | Added Suriya Sureshkumar to the 2026 contributors list |
| .gitignore | Added venv/ to ignore virtual environment directories |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5233 +/- ##
===========================================
- Coverage 93.83% 93.82% -0.02%
===========================================
Files 180 180
Lines 22475 22476 +1
Branches 3190 3190
===========================================
- Hits 21090 21088 -2
- Misses 923 925 +2
- Partials 462 463 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@orbeckst Can you please review my PR? |
|
Currently there are multiple glaring issues with this PR which would prevent us from reviewing / potentially merging.
Can you please explain the motivation behind removing unrelated tests? |
Thanks for pointing these out. The removal of the unrelated test was a complete mistake. I’ll revert that immediately. The .gitignore change is also unrelated to this issue and will be removed. |
|
Closing this PR to clean up the branch and resubmit a minimal fix focused only on the NumPy scalar indexing issue in GSDReader. |
Fixes #5224
Changes made in this Pull Request:
LLM / AI generated code disclosure
LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: no
PR Checklist
package/CHANGELOGfile updated?package/AUTHORS? (If it is not, add it!)Developers Certificate of Origin
I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.
📚 Documentation preview 📚: https://mdanalysis--5233.org.readthedocs.build/en/5233/