Skip to content

Conversation

@hejamu
Copy link
Contributor

@hejamu hejamu commented Feb 5, 2026

Changes made in this Pull Request:

  • Changed the wrap method to use index mapping instead of a loop.

The method was significantly slower than expected when wrapping compounds (residues, fragments, molecules, segments). See the corresponding issue on the MAICoS repo.

It just never got the same performance increases as for example unwrap. I considered using the _split_by_compound_indices approach from unwrap, but that is actually not needed for wrapping. Simply calculating the shifts and applying them in one go yields an enormous speed up. Most the performance penalty actually came from the np.where done in each loop I believe.

Here is a scaling plot comparing the speeds:

output

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.

@codecov
Copy link

codecov bot commented Feb 5, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.82%. Comparing base (e6c1b44) to head (ddf80d9).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #5226      +/-   ##
===========================================
- Coverage    93.83%   93.82%   -0.01%     
===========================================
  Files          180      180              
  Lines        22475    22473       -2     
  Branches      3190     3189       -1     
===========================================
- Hits         21090    21086       -4     
- Misses         923      924       +1     
- Partials       462      463       +1     

☔ 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.

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

This looks good but I didn't have time to dive into it. Can you confirm that it produces the same results? I know that the tests covered it but did you look if the tests for it actually likely to find any disagreement?

@hejamu
Copy link
Contributor Author

hejamu commented Feb 9, 2026

The tests are already quite comprehensive. I only noticed that the tests do not check whether it also works with non-contiguous compound indexes. Since the current code and the PR use fancy-indexing, I added such a test.

@PardhavMaradani
Copy link
Contributor

PardhavMaradani commented Feb 9, 2026

Linking the performance gains here to the ones in an older open PR #2982 (relevant changes to groups.py) to note that this PR would address this aspect of that old PR for future reference. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants