Skip to content

Conversation

DrDomenicoMarson
Copy link
Contributor

@DrDomenicoMarson DrDomenicoMarson commented Sep 11, 2025

Fixes #5112

Problem:

sort_backbone can hang indefinitely when the input AtomGroup is disconnected (e.g., some backbone atoms were excluded by selection). Currently, the function only checks n_fragments, which is insufficient to catch all cases of disconnected backbones.

Cause:

  1. The reliance on n_fragments alone does not reliably detect disconnected selections.
  2. The while loop in sort_backbone iterates until the backbone is fully traversed, but if the selection is disconnected, the loop can never complete.

Solution / Changes made:

  1. Added robust validation before the traversal, checking:

    • Each atom’s connectivity degree: 1 for caps, 2 for internal atoms
    • No atom has a degree outside {1, 2}
    • Exactly two cap atoms exist
    • Added final sanity check after traversal (ensure the sorted backbone has the same length as the input)
    • All error messages now report the problematic atoms to help with debugging.
  2. Replaced the “dangerous” unbounded while loop with a bounded iteration over the backbone length.

Tested with backbone selections missing internal atoms, that now raises a descriptive ValueError instead of hanging.

PR Checklist

  • Issue raised/referenced?
  • Tests updated/added?
  • Documentation updated/added? No change needed?
  • package/CHANGELOG file updated?
  • Is your name in 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--5113.org.readthedocs.build/en/5113/

Copy link

codecov bot commented Sep 11, 2025

Codecov Report

❌ Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.86%. Comparing base (5d48c5c) to head (b26705c).

Files with missing lines Patch % Lines
package/MDAnalysis/analysis/polymer.py 80.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #5113      +/-   ##
===========================================
- Coverage    93.86%   93.86%   -0.01%     
===========================================
  Files          179      179              
  Lines        22249    22250       +1     
  Branches      3161     3161              
===========================================
- Hits         20885    20884       -1     
- Misses         902      903       +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.

u.add_TopologyAttr(Bonds(bondlist))
with pytest.raises(ValueError) as ex:
polymer.sort_backbone(u.atoms)
assert "Backbone connectivity invalid" in str(ex.value)
Copy link
Member

Choose a reason for hiding this comment

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

When writing new tests it might be preferable to use the built-in match=... argument of pytest.raises() to check the error message that way. It is also borderline if we should just parametrize this test with the one above it--perhaps they're different enough not to bother.

bad_ag = u.atoms[:10] # include -H etc

with pytest.raises(ValueError):
with pytest.raises(ValueError) as ex:
Copy link
Member

Choose a reason for hiding this comment

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

maybe just match=... here too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sort_backbone hangs when backbone selection is disconnected
2 participants