Fix issue with incorrect lower bound on air05#890
Fix issue with incorrect lower bound on air05#890rapids-bot[bot] merged 6 commits intoNVIDIA:mainfrom
Conversation
chris-maes
added
bug
|
/ok to test 1258dee |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved relative mip-gap pruning across branch-and-bound codepaths, tightened LP cutoff arithmetic, and updated best-first queue handling to rely on strict Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/branch_and_bound/branch_and_bound.cpp (1)
1419-1429:⚠️ Potential issue | 🟡 Minor
rel_gapis now dead code in four pruning sites — remove to prevent compiler warningsAfter the pruning condition was simplified to
lower_bound > upper_boundalone, therel_gapvariable computed immediately before each condition is never read. This affectsplunge_with(line 1419),dive_with(line 1536),run_deterministic_bfs_loop(line 2800), anddeterministic_dive(line 3589). If the project builds with-Wunused-variable -Werror, this will break compilation.🐛 Proposed removal (same pattern applies to all four sites)
- f_t rel_gap = user_relative_gap(original_lp_, upper_bound, lower_bound); - if (lower_bound > upper_bound) {Also applies to: 1536-1539, 2800-2801, 3589-3590
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 1419 - 1429, Remove the now-unused rel_gap local and its computation via user_relative_gap at each pruning site to avoid unused-variable warnings: delete the line defining rel_gap (and any immediate comment only about rel_gap) in plunge_with, dive_with, run_deterministic_bfs_loop, and deterministic_dive; keep the worker->lower_bound assignment and the simplified pruning check (if (lower_bound > upper_bound)) intact so behavior is unchanged. Ensure you only remove the rel_gap declaration/call to user_relative_gap and not any other logic or variables referenced in those functions.
🧹 Nitpick comments (2)
cpp/src/branch_and_bound/branch_and_bound.cpp (2)
2474-2492: Redundant outerifin lower-bound drain — both branches are identicalThe outer
if (node_queue_.best_first_queue_size() > 0)guard is unnecessary; when the queue is already empty thewhileloop is a no-op, so both branches reduce to the same assignment. The logic can be expressed more directly:♻️ Suggested simplification
- if (node_queue_.best_first_queue_size() > 0) { - // We need to clear the queue and use the info in the search tree for the lower bound - while (node_queue_.best_first_queue_size() > 0) { - std::optional<mip_node_t<i_t, f_t>*> start_node = node_queue_.pop_best_first(); - - if (!start_node.has_value()) { continue; } - if (upper_bound_ < start_node.value()->lower_bound) { - // This node was put on the heap earlier but its lower bound is now greater than the - // current upper bound - search_tree_.graphviz_node( - settings_.log, start_node.value(), "cutoff", start_node.value()->lower_bound); - search_tree_.update(start_node.value(), node_status_t::FATHOMED); - continue; - } - } - lower_bound = search_tree_.root.lower_bound; - } else { - lower_bound = search_tree_.root.lower_bound; - } + // Drain the queue: fathom any node whose lower bound already exceeds the upper bound, + // leaving the remaining nodes active in the search tree to anchor the final lower bound. + while (node_queue_.best_first_queue_size() > 0) { + std::optional<mip_node_t<i_t, f_t>*> start_node = node_queue_.pop_best_first(); + if (!start_node.has_value()) { continue; } + if (upper_bound_ < start_node.value()->lower_bound) { + search_tree_.graphviz_node( + settings_.log, start_node.value(), "cutoff", start_node.value()->lower_bound); + search_tree_.update(start_node.value(), node_status_t::FATHOMED); + } + } + lower_bound = search_tree_.root.lower_bound;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 2474 - 2492, Remove the redundant outer if: always attempt to drain the best-first queue with the while loop using node_queue_.best_first_queue_size() and pop_best_first(), then set lower_bound = search_tree_.root.lower_bound; keep the cutoff handling (search_tree_.graphviz_node and update to FATHOMED) inside the loop when upper_bound_ < start_node->lower_bound; in short, delete the outer if/else and leave the while drain followed by the single lower_bound assignment to simplify logic around node_queue_, pop_best_first(), search_tree_, and lower_bound.
1319-1323:solve_node_lpintegral cut-off fix is correct; flag asymmetry with deterministic pathsRemoving the
- 1is the right fix. Withinteger_tol > dual_tol(typical values 1e-6 vs 1e-7), the old formula producedcut_off = (ub_int - 1) + dual_tol, causing LP values in the range(ub_int - 1 + dual_tol, ub_int - 1 + integer_tol)to be incorrectly cut off. For example, withub = 10, an LP value of9.000001(withininteger_tolof 9) would satisfyleaf_obj <= upper_bound + abs_fathom_tolsemantics for an improvable integer bound of 9, yet the LP was terminated early and the node receivedlower_bound = upper_bound = 10, inflating the reported lower bound.The equivalent fix is not applied in
solve_node_deterministic(line 3003) ordeterministic_dive(line 3614), which useworker.local_upper_bound + settings_.dual_toldirectly. Whenlocal_upper_boundis an exact integer this is equivalent, but if floating-point representation places it withininteger_tolof the next integer, the same off-by-one could reappear. Consider applying the same formula there for consistency.♻️ Suggested alignment for deterministic paths
// In solve_node_deterministic (~line 3003) and deterministic_dive (~line 3614): - lp_settings.cut_off = worker.local_upper_bound + settings_.dual_tol; + if (original_lp_.objective_is_integral) { + lp_settings.cut_off = std::ceil(worker.local_upper_bound - settings_.integer_tol) + settings_.dual_tol; + } else { + lp_settings.cut_off = worker.local_upper_bound + settings_.dual_tol; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 1319 - 1323, The deterministic code paths must apply the same integer-aware cut_off logic you fixed in solve_node_lp: in solve_node_deterministic and deterministic_dive replace the current cut_off = worker.local_upper_bound + settings_.dual_tol with a conditional that, when original_lp_.objective_is_integral is true, sets cut_off = std::ceil(worker.local_upper_bound - settings_.integer_tol) + settings_.dual_tol, otherwise uses worker.local_upper_bound + settings_.dual_tol; use the same symbols (worker.local_upper_bound, settings_.integer_tol, settings_.dual_tol, original_lp_.objective_is_integral) so the deterministic paths and solve_node_lp behave consistently for upper bounds that are within integer_tol of an integer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 1419-1429: Remove the now-unused rel_gap local and its computation
via user_relative_gap at each pruning site to avoid unused-variable warnings:
delete the line defining rel_gap (and any immediate comment only about rel_gap)
in plunge_with, dive_with, run_deterministic_bfs_loop, and deterministic_dive;
keep the worker->lower_bound assignment and the simplified pruning check (if
(lower_bound > upper_bound)) intact so behavior is unchanged. Ensure you only
remove the rel_gap declaration/call to user_relative_gap and not any other logic
or variables referenced in those functions.
---
Nitpick comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 2474-2492: Remove the redundant outer if: always attempt to drain
the best-first queue with the while loop using
node_queue_.best_first_queue_size() and pop_best_first(), then set lower_bound =
search_tree_.root.lower_bound; keep the cutoff handling
(search_tree_.graphviz_node and update to FATHOMED) inside the loop when
upper_bound_ < start_node->lower_bound; in short, delete the outer if/else and
leave the while drain followed by the single lower_bound assignment to simplify
logic around node_queue_, pop_best_first(), search_tree_, and lower_bound.
- Around line 1319-1323: The deterministic code paths must apply the same
integer-aware cut_off logic you fixed in solve_node_lp: in
solve_node_deterministic and deterministic_dive replace the current cut_off =
worker.local_upper_bound + settings_.dual_tol with a conditional that, when
original_lp_.objective_is_integral is true, sets cut_off =
std::ceil(worker.local_upper_bound - settings_.integer_tol) +
settings_.dual_tol, otherwise uses worker.local_upper_bound +
settings_.dual_tol; use the same symbols (worker.local_upper_bound,
settings_.integer_tol, settings_.dual_tol, original_lp_.objective_is_integral)
so the deterministic paths and solve_node_lp behave consistently for upper
bounds that are within integer_tol of an integer.
|
/ok to test b5c6e94 |
|
/ok to test c8324c1 |
@chris-maes, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test 99a8bb5 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cpp/src/branch_and_bound/branch_and_bound.cpp (1)
1417-1436:rel_gapis computed but no longer used — remove dead variable.With the removal of the
rel_gap-based fathoming condition, the variable on line 1419 is now unreferenced. Same pattern applies at line 1536 indive_with, line 2803 inrun_deterministic_bfs_loop, and line 3592 indeterministic_dive.♻️ Remove unused `rel_gap` in all four functions
// plunge_with (line ~1419) f_t lower_bound = node_ptr->lower_bound; f_t upper_bound = upper_bound_; - f_t rel_gap = user_relative_gap(original_lp_, upper_bound, lower_bound);// dive_with (line ~1536) f_t lower_bound = node_ptr->lower_bound; f_t upper_bound = upper_bound_; - f_t rel_gap = user_relative_gap(original_lp_, upper_bound, lower_bound); worker->lower_bound = lower_bound;// run_deterministic_bfs_loop (line ~2803) f_t upper_bound = worker.local_upper_bound; - f_t rel_gap = user_relative_gap(original_lp_, upper_bound, node->lower_bound); if (node->lower_bound > upper_bound) {// deterministic_dive (line ~3592) f_t rel_gap = user_relative_gap(original_lp_, worker.local_upper_bound, node_ptr->lower_bound); - if (node_ptr->lower_bound > worker.local_upper_bound) { + if (node_ptr->lower_bound > worker.local_upper_bound) {(only the
rel_gapline needs to be removed there)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 1417 - 1436, Remove the now-dead rel_gap local variable declaration (the line declaring "f_t rel_gap = user_relative_gap(...);") from each function where it's unused: the current function around node_ptr (in branch_and_bound.cpp), dive_with, run_deterministic_bfs_loop, and deterministic_dive; simply delete that single declaration in each function and run a build to confirm no other references remain.
🤖 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/branch_and_bound/branch_and_bound.cpp`:
- Around line 2474-2495: When popping from node_queue_ in the best-first loop,
the first non-fathomed node returned by node_queue_.pop_best_first() is
discarded before computing lower_bound, which overstates the proven lower bound;
fix by capturing that popped node (the optional from pop_best_first) into a
local variable (e.g., first_non_fathomed_node) when you break out of the loop
and then compute lower_bound as the minimum of that node's lower_bound and the
queue/root lower bound (use node_queue_.get_lower_bound() if queue non-empty,
else search_tree_.root.lower_bound); update references to
node_queue_.get_lower_bound() to incorporate
first_non_fathomed_node->lower_bound so the popped-but-not-fathomed node is
included in the provable lower bound calculation.
---
Nitpick comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 1417-1436: Remove the now-dead rel_gap local variable declaration
(the line declaring "f_t rel_gap = user_relative_gap(...);") from each function
where it's unused: the current function around node_ptr (in
branch_and_bound.cpp), dive_with, run_deterministic_bfs_loop, and
deterministic_dive; simply delete that single declaration in each function and
run a build to confirm no other references remain.
|
/ok to test 662efbc |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/branch_and_bound/branch_and_bound.cpp (1)
1417-1436:⚠️ Potential issue | 🟡 MinorRemove unused
rel_gaplocals — dead code after the rel-gap fathoming removal.
rel_gapis computed but never read in four separate functions now that therel_gap-based fathoming condition has been removed:
Location Unused variable plunge_with, line 1419f_t rel_gap = user_relative_gap(...)dive_with, line 1536f_t rel_gap = user_relative_gap(...)run_deterministic_bfs_loop, line 2805f_t rel_gap = user_relative_gap(...)deterministic_dive, line 3594f_t rel_gap = user_relative_gap(...)With
-Wunused-variable -Werror(common in CUDA/C++ builds) these four sites will fail compilation.🗑️ Proposed removals (same pattern in all four sites)
plunge_with:f_t lower_bound = node_ptr->lower_bound; f_t upper_bound = upper_bound_; - f_t rel_gap = user_relative_gap(original_lp_, upper_bound, lower_bound);
dive_with:f_t lower_bound = node_ptr->lower_bound; f_t upper_bound = upper_bound_; - f_t rel_gap = user_relative_gap(original_lp_, upper_bound, lower_bound); worker->lower_bound = lower_bound;
run_deterministic_bfs_loop:f_t upper_bound = worker.local_upper_bound; - f_t rel_gap = user_relative_gap(original_lp_, upper_bound, node->lower_bound); if (node->lower_bound > upper_bound) {
deterministic_dive:- f_t rel_gap = user_relative_gap(original_lp_, worker.local_upper_bound, node_ptr->lower_bound); if (node_ptr->lower_bound > worker.local_upper_bound) {Also applies to: 1534-1543, 2804-2812, 3593-3598
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 1417 - 1436, Remove the unused local variable rel_gap in each affected function: delete the line "f_t rel_gap = user_relative_gap(original_lp_, upper_bound, lower_bound);" from plunge_with, dive_with, run_deterministic_bfs_loop, and deterministic_dive; ensure no other code in those functions references rel_gap and rebuild to confirm no unused-variable warning remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 1417-1436: Remove the unused local variable rel_gap in each
affected function: delete the line "f_t rel_gap =
user_relative_gap(original_lp_, upper_bound, lower_bound);" from plunge_with,
dive_with, run_deterministic_bfs_loop, and deterministic_dive; ensure no other
code in those functions references rel_gap and rebuild to confirm no
unused-variable warning remains.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/src/branch_and_bound/branch_and_bound.cpp (1)
1419-1419: Deadrel_gapvariables left behind by the pruning-condition simplification.After removing the
rel_gap-based pruning from the conditions at lines 1429, 1539, 2806, and 3595, therel_gapvariable computed immediately before each condition is no longer read anywhere in its scope. Each call touser_relative_gap()performs a floating-point division and a NaN check per node — unnecessary overhead on hot paths.♻️ Suggested cleanup (same pattern at all four sites)
- f_t rel_gap = user_relative_gap(original_lp_, upper_bound, lower_bound); worker->lower_bound = lower_bound; if (lower_bound > upper_bound) {- f_t rel_gap = user_relative_gap(original_lp_, upper_bound, lower_bound); worker->lower_bound = lower_bound; if (node_ptr->lower_bound > upper_bound) {- f_t rel_gap = user_relative_gap(original_lp_, upper_bound, node->lower_bound); if (node->lower_bound > upper_bound) {- f_t rel_gap = user_relative_gap(original_lp_, worker.local_upper_bound, node_ptr->lower_bound); if (node_ptr->lower_bound > worker.local_upper_bound) {Also applies to: 1536-1536, 2805-2805, 3594-3594
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` at line 1419, Remove the now-unused rel_gap local and its call to user_relative_gap() to avoid unnecessary division/NaN checks; specifically, in the branch-and-bound node processing where rel_gap is computed (the local named rel_gap assigned from user_relative_gap(original_lp_, upper_bound, lower_bound)), delete that assignment and any dead local declaration so the hot path no longer calls user_relative_gap; ensure you only remove the unused variable and call (keep original_lp_, upper_bound, and lower_bound usage elsewhere intact) and run tests to confirm no remaining references to rel_gap or its value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 2474-2497: In the cleanup loop that pops best-first nodes (uses
node_queue_.pop_best_first and search_tree_.update with
node_status_t::FATHOMED), ensure the global unexplored-node counter is kept
consistent: when you mark a node FATHOMED inside this loop, decrement
nodes_unexplored (or call the same helper used elsewhere) so fathomed nodes are
counted the same as in plunge_with/run_deterministic_bfs_loop; leave the logic
that pushes the first non-fathomed node back into node_queue_ unchanged.
---
Nitpick comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Line 1419: Remove the now-unused rel_gap local and its call to
user_relative_gap() to avoid unnecessary division/NaN checks; specifically, in
the branch-and-bound node processing where rel_gap is computed (the local named
rel_gap assigned from user_relative_gap(original_lp_, upper_bound,
lower_bound)), delete that assignment and any dead local declaration so the hot
path no longer calls user_relative_gap; ensure you only remove the unused
variable and call (keep original_lp_, upper_bound, and lower_bound usage
elsewhere intact) and run tests to confirm no remaining references to rel_gap or
its value.
|
/ok to test 236f9bc |
|
/merge |
4 participants
Fix an issue with incorrect lower bound on air05.
In 26.02, with the new scheduler worker model, it looks like the lower bound is not computed correctly if there are nodes still remaining in the
node_queue. This PR fathoms all the nodes and pulls the lower bound from the search tree.In 26.02 we were also incorrectly fathoming nodes with objectives less than the optimal objective, due to the
rel_gapcheck. This PR removes that check.In 26.04 we were incorrectly setting the cutoff to one less than it should be when the objective was integral. This PR fixes that bug.