Robust sort_backbone: detect disconnected selections and avoid infinite loops#5113
Robust sort_backbone: detect disconnected selections and avoid infinite loops#5113orbeckst merged 6 commits intoMDAnalysis:developfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5113 +/- ##
===========================================
+ Coverage 93.86% 93.88% +0.02%
===========================================
Files 179 180 +1
Lines 22249 22421 +172
Branches 3161 3185 +24
===========================================
+ Hits 20885 21051 +166
Misses 902 902
- Partials 462 468 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
orbeckst
left a comment
There was a problem hiding this comment.
Thank you for your fix @DrDomenicoMarson ! Please address @tylerjereddy 's comments and my minor ones.
| if len(sorted_backbone) != len(backbone): | ||
| raise ValueError( | ||
| "Backbone traversal did not visit all atoms. " | ||
| "Expected {} atoms, got {}.".format( | ||
| len(backbone), len(sorted_backbone) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Not covered by tests – could you please add a test for it?
There was a problem hiding this comment.
I wrote this as a final sanity check, but when I tested the code, I realized that I can’t reach this part of the code without having the analysis fail earlier (which is what it should do, considering the situation). So, I decided to remove this “untestable and useless” part.
orbeckst
left a comment
There was a problem hiding this comment.
LGTM, thank you!
Will get into release 2.10.0.
Fixes #5112
Problem:
sort_backbonecan hang indefinitely when the inputAtomGroupis disconnected (e.g., some backbone atoms were excluded by selection). Currently, the function only checksn_fragments, which is insufficient to catch all cases of disconnected backbones.Cause:
n_fragmentsalone does not reliably detect disconnected selections.whileloop insort_backboneiterates until the backbone is fully traversed, but if the selection is disconnected, the loop can never complete.Solution / Changes made:
Added robust validation before the traversal, checking:
Replaced the “dangerous” unbounded
whileloop 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
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--5113.org.readthedocs.build/en/5113/