Fix off-by-one loops + self-distance bug in SEWING/splice/de novo/RNA (split 3/4 of #707) - #732
Open
lyskov-ai wants to merge 1 commit into
Open
Conversation
… RNA) Split from RosettaCommons#707. Two related fixes in structural-assembly / design code. Off-by-one in 1-indexed loops (< -> <=), last element was silently skipped: - legacy_sewing/conformation/Assembly.cc x2 (pose_loop_anchors, disconnected_segments -- the last segment, which by definition has no next, was skipped, exactly the case '!has_next(i)' is meant to catch) - pose_sewing/movers/BlockwiseAnalysisMover.cc - pose_sewing/movers/OmnibusDisulfideAnalysisLabelerMover.cc - splice/SampleRotamersFromPDB.cc x2 (rotamer-deduplication scan) - splice/SpliceManager.cc (PSSM row check -- 'go over all the PSSM segments') - stepwise/legacy/modeler/rna/StepWiseRNA_WorkingParametersSetup.cc - rna/movers/RNAIdealizeMover.cc (CoordinateConstraint application) - denovo_design/components/FoldGraph.cc - fldsgn/MatchResidues.cc x2 (RMSD/superimpose atom-id map build) - features/strand_assembly/StrandBundleFeatures.cc x2 (per-atom distance scan) Self-distance copy/paste bug in the two pose_sewing movers: 'pose.residue(upstream_res).xyz(2).distance(pose.residue(upstream_res).xyz(2))' is identically 0, so the '<= crit_dist_' gate was always satisfied and the distance filter was a no-op. Corrected the second operand to downstream_res. Note: the pose_sewing regression test output changes with this batch; the change is intentional and warrants scientific sign-off before merge.
roccomoretti
approved these changes
Jul 6, 2026
roccomoretti
left a comment
roccomoretti
left a comment
Member
There was a problem hiding this comment.
Looks fine, pending updates to the OmnibusDisulfideAnalysisLabelerMover unit test.
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
2 participants
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.
Summary
One of four PRs splitting #707 (originally a single 35-file off-by-one batch) into coherent, per-subsystem pieces, so each can be reviewed and its regression-test impact assessed independently. #707 is being closed in favor of these four. The split is deliberate — the individual diffs are below the usual bundling threshold — because the changes are behavior-affecting and the maintainer asked to isolate regression-test impact per subsystem.
This PR: SEWING / splice / de novo / RNA structural assembly (10 files). Two related fixes.
Off-by-one in 1-indexed loops (
<→<=)The last element was silently skipped:
legacy_sewing/conformation/Assembly.cc×2 —pose_loop_anchors,disconnected_segments; the last segment, which by definition has no next, was skipped — exactly the case the predicate!has_next(i)is meant to catchpose_sewing/movers/BlockwiseAnalysisMover.ccpose_sewing/movers/OmnibusDisulfideAnalysisLabelerMover.ccsplice/SampleRotamersFromPDB.cc×2 — rotamer-deduplication scansplice/SpliceManager.cc— PSSM row check ("go over all the PSSM segments")stepwise/legacy/modeler/rna/StepWiseRNA_WorkingParametersSetup.ccrna/movers/RNAIdealizeMover.cc—CoordinateConstraintapplicationdenovo_design/components/FoldGraph.ccfldsgn/MatchResidues.cc×2 — RMSD / superimpose atom-id map buildfeatures/strand_assembly/StrandBundleFeatures.cc×2 — per-atom distance scanSelf-distance copy/paste bug (in the two
pose_sewingmovers)pose.residue(upstream_res).xyz(2).distance(pose.residue(upstream_res).xyz(2))is identically0, so the<= crit_dist_gate was always satisfied and the distance filter was a no-op. Corrected the second operand todownstream_res.Regression tests
This PR touches
pose_sewing. The output change is intentional (combined off-by-one + self-distance correction) but results-affecting and warrants scientific sign-off before merge. The other files here are not in the reported changed-test set.