Fix XTCReader and TRRReader frame navigation when sub is set - #5308
Open
xovishnukosuri wants to merge 1 commit into
Open
xovishnukosuri wants to merge 1 commit into
xovishnukosuri wants to merge 1 commit into
Conversation
When XTCReader or TRRReader is initialized with the sub keyword, _read_next_timestep used read_direct_x / read_direct_xvf and passed ts.positions (sized len(sub)) as the write buffer. The underlying C call wrote n_atoms values into that smaller buffer, corrupting memory or raising a TypeError on the second and later frames. The fix skips the direct-read path when _sub is not None and falls back to the regular read(), which allocates a full-atom array. _frame_to_ts then correctly indexes frame.x with self._sub (a numpy array) to fill ts.positions. A new test covers direct frame indexing across all frames with sub active. Fixes MDAnalysis#4116 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4116
Changes
When
XTCReaderorTRRReaderis initialized with thesubkeyword,_read_next_timestepcalledread_direct_x/read_direct_xvfand passedts.positions(sizedlen(sub)) as the write buffer. The underlying Cread_xtc/read_trrfunction wroten_atomsvalues into that smaller buffer, causing memory corruption or aTypeErrorwhen navigating to any frame beyond frame 0.Root cause:
read_direct_xandread_direct_xvfuse the passed array as a Cython typed memview (cnp.float32_t[:, ::1]). Fancy numpy indexing does not work on typed memviews, and the buffer size does not matchn_atoms.Fix: Skip the direct-read optimization when
self._sub is not None. Use the regularread()instead, which allocates a full-atom numpy ndarray._frame_to_tsthen indexesframe.x[self._sub]with a regular numpy fancy index, which works correctly.The fast
read_direct_x/read_direct_xvfpath is preserved for the normal (non-sub) case.Added test:
test_sub_frame_navigationiterates over all frames using direct index access (trajectory[i]) withsubactive and compares positions against the reference solvated universe. The existingtest_sub_coordinatesonly checked frame 0 and did not catch the regression.LLM / AI generated code disclosure
LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: yes
(Claude by Anthropic was used to help identify and structure the fix.)
PR Checklist
package/CHANGELOGfile updated?package/AUTHORS?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.