Fix/gsd numpy int64 indexing issue 5224

[kunjsinha]

Fixes #5224

Changes made in this Pull Request:

• Fixed TypeError when indexing GSDReader trajectory with np.int64
• Added minimal cast frame = int(frame) in GSDReader._read_frame
• Added test test_gsd_indexing_with_numpy_int in test_gsd.py

LLM / AI generated code disclosure

LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: no

PR Checklist

• Issue raised/referenced?
• Tests updated/added?
• Documentation updated/added?
• package/CHANGELOG file updated?
• Is your name in package/AUTHORS? (If it is not, add it!)
• LLM/AI disclosure was updated.

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--5260.org.readthedocs.build/en/5260/

[github-actions]

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.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.82%. Comparing base (1eaf2e7) to head (5f75ad2).
⚠️ Report is 2 commits behind head on develop.

Files with missing lines	Patch %	Lines
package/MDAnalysis/coordinates/GSD.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #5260      +/-   ##
===========================================
- Coverage    93.82%   93.82%   -0.01%     
===========================================
  Files          180      180              
  Lines        22476    22477       +1     
  Branches      3191     3191              
===========================================
  Hits         21089    21089              
- Misses         924      925       +1     
  Partials       463      463              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jeremyleung521]

Just nitpicky stuff but looks good to me.

[jeremyleung521]

import numpy should probably be imported above import pytest.

[jeremyleung521]

Actually I'm having doubts, sorry. The code coverage is claiming there is 0% coverage, which means the _read_frame method might not be called in the new test using []. Might be worth double checking that. It's a nested funnel of subclasses so it's a little difficult to track.

having second doubts due to the code coverage failure

[kunjsinha]

Hi @jeremyleung521 ,
Thanks for the review. I've moved the numpy import above pytest as you suggested. Regarding the 0% coverage, I had tried a few more ways on getting that coverage to hit (as you can see from my line of commits) but it didn't work. After which I looked into the CI logs and found that gsd is disabled intentionally across CI jobs. I think it was done in this PR #4707. Since gsd is never installed, HAS_GSD evaluates to False and the entire TestGSDReader class gets skipped. I've verified locally with gsd installed that the test runs and passes and that the int() type cast correctly handles the np.int64 index. Let me know if i'm wrong or if there is anything you would like me to change.

[jeremyleung521]

Hi @jeremyleung521 , Thanks for the review. I've moved the numpy import above pytest as you suggested. Regarding the 0% coverage, I had tried a few more ways on getting that coverage to hit (as you can see from my line of commits) but it didn't work. After which I looked into the CI logs and found that gsd is disabled intentionally across CI jobs. I think it was done in this PR #4707. Since gsd is never installed, HAS_GSD evaluates to False and the entire TestGSDReader class gets skipped. I've verified locally with gsd installed that the test runs and passes and that the int() type cast correctly handles the np.int64 index. Let me know if i'm wrong or if there is anything you would like me to change.

Makes sense! Thanks for testing/looking through it. I'll ping @yuxuanzhuang just in case since he removed the GSD tests but LGTM otherwise now!

[orbeckst]

@yuxuanzhuang @IAlibay did we remove GSD from standard CI and only run in CRON CI?

[orbeckst]

@kunjsinha can you please run your newly added test locally in an environment that contains gsd? Also run it with coverage.

Show how you've been running the test and the output that demonstrates

• test fails WITHOUT your fix
• test passes with your fix
• coverage of your code

(look at pytest and coverage to learn how to do these things)

I'd basically like to see evidence that the tests work when GSD is present, even though we cannot currently verify this in CI. Thank you!

[kunjsinha]

@orbeckst I've ran everything locally with gsd==4.2.0 installed. Here is the evidence:

Without the fix (commented out frame = int(frame)):

collected 7 items / 6 deselected / 1 selected                                                                                                                                                                                                                                                                         

testsuite/MDAnalysisTests/coordinates/test_gsd.py::TestGSDReader::test_gsd_indexing_with_numpy_int FAILED                                                                                                                                                                                                       [100%]

====================================================================================================================================================== FAILURES =======================================================================================================================================================
___________________________________________________________________________________________________________________________________ TestGSDReader.test_gsd_indexing_with_numpy_int ____________________________________________________________________________________________________________________________________
testsuite/MDAnalysisTests/coordinates/test_gsd.py:80: in test_gsd_indexing_with_numpy_int
    ts = traj[np.int64(0)]
         ^^^^^^^^^^^^^^^^^
../venv/lib/python3.12/site-packages/MDAnalysis/coordinates/base.py:838: in __getitem__
    return self._read_frame_with_aux(frame)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
../venv/lib/python3.12/site-packages/MDAnalysis/coordinates/base.py:870: in _read_frame_with_aux
    ts = self._read_frame(frame)  # pylint: disable=assignment-from-no-return
         ^^^^^^^^^^^^^^^^^^^^^^^
../venv/lib/python3.12/site-packages/MDAnalysis/coordinates/GSD.py:133: in _read_frame
    myframe = self._file[frame]
              ^^^^^^^^^^^^^^^^^
../venv/lib/python3.12/site-packages/gsd/hoomd.py:1046: in __getitem__
    raise TypeError
E   TypeError
=============================================================================================================================================== short test summary info ===============================================================================================================================================
FAILED testsuite/MDAnalysisTests/coordinates/test_gsd.py::TestGSDReader::test_gsd_indexing_with_numpy_int - TypeError
=========================================================================================================================================== 1 failed, 6 deselected in 0.35s =========================================================================================================================================== 

---

With the fix:

collected 7 items / 6 deselected / 1 selected                                                                                                                                                                                                                                                                         

testsuite/MDAnalysisTests/coordinates/test_gsd.py::TestGSDReader::test_gsd_indexing_with_numpy_int PASSED                                                                                                                                                                                                       [100%]

=========================================================================================================================================== 1 passed, 6 deselected in 0.31s =========================================================================================================================================== 

I used the command pytest testsuite/MDAnalysisTests/coordinates/test_gsd.py -v -k "test_gsd_indexing_with_numpy_int" --tb=short

For the coverage, i ran python -m pytest testsuite/MDAnalysisTests/coordinates/test_gsd.py --cov=MDAnalysis.coordinates.GSD --cov-report=html -v. Upon opening GSD.py in the resulting htmlcov/index.html file, frame = int(frame) is highlighted with green, confirming that the test covers the fix.

[IAlibay]

@yuxuanzhuang @IAlibay did we remove GSD from standard CI and only run in CRON CI?

Yeah, gsd caused our tests to fail randomly.

[orbeckst]

lgtm, thanks for doing the manual test