Fix variable bound violation in CPUFJ moves#930
Merged
rapids-bot[bot] merged 3 commits intoNVIDIA:mainfrom Mar 4, 2026
Merged
Conversation
aliceb-nv
added
bug
rg20
approved these changes
Mar 4, 2026
Contributor
Author
|
/ok to test 42267dd |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change updates the feasibility jump CPU implementation to consistently use incumbent objective tracking instead of best objective, adds defensive bounds clamping in variable assignment and constraint recomputation, and refines move application logic to ensure variables remain within bounds. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu (1)
764-779:⚠️ Potential issue | 🔴 CriticalPropagate the clamped delta through all state updates.
new_valis clamped, but LHS/objective logic still uses the originaldelta. When clamp changes the move,h_assignmentcan diverge fromh_lhs/h_incumbent_objective, so drift can still accumulate.Proposed fix (use
applied_deltaconsistently)static void apply_move(fj_cpu_climber_t<i_t, f_t>& fj_cpu, i_t var_idx, f_t delta, bool localmin = false) { + f_t old_val = fj_cpu.h_assignment[var_idx]; + f_t new_val = old_val + delta; + if (is_integer_var<i_t, f_t>(fj_cpu, var_idx)) { + cuopt_assert(fj_cpu.view.pb.integer_equal(new_val, round(new_val)), "new_val is not integer"); + new_val = round(new_val); + } + new_val = std::min(std::max(new_val, get_lower(fj_cpu.h_var_bounds[var_idx].get())), + get_upper(fj_cpu.h_var_bounds[var_idx].get())); + f_t applied_delta = new_val - old_val; + // Update the LHSs of all involved constraints. ... - f_t y = cstr_coeff * delta - fj_cpu.h_lhs_sumcomp[cstr_idx]; + f_t y = cstr_coeff * applied_delta - fj_cpu.h_lhs_sumcomp[cstr_idx]; ... - f_t new_val = fj_cpu.h_assignment[var_idx] + delta; - ... fj_cpu.h_assignment[var_idx] = new_val; - fj_cpu.h_incumbent_objective += fj_cpu.h_obj_coeffs[var_idx] * delta; + fj_cpu.h_incumbent_objective += fj_cpu.h_obj_coeffs[var_idx] * applied_delta; - if (delta > 0) { + if (applied_delta > 0) {As per coding guidelines: "Validate algorithm correctness in optimization logic: ... constraint/objective handling must produce correct results" and "Check numerical stability: prevent ... precision loss ...".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu` around lines 764 - 779, The code clamps new_val but continues to use the original delta, causing h_assignment to diverge from h_lhs and h_incumbent_objective; compute an applied_delta = new_val - old_value (old_value is the prior fj_cpu.h_assignment[var_idx]) immediately after clamping and then use applied_delta everywhere the move is applied (replace uses of delta for updating fj_cpu.h_lhs, fj_cpu.h_incumbent_objective, and any other state updates that depend on the actual change); ensure checks (cuopt_assert/check_variable_within_bounds/isfinite) still refer to new_val and that h_obj_coeffs[var_idx] is multiplied by applied_delta when updating the objective.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/src/mip_heuristics/feasibility_jump/feasibility_jump_kernels.cu`:
- Around line 592-599: The kernel clamps new_val but still uses
fj.jump_move_delta when updating fj.incumbent_lhs and fj.incumbent_objective,
leading to inconsistent state; change the logic to compute the clamped new_val
from fj.incumbent_assignment[var_idx] and variable_bounds (using
get_lower/get_upper), then compute applied_delta = new_val -
fj.incumbent_assignment[var_idx] and use applied_delta for all subsequent
updates to fj.incumbent_lhs and fj.incumbent_objective (and any other places
that currently use jump_move_delta), so the stored assignment, LHS and objective
reflect the actual clamped step; apply the same fix at the other similar blocks
mentioned around the earlier (560-587) and later (654-657) ranges.
---
Outside diff comments:
In `@cpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu`:
- Around line 764-779: The code clamps new_val but continues to use the original
delta, causing h_assignment to diverge from h_lhs and h_incumbent_objective;
compute an applied_delta = new_val - old_value (old_value is the prior
fj_cpu.h_assignment[var_idx]) immediately after clamping and then use
applied_delta everywhere the move is applied (replace uses of delta for updating
fj_cpu.h_lhs, fj_cpu.h_incumbent_objective, and any other state updates that
depend on the actual change); ensure checks
(cuopt_assert/check_variable_within_bounds/isfinite) still refer to new_val and
that h_obj_coeffs[var_idx] is multiplied by applied_delta when updating the
objective.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2c0c0fb1-61d1-41b5-851e-f10d7599ac0a
📒 Files selected for processing (2)
cpp/src/mip_heuristics/feasibility_jump/feasibility_jump_kernels.cucpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu
Comment on lines
+592
to
+599
| f_t new_val = fj.incumbent_assignment[var_idx] + fj.jump_move_delta[var_idx]; | ||
|
|
||
| cuopt_assert(fj.pb.check_variable_within_bounds(var_idx, new_val), | ||
| "assignment not within bounds"); | ||
| // clamping to err on the safe size - assert catches this | ||
| auto bounds = fj.pb.variable_bounds[var_idx]; | ||
| f_t lb = get_lower(bounds); | ||
| f_t ub = get_upper(bounds); | ||
| new_val = min(max(new_val, lb), ub); |
There was a problem hiding this comment.
Use clamped applied delta for kernel LHS/objective updates.
The kernel clamps new_val, but incumbent_lhs and incumbent_objective are still updated with jump_move_delta. If clamp changes the step, state becomes inconsistent.
Proposed fix (shared clamped value + shared applied_delta)
__global__ void update_assignment_kernel(...){
+ __shared__ f_t clamped_new_val;
+ __shared__ f_t applied_delta;
...
+ if (FIRST_THREAD) {
+ auto bounds = fj.pb.variable_bounds[var_idx];
+ f_t old_val = fj.incumbent_assignment[var_idx];
+ f_t lb = get_lower(bounds);
+ f_t ub = get_upper(bounds);
+ clamped_new_val = min(max(old_val + fj.jump_move_delta[var_idx], lb), ub);
+ applied_delta = clamped_new_val - old_val;
+ }
+ __syncthreads();
+
for (auto i = offset_begin + blockIdx.x; i < offset_end; i += gridDim.x) {
...
- f_t new_lhs = old_lhs + cstr_coeff * fj.jump_move_delta[var_idx];
+ f_t new_lhs = old_lhs + cstr_coeff * applied_delta;
...
- f_t y = cstr_coeff * fj.jump_move_delta[var_idx] - fj.incumbent_lhs_sumcomp[cstr_idx];
+ f_t y = cstr_coeff * applied_delta - fj.incumbent_lhs_sumcomp[cstr_idx];
}
...
if (FIRST_THREAD) {
- fj.incumbent_assignment[var_idx] = new_val;
- *fj.incumbent_objective += fj.pb.objective_coefficients[var_idx] * fj.jump_move_delta[var_idx];
+ fj.incumbent_assignment[var_idx] = clamped_new_val;
+ *fj.incumbent_objective += fj.pb.objective_coefficients[var_idx] * applied_delta;
}
}As per coding guidelines: "Validate algorithm correctness in optimization logic: ... constraint/objective handling must produce correct results" and "Check numerical stability: prevent ... precision loss ...".
Also applies to: 560-587, 654-657
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/mip_heuristics/feasibility_jump/feasibility_jump_kernels.cu` around
lines 592 - 599, The kernel clamps new_val but still uses fj.jump_move_delta
when updating fj.incumbent_lhs and fj.incumbent_objective, leading to
inconsistent state; change the logic to compute the clamped new_val from
fj.incumbent_assignment[var_idx] and variable_bounds (using
get_lower/get_upper), then compute applied_delta = new_val -
fj.incumbent_assignment[var_idx] and use applied_delta for all subsequent
updates to fj.incumbent_lhs and fj.incumbent_objective (and any other places
that currently use jump_move_delta), so the stored assignment, LHS and objective
reflect the actual clamped step; apply the same fix at the other similar blocks
mentioned around the earlier (560-587) and later (654-657) ranges.
Contributor
Author
|
/ok to test d07e2ae |
@aliceb-nv, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
Contributor
Author
|
/ok to test d07e2ae |
Contributor
Author
|
/merge |
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.
CPUFJ did not ensure that moves generated were clamped to the variable bounds. This allowed very slight (but within tolerance) bound violations accumulate, resulting in numerically flawed objective values.
This fixes a bug seen on
cbs-ctawhere solutions were found that beat the optimal of 0.Description
Issue
Checklist