Fix off-by-one in 1-indexed loops in frag_picker + accounting (split 2/4 of #707) - #731
Merged
lyskov merged 1 commit intoJul 10, 2026
Conversation
Split from RosettaCommons#707. Enumeration/accounting loops that use 'for ( i = 1; i < container.size(); ++i )' with i as a direct 1-indexed accessor, silently skipping the last element. Changed < to <=. - frag_picker/GrabAllCollector.hh (clear) - frag_picker/VallProvider.cc (find_chunk) - frag_picker/quota/QuotaCollector.cc x5 (per-position pool enumeration) - frag_picker/scores/AtomBasedConstraintsScore.cc x2 (constrainable_atoms map fill, per-row state init) - jd3/JobGenealogist.cc (newick_tree) - pose_metric_calculators/DecomposeAndReweightEnergiesCalculator.cc - pose_metric_calculators/SurfaceCalculator.cc (per-residue summary string) - canonical_sampling/mc_convergence_checks/HierarchicalLevel.cc (address match count) - unfolded_state_energy_calculator/UnfoldedStateEnergyCalculatorMover.cc (protein-residue count) Note: the unfolded_state_energy_calc regression test output changes with this batch (the trailing residue is now counted); the change is intentional and warrants scientific sign-off before merge.
roccomoretti
approved these changes
Jul 6, 2026
lyskov
approved these changes
Jul 8, 2026
Member
|
Wow, really is a common bug:) |
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
4 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: fragment picking + job / metric accounting (9 files).
Enumeration/accounting loops that use
for ( i = 1; i < container.size(); ++i )withias a direct 1-indexed accessor, silently skipping the last element. Changed<to<=.frag_picker/GrabAllCollector.hh—clearfrag_picker/VallProvider.cc—find_chunkfrag_picker/quota/QuotaCollector.cc×5 — per-position pool enumerationfrag_picker/scores/AtomBasedConstraintsScore.cc×2 —constrainable_atomsmap fill, per-row state initjd3/JobGenealogist.cc—newick_treepose_metric_calculators/DecomposeAndReweightEnergiesCalculator.ccpose_metric_calculators/SurfaceCalculator.cc— per-residue summary stringcanonical_sampling/mc_convergence_checks/HierarchicalLevel.cc— address match countunfolded_state_energy_calculator/UnfoldedStateEnergyCalculatorMover.cc— protein-residue countRegression tests
This PR touches
unfolded_state_energy_calc(viaUnfoldedStateEnergyCalculatorMover.cc— the trailing residue is now counted). That output change is intentional but results-affecting and warrants scientific sign-off before merge. The remaining files (frag_picker, jd3, metric calculators,HierarchicalLevel) are not in the reported changed-test set.