[WIP] Improvements to reliability branching#979
[WIP] Improvements to reliability branching#979nguidotti wants to merge 9 commits intoNVIDIA:release/26.04from
Conversation
…the cutoff when no incumbent was found.
… improve-reliable-branching # Conflicts: # cpp/src/branch_and_bound/pseudo_costs.cpp
…change estimate via dual simplex single pivot (NVIDIA#963).
…ange instead of the objective. fixed candidate ranking in reliability branching.
|
/ok to test 94c3dc3 |
📝 WalkthroughWalkthroughAdded pivot-based dual-simplex objective-change estimation and wiring to strong-branching; updated reliable-variable selection and strong-branching interfaces to pass full LP solution and basis/context; introduced dual-simplex helpers for reduced-cost updates; adjusted solver setting ranges for branching modes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/pseudo_costs.cpp`:
- Around line 293-295: dual_phase2() can return dual::status_t::CUTOFF when
child_settings.cut_off is set, but the current helper treats any non-success as
failure and leaves obj as NaN; change the failure handling so that when
dual_phase2 returns CUTOFF you set obj to the applied cutoff
(child_settings.cut_off, i.e., upper_bound + settings.dual_tol) and treat it as
a valid result for seeding pseudo-costs instead of the generic failure path;
apply the same change to the other occurrence handling lines covering the same
logic (the block that currently inspects dual::status_t results and assigns
obj).
- Around line 898-906: Replace the std::cout/std::format usage in the block that
checks unreliabe_list.size() > max_num_candidates with the existing logger: call
log.printf() and pass a formatted message including
strong_branching_lp_iter.load(), branch_and_bound_lp_iters,
unreliable_list.size(), num_tasks, and reliable_threshold; remove the
std::format/include dependency and ensure the message format matches other
log.printf() calls in this file so logging is consistent with the parallel
solver pattern.
- Around line 219-245: The code currently passes basic_map[j] directly into
single_pivot_objective_change_estimate for each j in fractional; add a defensive
check on basic_map[j] before that call (e.g., if basic_map[j] < 0 then skip the
dual-pivot estimate for this j or assert/fail fast), so only valid basic indices
are forwarded; update both occurrences where fractional is iterated (the loop
using basic_map and the second occurrence mentioned) to guard around the call to
single_pivot_objective_change_estimate and avoid pushing an invalid basic index
into structures like e_k.i.
In `@cpp/src/dual_simplex/phase2.cpp`:
- Around line 175-206: The CHECK_CHANGE_IN_REDUCED_COST debug block calls
phase2::compute_reduced_cost_update but references out-of-scope symbols (lp,
basic_list, nonbasic_list); to fix, thread the required context into
compute_delta_z by updating its signature (and all callers) to accept the LP and
basis lists (or pass appropriate local equivalents available in this translation
unit) and then forward those parameters into
phase2::compute_reduced_cost_update, or alternatively construct/obtain the
needed lp, basic_list and nonbasic_list inside compute_delta_z before the debug
call so the debug path compiles; ensure you update function names
compute_delta_z and the call site(s) consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eb905199-9239-4fa9-a7c5-55a6b8c381b6
📒 Files selected for processing (6)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/pseudo_costs.cppcpp/src/branch_and_bound/pseudo_costs.hppcpp/src/dual_simplex/phase2.cppcpp/src/dual_simplex/phase2.hppcpp/src/math_optimization/solver_settings.cu
| if (unreliable_list.size() > max_num_candidates) { | ||
| std::cout << std::format( | ||
| "RB iters = {}, B&B iters = {}, unreliable = {}, num_tasks = {}, " | ||
| "reliable_threshold = {}\n", | ||
| strong_branching_lp_iter.load(), | ||
| branch_and_bound_lp_iters, | ||
| unreliable_list.size(), | ||
| num_tasks, | ||
| reliable_threshold); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "CMAKE_CXX_STANDARD|cxx_std_|-std=c\\+\\+" .
rg -n "#include <format>|std::format" cpp/src/branch_and_bound/pseudo_costs.cpp cpp/srcRepository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Check if the file exists and its size
fd "pseudo_costs.cpp" --type f
# If found, read the specific lines mentioned
if [ -f "cpp/src/branch_and_bound/pseudo_costs.cpp" ]; then
echo "=== Lines around 898-906 ==="
sed -n '890,910p' "cpp/src/branch_and_bound/pseudo_costs.cpp"
fiRepository: NVIDIA/cuopt
Length of output: 900
🏁 Script executed:
# Search for std::format more broadly with different patterns
rg "format\(" . -t cpp -t hpp --max-count 20
# Check for iostream usage and cout
rg "std::cout" cpp/src/branch_and_bound/ -n
# Search project root for CMakeLists.txt and check C++ standard
find . -name "CMakeLists.txt" -type f | head -5Repository: NVIDIA/cuopt
Length of output: 331
🏁 Script executed:
# Check includes at top of the file
head -50 cpp/src/branch_and_bound/pseudo_costs.cpp | grep -E "#include|format"
# Check C++ standard in CMakeLists.txt
grep -r "CMAKE_CXX_STANDARD\|cxx_std_" cpp/CMakeLists.txt cpp/src/CMakeLists.txt
# Count all std::format usages in the file
rg "std::format" cpp/src/branch_and_bound/pseudo_costs.cpp -n
# Check if <format> is explicitly included anywhere
rg "#include.*format" cpp/src/Repository: NVIDIA/cuopt
Length of output: 1143
🏁 Script executed:
# Check CMakeLists.txt for C++ standard setting
cat cpp/CMakeLists.txt | grep -A5 -B5 "CMAKE_CXX_STANDARD\|CXX_STANDARD" | head -20
# Also check cpp/src/CMakeLists.txt
cat cpp/src/CMakeLists.txt | grep -A5 -B5 "CMAKE_CXX_STANDARD\|CXX_STANDARD" | head -20
# Check if <format> is included in the hpp file
head -50 cpp/src/branch_and_bound/pseudo_costs.hpp | grep -E "#include|format"Repository: NVIDIA/cuopt
Length of output: 907
Use the existing logger instead of std::cout and std::format.
Lines 893-899 show the code already uses log.printf() for logging. The subsequent block (lines 899-908) switches to std::cout << std::format(), bypassing the established logging infrastructure. Additionally, <format> is not explicitly included in this translation unit. Maintain consistency by using log.printf() throughout, which aligns with the parallel solver pattern already established in this function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/branch_and_bound/pseudo_costs.cpp` around lines 898 - 906, Replace
the std::cout/std::format usage in the block that checks unreliabe_list.size() >
max_num_candidates with the existing logger: call log.printf() and pass a
formatted message including strong_branching_lp_iter.load(),
branch_and_bound_lp_iters, unreliable_list.size(), num_tasks, and
reliable_threshold; remove the std::format/include dependency and ensure the
message format matches other log.printf() calls in this file so logging is
consistent with the parallel solver pattern.
| #ifdef CHECK_CHANGE_IN_REDUCED_COST | ||
| const i_t m = A_transpose.n; | ||
| const i_t n = A_transpose.m; | ||
| std::vector<f_t> delta_y_dense(m); | ||
| delta_y.to_dense(delta_y_dense); | ||
| std::vector<f_t> delta_z_check(n); | ||
| std::vector<i_t> delta_z_mark_check(n, 0); | ||
| std::vector<i_t> delta_z_indices_check; | ||
| phase2::compute_reduced_cost_update(lp, | ||
| basic_list, | ||
| nonbasic_list, | ||
| delta_y_dense, | ||
| leaving_index, | ||
| direction, | ||
| delta_z_mark_check, | ||
| delta_z_indices_check, | ||
| delta_z_check, | ||
| work_estimate); | ||
| f_t error_check = 0.0; | ||
| for (i_t k = 0; k < n; ++k) { | ||
| const f_t diff = std::abs(delta_z[k] - delta_z_check[k]); | ||
| if (diff > 1e-6) { | ||
| printf("delta_z error %d transpose %e no transpose %e diff %e\n", | ||
| k, | ||
| delta_z[k], | ||
| delta_z_check[k], | ||
| diff); | ||
| } | ||
| error_check = std::max(error_check, diff); | ||
| } | ||
| if (error_check > 1e-6) { printf("delta_z error %e\n", error_check); } | ||
| #endif |
There was a problem hiding this comment.
CHECK_CHANGE_IN_REDUCED_COST path is currently uncompilable after refactor.
At Line 183, the debug path uses phase2::compute_reduced_cost_update(...) and references lp, basic_list, and nonbasic_list, but those symbols are not in scope in this function. Enabling the macro will fail compilation.
Suggested fix (thread required context into compute_delta_z)
-template <typename i_t, typename f_t>
-void compute_delta_z(const csc_matrix_t<i_t, f_t>& A_transpose,
+template <typename i_t, typename f_t>
+void compute_delta_z(const lp_problem_t<i_t, f_t>& lp,
+ const std::vector<i_t>& basic_list,
+ const std::vector<i_t>& nonbasic_list,
+ const csc_matrix_t<i_t, f_t>& A_transpose,
const sparse_vector_t<i_t, f_t>& delta_y,
i_t leaving_index,
i_t direction,
@@
- phase2::compute_reduced_cost_update(lp,
- basic_list,
- nonbasic_list,
- delta_y_dense,
- leaving_index,
- direction,
- delta_z_mark_check,
- delta_z_indices_check,
- delta_z_check,
- work_estimate);
+ compute_reduced_cost_update(lp,
+ basic_list,
+ nonbasic_list,
+ delta_y_dense,
+ leaving_index,
+ direction,
+ delta_z_mark_check,
+ delta_z_indices_check,
+ delta_z_check,
+ work_estimate);- compute_delta_z(A_transpose,
+ compute_delta_z(lp,
+ basic_list,
+ nonbasic_list,
+ A_transpose,
delta_y_sparse,
leaving_index,
direction,
nonbasic_mark,
delta_z_mark,
delta_z_indices,
delta_z,
phase2_work_estimate);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/dual_simplex/phase2.cpp` around lines 175 - 206, The
CHECK_CHANGE_IN_REDUCED_COST debug block calls
phase2::compute_reduced_cost_update but references out-of-scope symbols (lp,
basic_list, nonbasic_list); to fix, thread the required context into
compute_delta_z by updating its signature (and all callers) to accept the LP and
basis lists (or pass appropriate local equivalents available in this translation
unit) and then forward those parameters into
phase2::compute_reduced_cost_update, or alternatively construct/obtain the
needed lp, basic_list and nonbasic_list inside compute_delta_z before the debug
call so the debug path compiles; ensure you update function names
compute_delta_z and the call site(s) consistently.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/pseudo_costs.cpp`:
- Around line 420-431: The cutoff sum in the else branch of pseudo_costs.cpp may
multiply objective coefficients by infinite bounds; update the computation in
the block that sets child_settings.cut_off to check
std::isfinite(child_problem.upper[i]) and std::isfinite(child_problem.lower[i])
before doing the multiplication with original_lp.objective[i]; skip terms with
non-finite bounds, and if a skipped term has a non-zero objective coefficient
treat the cutoff as unbounded (set child_settings.cut_off to +/-INFINITY based
on the sign of original_lp.objective[i]) or otherwise keep the finite
accumulated sum, ensuring no multiplication by non-finite values occurs (refer
to child_settings.cut_off, original_lp.objective, and
child_problem.upper/lower).
- Around line 140-161: The debug block wrongly accesses
delta_y.lp_solution.x[k]; delta_y is a sparse_vector_t with arrays .i and .x, so
replace delta_y.lp_solution.x[k] with delta_y.x[k] (keeping the loop over k <
delta_y.i.size()) so the index/value pairs are read correctly; leave the rest of
the CHECK_DELTA_OBJ computation (uses delta_z_indices, vstatus, delta_z,
variable_j, delta_obj_check, step_length) unchanged so the validity check
compiles and compares delta_obj_check to delta_obj as intended.
- Around line 121-131: The debug block references an undefined variable y
causing a compile error; change the code to use the solver's dual vector (e.g.
lp_solution.y) or declare a local alias before use. Specifically, in the
CHECK_DUAL_FEASIBILITY block update the call to matrix_transpose_vector_multiply
to pass lp_solution.y (or create vector<f_t> y = lp_solution.y) instead of the
undefined y so dual_residual, matrix_transpose_vector_multiply, vector_norm_inf
and settings.log.printf work correctly.
- Around line 296-308: The fallback cutoff calculation when upper_bound is not
finite can produce +/-inf by multiplying objective coefficients with infinite
variable bounds; update the block that sets child_settings.cut_off (the else
branch using original_lp.objective, child_problem.upper and child_problem.lower)
to ignore terms where the corresponding child_problem.upper or
child_problem.lower is not finite (or only compute the fallback when all
contributing bounds are finite), summing only finite products and leaving
child_settings.cut_off at a safe default (or signaling no cutoff) if any
required bound is infinite; ensure the logic preserves the existing behavior
when bounds are finite and still adds settings.dual_tol when applicable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7905b856-7959-4fe1-bae4-678afac404e8
📒 Files selected for processing (1)
cpp/src/branch_and_bound/pseudo_costs.cpp
| #ifdef CHECK_DUAL_FEASIBILITY | ||
| { | ||
| std::vector<f_t> dual_residual = lp_solution.z; | ||
| for (i_t j = 0; j < lp.num_cols; j++) { | ||
| dual_residual[j] -= lp.objective[j]; | ||
| } | ||
| matrix_transpose_vector_multiply(lp.A, 1.0, y, 1.0, dual_residual); | ||
| f_t dual_residual_norm = vector_norm_inf<i_t, f_t>(dual_residual); | ||
| settings.log.printf("Dual residual norm: %e\n", dual_residual_norm); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Debug code uses undefined variable y.
In the CHECK_DUAL_FEASIBILITY block, line 127 references y which is not declared in this function's scope. This will cause a compilation error when the debug macro is enabled.
Suggested fix
- matrix_transpose_vector_multiply(lp.A, 1.0, y, 1.0, dual_residual);
+ matrix_transpose_vector_multiply(lp.A, 1.0, lp_solution.y, 1.0, dual_residual);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/branch_and_bound/pseudo_costs.cpp` around lines 121 - 131, The debug
block references an undefined variable y causing a compile error; change the
code to use the solver's dual vector (e.g. lp_solution.y) or declare a local
alias before use. Specifically, in the CHECK_DUAL_FEASIBILITY block update the
call to matrix_transpose_vector_multiply to pass lp_solution.y (or create
vector<f_t> y = lp_solution.y) instead of the undefined y so dual_residual,
matrix_transpose_vector_multiply, vector_norm_inf and settings.log.printf work
correctly.
| #ifdef CHECK_DELTA_OBJ | ||
| f_t delta_obj_check = 0.0; | ||
| for (i_t k = 0; k < delta_y.i.size(); k++) { | ||
| delta_obj_check += lp.rhs[delta_y.i[k]] * delta_y.lp_solution.x[k]; | ||
| } | ||
| for (i_t h = 0; h < delta_z_indices.size(); h++) { | ||
| const i_t j = delta_z_indices[h]; | ||
| if (vstatus[j] == variable_status_t::NONBASIC_LOWER) { | ||
| delta_obj_check += lp.lower[j] * delta_z[j]; | ||
| } else if (vstatus[j] == variable_status_t::NONBASIC_UPPER) { | ||
| delta_obj_check += lp.upper[j] * delta_z[j]; | ||
| } | ||
| } | ||
| delta_obj_check += std::floor(lp_solution.x[variable_j]) * delta_z[variable_j]; | ||
| delta_obj_check *= step_length; | ||
| if (std::abs(delta_obj_check - delta_obj) > 1e-6) { | ||
| settings.log.printf("Delta obj check %e. Delta obj %e. Step length %e.\n", | ||
| delta_obj_check, | ||
| delta_obj, | ||
| step_length); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Debug code has invalid member access delta_y.lp_solution.x[k].
delta_y is a sparse_vector_t<i_t, f_t> which has .i and .x members for indices and values respectively. The expression delta_y.lp_solution.x[k] is syntactically invalid and will fail to compile when CHECK_DELTA_OBJ is defined.
Suggested fix
f_t delta_obj_check = 0.0;
for (i_t k = 0; k < delta_y.i.size(); k++) {
- delta_obj_check += lp.rhs[delta_y.i[k]] * delta_y.lp_solution.x[k];
+ delta_obj_check += lp.rhs[delta_y.i[k]] * delta_y.x[k];
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/branch_and_bound/pseudo_costs.cpp` around lines 140 - 161, The debug
block wrongly accesses delta_y.lp_solution.x[k]; delta_y is a sparse_vector_t
with arrays .i and .x, so replace delta_y.lp_solution.x[k] with delta_y.x[k]
(keeping the loop over k < delta_y.i.size()) so the index/value pairs are read
correctly; leave the rest of the CHECK_DELTA_OBJ computation (uses
delta_z_indices, vstatus, delta_z, variable_j, delta_obj_check, step_length)
unchanged so the validity check compiles and compares delta_obj_check to
delta_obj as intended.
|
|
||
| if (std::isfinite(upper_bound)) { | ||
| child_settings.cut_off = upper_bound + settings.dual_tol; | ||
| } else { | ||
| child_settings.cut_off = 0; | ||
| for (i_t i = 0; i < original_lp.num_cols; ++i) { | ||
| if (original_lp.objective[i] < 0) { | ||
| child_settings.cut_off += original_lp.objective[i] * child_problem.upper[i]; | ||
| } else if (original_lp.objective[i] > 0) { | ||
| child_settings.cut_off += original_lp.objective[i] * child_problem.lower[i]; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Cutoff computation may produce invalid results with infinite variable bounds.
When upper_bound is not finite, the fallback cutoff computation (lines 301-307) multiplies objective coefficients by variable bounds. If objective[i] < 0 and child_problem.upper[i] is +inf, or objective[i] > 0 and child_problem.lower[i] is -inf, the resulting cutoff will be +/-inf, rendering it ineffective or incorrect.
Consider skipping variables with unbounded bounds in the sum, or only applying this fallback when all relevant bounds are finite.
Suggested fix
} else {
child_settings.cut_off = 0;
for (i_t i = 0; i < original_lp.num_cols; ++i) {
- if (original_lp.objective[i] < 0) {
+ if (original_lp.objective[i] < 0 && std::isfinite(child_problem.upper[i])) {
child_settings.cut_off += original_lp.objective[i] * child_problem.upper[i];
- } else if (original_lp.objective[i] > 0) {
+ } else if (original_lp.objective[i] > 0 && std::isfinite(child_problem.lower[i])) {
child_settings.cut_off += original_lp.objective[i] * child_problem.lower[i];
+ } else if (original_lp.objective[i] != 0 &&
+ (!std::isfinite(child_problem.upper[i]) || !std::isfinite(child_problem.lower[i]))) {
+ // Cannot compute valid cutoff with unbounded variables
+ child_settings.cut_off = std::numeric_limits<f_t>::infinity();
+ break;
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/branch_and_bound/pseudo_costs.cpp` around lines 296 - 308, The
fallback cutoff calculation when upper_bound is not finite can produce +/-inf by
multiplying objective coefficients with infinite variable bounds; update the
block that sets child_settings.cut_off (the else branch using
original_lp.objective, child_problem.upper and child_problem.lower) to ignore
terms where the corresponding child_problem.upper or child_problem.lower is not
finite (or only compute the fallback when all contributing bounds are finite),
summing only finite products and leaving child_settings.cut_off at a safe
default (or signaling no cutoff) if any required bound is infinite; ensure the
logic preserves the existing behavior when bounds are finite and still adds
settings.dual_tol when applicable.
| if (std::isfinite(upper_bound)) { | ||
| child_settings.cut_off = upper_bound + settings.dual_tol; | ||
| } else { | ||
| child_settings.cut_off = 0; | ||
| for (i_t i = 0; i < original_lp.num_cols; ++i) { | ||
| if (original_lp.objective[i] < 0) { | ||
| child_settings.cut_off += original_lp.objective[i] * child_problem.upper[i]; | ||
| } else if (original_lp.objective[i] > 0) { | ||
| child_settings.cut_off += original_lp.objective[i] * child_problem.lower[i]; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Same infinite-bounds issue in trial_branching cutoff computation.
This cutoff computation has the same potential issue as noted for strong_branch_helper (lines 296-308): multiplying objective coefficients by potentially infinite bounds will produce invalid cutoff values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/branch_and_bound/pseudo_costs.cpp` around lines 420 - 431, The cutoff
sum in the else branch of pseudo_costs.cpp may multiply objective coefficients
by infinite bounds; update the computation in the block that sets
child_settings.cut_off to check std::isfinite(child_problem.upper[i]) and
std::isfinite(child_problem.lower[i]) before doing the multiplication with
original_lp.objective[i]; skip terms with non-finite bounds, and if a skipped
term has a non-zero objective coefficient treat the cutoff as unbounded (set
child_settings.cut_off to +/-INFINITY based on the sign of
original_lp.objective[i]) or otherwise keep the finite accumulated sum, ensuring
no multiplication by non-finite values occurs (refer to child_settings.cut_off,
original_lp.objective, and child_problem.upper/lower).
We can use a single dual simplex pivot to estimate the objective change after branch up or down in a variable (#963), which can be later be used for ranking the unreliable variables in reliability branching. To minimize the cost of reliability branching, we only apply trial branching to the 100 best variables. Other variables are not considered.
Previously, the 100 unreliable variables were selected at random, which may not lead to best branching decision.
In this PR, we also apply a cutoff based on the objective coefficients when no incumbent is available.
Pending: MIPLIB benchmark and validation
Checklist