Fix off-by-one loops over 1-indexed residue/atom/element ranges#707
Open
lyskov-ai wants to merge 1 commit into
Open
Fix off-by-one loops over 1-indexed residue/atom/element ranges#707lyskov-ai wants to merge 1 commit into
lyskov-ai wants to merge 1 commit into
Conversation
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.
Member
roccomoretti
left a comment
There was a problem hiding this comment.
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.
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
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
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 usesias 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 siblingJumpDownstreamSelectorwhich uses<=)protocols/antibody/residue_selector/CDRResidueSelector.ccprotocols/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.ccprotocols/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_atomsmap fill, per-row state init)protocols/jd3/JobGenealogist.ccprotocols/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.ccprotocols/membrane/MPLipidAccessibility.cc×2 (slice iteration)protocols/moves/PyMOLMover.cc(relevant_residuesmask init — last entry left defaultfalsedespite the intent of "all relevant")protocols/pose_metric_calculators/DecomposeAndReweightEnergiesCalculator.ccprotocols/pose_metric_calculators/SurfaceCalculator.cc(per-residue summary string)protocols/pose_sewing/movers/BlockwiseAnalysisMover.cc×2protocols/pose_sewing/movers/OmnibusDisulfideAnalysisLabelerMover.cc×3protocols/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.ccprotocols/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 identically0, 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.ccprotocols/pose_sewing/movers/OmnibusDisulfideAnalysisLabelerMover.ccWhat was deliberately NOT touched
The audit also turned up many candidate matches that are actually intentional and were left alone:
vec[i+1]/pose.residue(i+1)(need to stop atsize-1to stay in bounds — e.g.,is_cutpointscans, SS-transition checks, adjacent-distance checks).for i<size: print sep; print last) — e.g.,protocols/jd3/InnerLarvalJob.cc,protocols/ligand_evolution/Scorer.cc.std::vectorbody 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]).SurfaceInteractionGraph.hh,RNAIdealizeMover.cc, etc.).Debug build (
mode=debug) is clean.