Fix off-by-one in 1-indexed loops in core/ (split 1/4 of #707)#730
Open
lyskov-ai wants to merge 1 commit into
Open
Fix off-by-one in 1-indexed loops in core/ (split 1/4 of #707)#730lyskov-ai wants to merge 1 commit into
lyskov-ai wants to merge 1 commit into
Conversation
Split from RosettaCommons#707 (subsystem: core/). Each loop uses 'for ( Size i = 1; i < container.size(); ++i )' (or < pose.size() / < nheavyatoms() / < upstream.size()) and the body uses i as a direct 1-indexed accessor, so the last element is silently skipped. Changed < to <= to include it. - energy_methods/RNA_LJ_BaseEnergy.cc (RNA neighbor scan) - io/pose_to_sfr/PoseToStructFileRepConverter.cc (SS-element enumeration; a 1-residue SS element at the C-terminus was dropped) - pack/interaction_graph/SurfacePotential.cc x3 (neighbor count, surface energy sum) - pose/rna/util.cc (heavy-atom contact scan) - select/residue_selector/JumpUpstreamSelector.cc (subset assignment; diverged from sibling JumpDownstreamSelector, which already uses <=)
roccomoretti
approved these changes
Jul 6, 2026
roccomoretti
left a comment
roccomoretti
left a comment
Member
There was a problem hiding this comment.
Looks reasonable, pending an update to the SurfacePotential unit tests.
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:
core/(5 files).Each loop uses
for ( Size i = 1; i < container.size(); ++i )(or< pose.size()/< nheavyatoms()/< upstream.size()) and the body usesias a direct 1-indexed accessor —pose.residue(i),vec[i],xyz(i)— so the last element is silently skipped. Changed<to<=to include it.core/energy_methods/RNA_LJ_BaseEnergy.cc— RNA neighbor scancore/io/pose_to_sfr/PoseToStructFileRepConverter.cc— SS-element enumeration; a 1-residue SS element at the C-terminus was droppedcore/pack/interaction_graph/SurfacePotential.cc×3 — neighbor count, surface-energy sumcore/pose/rna/util.cc— heavy-atom contact scancore/select/residue_selector/JumpUpstreamSelector.cc— subset assignment; diverged from siblingJumpDownstreamSelector, which already uses<=Regression tests
None of the changed regression tests reported for #707 (
enzdes,inverse_rotamer_remodel,ligand_dock_cholesterol,mp_f19_relax,mpil_load_implicit_lipids,pose_sewing,unfolded_state_energy_calc) fall in this subsystem, so this PR is expected to be inert with respect to that set. CI will confirm.