Skip to content

Fix off-by-one loops over 1-indexed residue/atom/element ranges#707

Open
lyskov-ai wants to merge 1 commit into
RosettaCommons:mainfrom
lyskov-ai:fix/off-by-one-1-indexed-loops-batch
Open

Fix off-by-one loops over 1-indexed residue/atom/element ranges#707
lyskov-ai wants to merge 1 commit into
RosettaCommons:mainfrom
lyskov-ai:fix/off-by-one-1-indexed-loops-batch

Conversation

@lyskov-ai
Copy link
Copy Markdown
Contributor

Summary

Audit-pass fix for a family of off-by-one bugs of the same shape across 35 files. Continues the pattern from #695 and #701; this batch covers loops the earlier audits did not reach.

Off-by-one in 1-indexed loops (33 files)

Each loop uses for ( Size i = 1; i < container.size(); ++i ) (or < pose.size() / < pose.total_residue() / < natoms() / < nres) and the body uses i as a direct 1-indexed element accessor — pose.residue(i), vec[i], xyz(i), etc. The last element is therefore silently skipped. Replaced with <= to include it.

Affected functions cover residue iteration in:

  • core/energy_methods/RNA_LJ_BaseEnergy.cc (RNA neighbor scan)
  • core/io/pose_to_sfr/PoseToStructFileRepConverter.cc (SS element enumeration — a 1-residue SS element at the C-terminus was dropped)
  • core/pack/interaction_graph/SurfacePotential.cc ×3 (neighbor count, surface energy sum)
  • core/pose/rna/util.cc (heavy-atom contact scan)
  • core/select/residue_selector/JumpUpstreamSelector.cc (subset assignment, divergent from sibling JumpDownstreamSelector which uses <=)
  • protocols/antibody/residue_selector/CDRResidueSelector.cc
  • protocols/canonical_sampling/mc_convergence_checks/HierarchicalLevel.cc (address match count)
  • protocols/cartesian/md.cc ×2 (per-atom state save/derivative check)
  • protocols/cutoutdomain/CutOutDomain.cc (find_nearest_res)
  • protocols/denovo_design/components/FoldGraph.cc
  • protocols/docking/DockingEnsemblePrepackProtocol.cc ×2 (chain identity validation)
  • protocols/docking/metrics.cc ×2 (cutpoint scan in Irmsd / contact metrics)
  • protocols/enzdes/EnzRepackMinimize.cc (movable-residue collection for backrub)
  • protocols/features/strand_assembly/StrandBundleFeatures.cc ×2 (per-atom distance scan)
  • protocols/fldsgn/MatchResidues.cc ×2 (RMSD/superimpose atom-id map build)
  • protocols/frag_picker/GrabAllCollector.hh (clear)
  • protocols/frag_picker/VallProvider.cc (find_chunk)
  • protocols/frag_picker/quota/QuotaCollector.cc ×5 (per-position pool enumeration)
  • protocols/frag_picker/scores/AtomBasedConstraintsScore.cc ×2 (constrainable_atoms map fill, per-row state init)
  • protocols/jd3/JobGenealogist.cc
  • protocols/legacy_sewing/conformation/Assembly.cc ×2 (pose_loop_anchors, disconnected_segments — the last segment, which by definition has no next, was being skipped — exactly the case the predicate !has_next(i) is meant to catch)
  • protocols/loops/util.cc ×3 (non-protein-chunk and per-loop accounting)
  • protocols/match/output/UpstreamDownstreamCollisionFilter.cc
  • protocols/membrane/MPLipidAccessibility.cc ×2 (slice iteration)
  • protocols/moves/PyMOLMover.cc (relevant_residues mask init — last entry left default false despite the intent of "all relevant")
  • protocols/pose_metric_calculators/DecomposeAndReweightEnergiesCalculator.cc
  • protocols/pose_metric_calculators/SurfaceCalculator.cc (per-residue summary string)
  • protocols/pose_sewing/movers/BlockwiseAnalysisMover.cc ×2
  • protocols/pose_sewing/movers/OmnibusDisulfideAnalysisLabelerMover.cc ×3
  • protocols/rna/movers/RNAIdealizeMover.cc (CoordinateConstraint application)
  • protocols/splice/SampleRotamersFromPDB.cc ×2 (rotamer-deduplication scan)
  • protocols/splice/SpliceManager.cc (PSSM row check — comment says "go over all the PSSM segments")
  • protocols/stepwise/legacy/modeler/rna/StepWiseRNA_WorkingParametersSetup.cc
  • protocols/unfolded_state_energy_calculator/UnfoldedStateEnergyCalculatorMover.cc (protein-residue count)

Self-distance copy/paste bug (folded into 2 of the above files)

pose.residue(upstream_res).xyz(2).distance(pose.residue(upstream_res).xyz(2)) is identically 0, so the <= crit_dist_ gate was always satisfied — the distance filter was a no-op. Fixed to .distance(pose.residue(downstream_res).xyz(2)) in:

  • protocols/pose_sewing/movers/BlockwiseAnalysisMover.cc
  • protocols/pose_sewing/movers/OmnibusDisulfideAnalysisLabelerMover.cc

What was deliberately NOT touched

The audit also turned up many candidate matches that are actually intentional and were left alone:

  • Loops with explicit pair access vec[i+1] / pose.residue(i+1) (need to stop at size-1 to stay in bounds — e.g., is_cutpoint scans, SS-transition checks, adjacent-distance checks).
  • Membrane-pose iterations that intentionally skip the trailing MEM virtual chain.
  • "Separator" output patterns (for i<size: print sep; print last) — e.g., protocols/jd3/InnerLarvalJob.cc, protocols/ligand_evolution/Scorer.cc.
  • Suite / connection enumerations where N segments produce N-1 connections by definition.
  • Variant-by-cutpoint scans where the variable indexes a between-residue position.
  • Self-pacing decompositions that already handle the last index in a special branch after the loop.
  • Loops where the analysis script flagged a 0-indexed std::vector body that is in fact correctly bounded (e.g., protocols/cluster/calibur/Clustering.cc, protocols/cluster/cluster.cc — cluster center at [0] plus members at [1..N-1]).
  • Commented-out code (in SurfaceInteractionGraph.hh, RNAIdealizeMover.cc, etc.).

Debug build (mode=debug) is clean.

Replace `i < container.size()` with `i <= container.size()` (or the
matching `i < pose.size()` -> `i <= pose.size()`) across 35 loops where
the body uses `i` as a direct 1-indexed element accessor
(`pose.residue(i)`, `vec[i]`, etc.) — so the last residue/atom/element
was silently being skipped.

Also fixes two related copy-paste bugs in pose_sewing that surfaced
during the audit: the distance check `pose.residue(upstream_res).xyz(2)
.distance(pose.residue(upstream_res).xyz(2))` was self-distance (always
zero, condition always satisfied) — should compare upstream vs
downstream residue xyz. Affects helix_pair detection in
BlockwiseAnalysisMover and OmnibusDisulfideAnalysisLabelerMover.

Skipped candidates: loops with pair access `[i+1]` (correct stop at
size-1), cutpoint-style "between residues" loops, MEM-chain-skipping
loops in protocols/membrane, separator-style "all-but-last + last raw"
patterns, suite/connection enumerations (N-1 connections for N
segments), and commented-out code.
Copy link
Copy Markdown
Member

@roccomoretti roccomoretti left a comment

Choose a reason for hiding this comment

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

Looks reasonable in principle, but it looks like there's a number of test changes which need to be tracked down to see if they're actual bug fixes, or if we missed some of the deliberately-skipped-the-last-entry cases.

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.

2 participants