Exploit dual degeneracy to reduce the number of integer infeasibilities; add primal simplex - #1685
Exploit dual degeneracy to reduce the number of integer infeasibilities; add primal simplex#1685chris-maes wants to merge 20 commits into
Conversation
…up after dual simplex Fixed the following bugs that were causing primal simplex to cycle: 1) Swapped input/output arguments in b_solve() 2) Incorrectly setting variable status of leaving variable 3) Primal step length was not limited by bounds of entering variable. Also fixed a bug/typo where the basis was reorderd twice after factorization. Added code to switch to phase I if we loose primal feasibility, and switch back to phase II once feasibility is regained. Tested on NETLIB LPs. Only 2 LPs pilot87 and pilot_ja need primal simplex to remove perturbations at the end of the dual simplex solve. Tested on the 14 MIPLIB root relaxations that need primal simplex to remove perturbations at the end of the dual simplex solve.
…mates for root relaxation. Add initial perturbation parameter
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (3)
cpp/src/dual_simplex/primal.cpp (1)
671-697: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
compute_basic_primal_variableshere.Lines 671-697 rebuild
rhs = b - N*x_N, callbasis_update.b_solve, and scatterxBintox.compute_basic_primal_variablesat lines 371-399 performs exactly the same steps, including the same work-estimate terms. The later call sites at lines 800, 1065, and 1069 already use the helper. Calling the helper here keeps one implementation of the reconstruction and avoids future drift between the two copies.♻️ Proposed refactor
- std::vector<f_t> rhs = lp.rhs; - work_estimate += m; - // rhs = b - sum_{j : x_j = l_j} A(:, j) l(j) - sum_{j : x_j = u_j} A(:, j) * - // u(j) - for (i_t k = 0; k < n - m; ++k) { - const i_t j = nonbasic_list[k]; - const i_t col_start = lp.A.col_start[j]; - const i_t col_end = lp.A.col_start[j + 1]; - const f_t xj = x[j]; - for (i_t p = col_start; p < col_end; ++p) { - rhs[lp.A.i[p]] -= xj * lp.A.x[p]; - } - work_estimate += 3.0*(col_end - col_start); - } - work_estimate += 4 * (n - m); - - - std::vector<f_t> xB(m); - work_estimate += m; - - basis_update.b_solve(rhs, xB); - - for (i_t k = 0; k < m; ++k) { - const i_t j = basic_list[k]; - x[j] = xB[k]; - } - work_estimate += 3 * m; + compute_basic_primal_variables(lp, basis_update, basic_list, nonbasic_list, x, work_estimate);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/dual_simplex/primal.cpp` around lines 671 - 697, Replace the duplicated rhs construction, basis solve, and basic-variable scatter block with a call to compute_basic_primal_variables, passing the existing LP, basis/update, nonbasic and basic lists, x, and work_estimate arguments as required. Remove the redundant local rhs/xB logic while preserving the helper’s work-estimate accounting and resulting x values.cpp/src/dual_simplex/solve.hpp (1)
101-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe primal entry point drops work-limit support.
solve_linear_program_advancedandsolve_linear_program_with_advanced_basisaccept awork_limit_context_t*, soCUOPT_WORK_LIMITapplies to them. This declaration has no such parameter.primal_phase2andprimal_phase2_with_advanced_basisaccumulatework_estimateand then discard it, so a solve started with--method=4ignores the configured work limit.Add the
work_limit_context_t* work_unit_context = nullptrparameter and record the accumulatedwork_estimatethrough it, or state in a comment that the primal method does not honour the work limit while it is experimental.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/dual_simplex/solve.hpp` around lines 101 - 106, Extend solve_linear_program_with_primal with a work_limit_context_t* work_unit_context = nullptr parameter, then propagate it through primal_phase2 and primal_phase2_with_advanced_basis so their accumulated work_estimate is recorded and CUOPT_WORK_LIMIT is enforced. If work-limit support cannot be implemented, document at the primal entry point that the experimental method intentionally does not honor the limit.cpp/src/dual_simplex/phase2.cpp (1)
2377-2423: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDocument the stale caller state after
prepare_optimality.
basis_update_mpf_tandcsc_matrix_tuse value-owning members, so the factorization snapshot is safe.prepare_optimalitymutates the basis lists and statuses without refreshing the caller’s cached basis-indexed state. Document at all three call sites that the immediatebreakis required unlessbasic_mark,nonbasic_mark,nonbasic_end,Arow,delta_y_steepest_edge,squared_infeasibilities, andinfeasibility_indicesare rebuilt.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/dual_simplex/phase2.cpp` around lines 2377 - 2423, Document at each of the three call sites where prepare_optimality mutates basis lists and statuses that the caller’s cached basis-indexed state is stale; the immediate break is required unless basic_mark, nonbasic_mark, nonbasic_end, Arow, delta_y_steepest_edge, squared_infeasibilities, and infeasibility_indices are rebuilt. Add this explanation near the affected control flow without changing the existing snapshot or cleanup behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/include/cuopt/mathematical_optimization/constants.h`:
- Around line 195-196: Update the Cython SolverMethod enum to match the C++
method_t values: add Primal with value 4 and assign Unset value 5, preserving
all existing method mappings and ensuring both values convert correctly without
ValueError.
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 3249-3266: In branch_and_bound_t::check_for_dual_degeneracy and
the related reduced-LP construction, vstatus mapping, and candidate scan paths,
replace every duplicated std::abs(... ) <= 1e-10 check with one shared
zero-reduced-cost predicate. Define a static constexpr f_t zero_reduced_cost_tol
and a small helper in the appropriate branch_and_bound_t scope, then use it
consistently through the restore loop as well.
- Around line 3559-3569: Update pivot_out_integer_variables to avoid
settings_.log.printf on the worker-thread node path; use settings_.log.debug or
a caller-provided logger while preserving solve_node_lp’s logging behavior. Move
solution, basis, status, and basis_update copies until after candidate
construction and filtering, creating them only when at least one pivot candidate
remains.
- Around line 3611-3670: Remove the unused fast-candidate debug computation in
the candidate loop: eliminate the delta_x construction, slack validation via ok,
dense conversion, residual allocation, matrix-vector multiply, and associated
log. Preserve the candidate discovery and bound checks, or guard the entire
diagnostic path behind the existing compile-time debug mechanism such as
CHECK_SLACKS.
- Around line 3422-3470: Update the feasibility-pump loop around
primal_phase2_with_advanced_basis to use the remaining time budget when
assigning primal_settings.time_limit, rather than settings_.time_limit. After
the solve, immediately terminate the pump for TIME_LIMIT, CONCURRENT_LIMIT,
ITERATION_LIMIT, or NUMERICAL statuses; retain the existing OPTIMAL processing
and avoid additional solve attempts for these terminal outcomes.
- Around line 1646-1659: In the solve_node_lp flow shown, guard the node-level
pivot pass by returning or skipping pivot_out_integer_variables when
fractional_variables reports num_fractional == 0. Also add and honor an
appropriate setting to disable this node-level pass entirely, while preserving
the existing behavior when enabled and fractional variables exist.
- Around line 3482-3517: Update the solution nonbasic-variable loop after
get_basis_from_vstatus to iterate over nonbasic_list.size() rather than
lp.num_cols - lp.num_rows, and explicitly handle a non-empty superbasic_list
instead of relying only on the assert. Preserve the existing bound assignment
logic for each valid nonbasic variable and return or otherwise safely stop
before processing an inconsistent basis.
In `@cpp/src/dual_simplex/primal.cpp`:
- Line 1134: Update the iteration-limit check in
primal_phase2_with_advanced_basis to use a greater-than-or-equal comparison, so
calls entering with iter already above iter_limit return ITERATION_LIMIT before
falling through to NUMERICAL.
- Around line 776-781: At the top of the iteration loop in the dual-simplex
solve flow, add a guard that checks the elapsed time from start_time against
settings.time_limit and checks settings.concurrent_halt before calling
phase2_pricing. Return the corresponding TIME_LIMIT or CONCURRENT_LIMIT status
immediately when either condition is met, so the checks also cover continue
paths that do not increment iter.
In `@cpp/src/dual_simplex/solve.cpp`:
- Around line 64-76: Add an INFEASIBLE value to primal_status_t, update
primal_phase2_with_advanced_basis to return it when phase I converges with
positive residual infeasibility, and update map_primal_status_to_lp_status to
map it to lp_status_t::INFEASIBLE. Ensure PRIMAL_UNBOUNDED is emitted only from
phase 2, not when primal_ratio_test finds no blocking variable during phase I;
preserve the existing phase-2 unbounded behavior.
- Around line 769-832: Avoid publishing constructed zero-valued solution fields
for non-optimal results in the solve flow around primal_phase2 and the
subsequent uncrush/copy block. For every primal status other than OPTIMAL (while
preserving the existing CONCURRENT_LIMIT handling), return the mapped status
before uncrushing or copying objective and solution values, or otherwise mark
objectives unavailable consistently with the dual path. Keep the existing
optimal solution computation and propagation unchanged.
- Around line 79-108: Update initialize_slack_basis_vstatus to return whether a
complete basis was built, track covered rows, and select at most one singleton
column per row without requiring its scaled coefficient to equal ±1. At the
primal_phase2 call site, check the returned status and log the existing failure
message before returning NUMERICAL_ISSUES when fewer than m rows are covered,
rather than relying on the assert.
- Around line 343-346: Update the concurrent-halt assignment in the dual-simplex
solve path to require both settings.inside_mip and a terminal solve status
before setting *settings.concurrent_halt to 1. Exclude NUMERICAL, TIME_LIMIT,
ITERATION_LIMIT, and CUTOFF outcomes, while preserving the existing null-pointer
guard and logging behavior.
---
Nitpick comments:
In `@cpp/src/dual_simplex/phase2.cpp`:
- Around line 2377-2423: Document at each of the three call sites where
prepare_optimality mutates basis lists and statuses that the caller’s cached
basis-indexed state is stale; the immediate break is required unless basic_mark,
nonbasic_mark, nonbasic_end, Arow, delta_y_steepest_edge,
squared_infeasibilities, and infeasibility_indices are rebuilt. Add this
explanation near the affected control flow without changing the existing
snapshot or cleanup behavior.
In `@cpp/src/dual_simplex/primal.cpp`:
- Around line 671-697: Replace the duplicated rhs construction, basis solve, and
basic-variable scatter block with a call to compute_basic_primal_variables,
passing the existing LP, basis/update, nonbasic and basic lists, x, and
work_estimate arguments as required. Remove the redundant local rhs/xB logic
while preserving the helper’s work-estimate accounting and resulting x values.
In `@cpp/src/dual_simplex/solve.hpp`:
- Around line 101-106: Extend solve_linear_program_with_primal with a
work_limit_context_t* work_unit_context = nullptr parameter, then propagate it
through primal_phase2 and primal_phase2_with_advanced_basis so their accumulated
work_estimate is recorded and CUOPT_WORK_LIMIT is enforced. If work-limit
support cannot be implemented, document at the primal entry point that the
experimental method intentionally does not honor the limit.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ccb63a78-ab46-41c7-9f90-b698078576d6
📒 Files selected for processing (11)
cpp/include/cuopt/mathematical_optimization/constants.hcpp/include/cuopt/mathematical_optimization/pdlp/solver_settings.hppcpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/branch_and_bound.hppcpp/src/dual_simplex/phase2.cppcpp/src/dual_simplex/primal.cppcpp/src/dual_simplex/primal.hppcpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/solve.hppcpp/src/math_optimization/solver_settings.cucpp/src/pdlp/solve.cu
| void initialize_slack_basis_vstatus(const lp_problem_t<i_t, f_t>& lp, | ||
| std::vector<variable_status_t>& vstatus) | ||
| { | ||
| const i_t m = lp.num_rows; | ||
| const i_t n = lp.num_cols; | ||
| vstatus.resize(n); | ||
| for (i_t j = 0; j < n; ++j) { | ||
| if (lp.lower[j] == -inf && lp.upper[j] == inf) { | ||
| vstatus[j] = variable_status_t::NONBASIC_FREE; | ||
| } else if (std::abs(lp.upper[j] - lp.lower[j]) < 1e-12) { | ||
| vstatus[j] = variable_status_t::NONBASIC_FIXED; | ||
| } else if (lp.lower[j] > -inf) { | ||
| vstatus[j] = variable_status_t::NONBASIC_LOWER; | ||
| } else { | ||
| vstatus[j] = variable_status_t::NONBASIC_UPPER; | ||
| } | ||
| } | ||
| i_t num_basic = 0; | ||
| for (i_t j = n - 1; j >= 0; --j) { | ||
| const i_t col_start = lp.A.col_start[j]; | ||
| const i_t col_end = lp.A.col_start[j + 1]; | ||
| const i_t nz = col_end - col_start; | ||
| if (nz == 1 && std::abs(lp.A.x[col_start]) == 1.0) { | ||
| vstatus[j] = variable_status_t::BASIC; | ||
| num_basic++; | ||
| } | ||
| if (num_basic == m) { break; } | ||
| } | ||
| assert(num_basic == m); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
initialize_slack_basis_vstatus can build an invalid or incomplete basis.
Two defects:
-
The unit-coefficient test runs on the scaled problem. The caller applies
scaling(presolved_lp, settings, lp, column_scales, row_scales)at line 762 and then passeslphere. Row and column scaling changes a slack coefficient of ±1 into an arbitrary value, sostd::abs(lp.A.x[col_start]) == 1.0rejects columns that are slacks in the unscaled problem.num_basiccan stay far belowm. -
The loop never checks which row each singleton column covers. Two singleton columns can share a row. The selected set of
mcolumns is then singular, and one row has no basic column.
If num_basic < m, the assert at line 107 fires in debug builds. In release builds get_basis_from_vstatus inside primal_phase2 fills fewer than m entries of basic_list, and the nonbasic_list.size() == n - m assertion no longer holds. factorize_basis then reads uninitialized indices and indexes lp.A out of range.
Track row coverage explicitly, and select at most one singleton column per row. Compare the existing loop at lines 235-245: it is safe only because create_phase1_problem guarantees a full artificial identity.
🐛 Proposed fix: cover each row exactly once and drop the unit-coefficient test
template <typename i_t, typename f_t>
-void initialize_slack_basis_vstatus(const lp_problem_t<i_t, f_t>& lp,
+bool initialize_slack_basis_vstatus(const lp_problem_t<i_t, f_t>& lp,
std::vector<variable_status_t>& vstatus)
{
const i_t m = lp.num_rows;
const i_t n = lp.num_cols;
vstatus.resize(n);
for (i_t j = 0; j < n; ++j) {
if (lp.lower[j] == -inf && lp.upper[j] == inf) {
vstatus[j] = variable_status_t::NONBASIC_FREE;
} else if (std::abs(lp.upper[j] - lp.lower[j]) < 1e-12) {
vstatus[j] = variable_status_t::NONBASIC_FIXED;
} else if (lp.lower[j] > -inf) {
vstatus[j] = variable_status_t::NONBASIC_LOWER;
} else {
vstatus[j] = variable_status_t::NONBASIC_UPPER;
}
}
- i_t num_basic = 0;
- for (i_t j = n - 1; j >= 0; --j) {
- const i_t col_start = lp.A.col_start[j];
- const i_t col_end = lp.A.col_start[j + 1];
- const i_t nz = col_end - col_start;
- if (nz == 1 && std::abs(lp.A.x[col_start]) == 1.0) {
- vstatus[j] = variable_status_t::BASIC;
- num_basic++;
- }
- if (num_basic == m) { break; }
- }
- assert(num_basic == m);
+ // One basic column per row. A singleton column with a nonzero coefficient
+ // spans exactly its own row, so it is a valid basis column after scaling.
+ std::vector<i_t> row_covered(m, 0);
+ i_t num_basic = 0;
+ for (i_t j = n - 1; j >= 0 && num_basic < m; --j) {
+ const i_t col_start = lp.A.col_start[j];
+ const i_t col_end = lp.A.col_start[j + 1];
+ if (col_end - col_start != 1) { continue; }
+ if (lp.A.x[col_start] == 0.0) { continue; }
+ const i_t i = lp.A.i[col_start];
+ if (row_covered[i]) { continue; }
+ row_covered[i] = 1;
+ vstatus[j] = variable_status_t::BASIC;
+ num_basic++;
+ }
+ return num_basic == m;
}Then handle the failure at the call site instead of relying on assert:
std::vector<variable_status_t> vstatus;
if (!initialize_slack_basis_vstatus(lp, vstatus)) {
settings.log.printf("Primal simplex requires a full slack basis.\n");
return lp_status_t::NUMERICAL_ISSUES;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void initialize_slack_basis_vstatus(const lp_problem_t<i_t, f_t>& lp, | |
| std::vector<variable_status_t>& vstatus) | |
| { | |
| const i_t m = lp.num_rows; | |
| const i_t n = lp.num_cols; | |
| vstatus.resize(n); | |
| for (i_t j = 0; j < n; ++j) { | |
| if (lp.lower[j] == -inf && lp.upper[j] == inf) { | |
| vstatus[j] = variable_status_t::NONBASIC_FREE; | |
| } else if (std::abs(lp.upper[j] - lp.lower[j]) < 1e-12) { | |
| vstatus[j] = variable_status_t::NONBASIC_FIXED; | |
| } else if (lp.lower[j] > -inf) { | |
| vstatus[j] = variable_status_t::NONBASIC_LOWER; | |
| } else { | |
| vstatus[j] = variable_status_t::NONBASIC_UPPER; | |
| } | |
| } | |
| i_t num_basic = 0; | |
| for (i_t j = n - 1; j >= 0; --j) { | |
| const i_t col_start = lp.A.col_start[j]; | |
| const i_t col_end = lp.A.col_start[j + 1]; | |
| const i_t nz = col_end - col_start; | |
| if (nz == 1 && std::abs(lp.A.x[col_start]) == 1.0) { | |
| vstatus[j] = variable_status_t::BASIC; | |
| num_basic++; | |
| } | |
| if (num_basic == m) { break; } | |
| } | |
| assert(num_basic == m); | |
| } | |
| bool initialize_slack_basis_vstatus(const lp_problem_t<i_t, f_t>& lp, | |
| std::vector<variable_status_t>& vstatus) | |
| { | |
| const i_t m = lp.num_rows; | |
| const i_t n = lp.num_cols; | |
| vstatus.resize(n); | |
| for (i_t j = 0; j < n; ++j) { | |
| if (lp.lower[j] == -inf && lp.upper[j] == inf) { | |
| vstatus[j] = variable_status_t::NONBASIC_FREE; | |
| } else if (std::abs(lp.upper[j] - lp.lower[j]) < 1e-12) { | |
| vstatus[j] = variable_status_t::NONBASIC_FIXED; | |
| } else if (lp.lower[j] > -inf) { | |
| vstatus[j] = variable_status_t::NONBASIC_LOWER; | |
| } else { | |
| vstatus[j] = variable_status_t::NONBASIC_UPPER; | |
| } | |
| } | |
| // One basic column per row. A singleton column with a nonzero coefficient | |
| // spans exactly its own row, so it is a valid basis column after scaling. | |
| std::vector<i_t> row_covered(m, 0); | |
| i_t num_basic = 0; | |
| for (i_t j = n - 1; j >= 0 && num_basic < m; --j) { | |
| const i_t col_start = lp.A.col_start[j]; | |
| const i_t col_end = lp.A.col_start[j + 1]; | |
| if (col_end - col_start != 1) { continue; } | |
| if (lp.A.x[col_start] == 0.0) { continue; } | |
| const i_t i = lp.A.i[col_start]; | |
| if (row_covered[i]) { continue; } | |
| row_covered[i] = 1; | |
| vstatus[j] = variable_status_t::BASIC; | |
| num_basic++; | |
| } | |
| return num_basic == m; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/dual_simplex/solve.cpp` around lines 79 - 108, Update
initialize_slack_basis_vstatus to return whether a complete basis was built,
track covered rows, and select at most one singleton column per row without
requiring its scaled coefficient to equal ±1. At the primal_phase2 call site,
check the returned status and log the existing failure message before returning
NUMERICAL_ISSUES when fewer than m rows are covered, rather than relying on the
assert.
| if (settings.inside_mip && settings.concurrent_halt != nullptr) { | ||
| settings.log.printf("Setting concurrent halt to 1 inside_mip\n"); | ||
| *settings.concurrent_halt = 1; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Gate the concurrent-halt flag on a terminal status.
This block sets *settings.concurrent_halt = 1 for every status value, including NUMERICAL, TIME_LIMIT, ITERATION_LIMIT, and CUTOFF. The previous phase-2 logic that set the flag was removed from phase2.cpp. Cooperating solvers now stop even when this dual solve produced no usable answer, so a concurrent MIP root solve can lose a PDLP result that would have finished.
Set the flag only after the solve reaches a terminal outcome.
🐛 Proposed fix
- if (settings.inside_mip && settings.concurrent_halt != nullptr) {
+ if (settings.inside_mip && settings.concurrent_halt != nullptr &&
+ (status == dual_status_t::OPTIMAL || status == dual_status_t::DUAL_UNBOUNDED ||
+ status == dual_status_t::CUTOFF)) {
settings.log.printf("Setting concurrent halt to 1 inside_mip\n");
*settings.concurrent_halt = 1;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (settings.inside_mip && settings.concurrent_halt != nullptr) { | |
| settings.log.printf("Setting concurrent halt to 1 inside_mip\n"); | |
| *settings.concurrent_halt = 1; | |
| } | |
| if (settings.inside_mip && settings.concurrent_halt != nullptr && | |
| (status == dual_status_t::OPTIMAL || status == dual_status_t::DUAL_UNBOUNDED || | |
| status == dual_status_t::CUTOFF)) { | |
| settings.log.printf("Setting concurrent halt to 1 inside_mip\n"); | |
| *settings.concurrent_halt = 1; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/dual_simplex/solve.cpp` around lines 343 - 346, Update the
concurrent-halt assignment in the dual-simplex solve path to require both
settings.inside_mip and a terminal solve status before setting
*settings.concurrent_halt to 1. Exclude NUMERICAL, TIME_LIMIT, ITERATION_LIMIT,
and CUTOFF outcomes, while preserving the existing null-pointer guard and
logging behavior.
| const primal_status_t primal_status = | ||
| primal_phase2(2, start_time, lp, settings, vstatus, lp_solution, iter); | ||
| lp_solution.iterations = iter; | ||
| original_solution.iterations = iter; | ||
|
|
||
| if (primal_status == primal_status_t::CONCURRENT_LIMIT) { | ||
| solution.iterations = iter; | ||
| return lp_status_t::CONCURRENT_LIMIT; | ||
| } | ||
|
|
||
| if (primal_status == primal_status_t::OPTIMAL) { | ||
| lp_solution.objective = compute_objective(lp, lp_solution.x); | ||
| lp_solution.user_objective = compute_user_objective(lp, lp_solution.objective); | ||
|
|
||
| std::vector<f_t> residual = lp.rhs; | ||
| matrix_vector_multiply(lp.A, 1.0, lp_solution.x, -1.0, residual); | ||
| lp_solution.l2_primal_residual = vector_norm2<i_t, f_t>(residual); | ||
|
|
||
| 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, lp_solution.y, 1.0, dual_residual); | ||
| lp_solution.l2_dual_residual = vector_norm2<i_t, f_t>(dual_residual); | ||
|
|
||
| std::vector<f_t> unscaled_x(lp.num_cols); | ||
| std::vector<f_t> unscaled_y(lp.num_rows); | ||
| std::vector<f_t> unscaled_z(lp.num_cols); | ||
| unscale_solution<i_t, f_t>(column_scales, | ||
| row_scales, | ||
| lp_solution.x, | ||
| lp_solution.y, | ||
| lp_solution.z, | ||
| unscaled_x, | ||
| unscaled_y, | ||
| unscaled_z); | ||
| uncrush_solution(presolve_info, | ||
| settings, | ||
| original_lp, | ||
| unscaled_x, | ||
| unscaled_y, | ||
| unscaled_z, | ||
| original_solution.x, | ||
| original_solution.y, | ||
| original_solution.z); | ||
| original_solution.objective = lp_solution.objective; | ||
| original_solution.user_objective = lp_solution.user_objective; | ||
| original_solution.l2_primal_residual = lp_solution.l2_primal_residual; | ||
| original_solution.l2_dual_residual = lp_solution.l2_dual_residual; | ||
| } | ||
|
|
||
| uncrush_primal_solution(user_problem, original_lp, original_solution.x, solution.x); | ||
| uncrush_dual_solution(user_problem, | ||
| original_lp, | ||
| original_solution.y, | ||
| original_solution.z, | ||
| solution.y, | ||
| solution.z); | ||
| solution.objective = original_solution.objective; | ||
| solution.user_objective = original_solution.user_objective; | ||
| solution.iterations = original_solution.iterations; | ||
| solution.l2_primal_residual = original_solution.l2_primal_residual; | ||
| solution.l2_dual_residual = original_solution.l2_dual_residual; | ||
| return map_primal_status_to_lp_status(primal_status); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not publish a zeroed solution for a non-optimal primal status.
The block at lines 779-818 runs only for primal_status_t::OPTIMAL. For TIME_LIMIT, ITERATION_LIMIT, and NUMERICAL, original_solution.x, y, and z keep their constructed values, and objective and user_objective are never assigned. Lines 820-831 then uncrush that zero vector into solution and copy those objective fields. The caller run_primal in cpp/src/pdlp/solve.cu converts the returned solution for every status, so a time-limited primal solve reports a zero primal point with a zero objective as if it were a real iterate.
The dual path handles this differently: solve_linear_program_with_advanced_basis fills original_solution only on OPTIMAL, and it sets user_objective explicitly for the unbounded case.
Return early for the non-optimal statuses, or set the objective fields to a sentinel that marks them as unavailable.
🐛 Proposed fix
if (primal_status == primal_status_t::CONCURRENT_LIMIT) {
solution.iterations = iter;
return lp_status_t::CONCURRENT_LIMIT;
}
- if (primal_status == primal_status_t::OPTIMAL) {
+ if (primal_status != primal_status_t::OPTIMAL) {
+ // No verified iterate to report. Leave the solution vectors untouched.
+ solution.iterations = iter;
+ return map_primal_status_to_lp_status(primal_status);
+ }
+
+ {
lp_solution.objective = compute_objective(lp, lp_solution.x);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const primal_status_t primal_status = | |
| primal_phase2(2, start_time, lp, settings, vstatus, lp_solution, iter); | |
| lp_solution.iterations = iter; | |
| original_solution.iterations = iter; | |
| if (primal_status == primal_status_t::CONCURRENT_LIMIT) { | |
| solution.iterations = iter; | |
| return lp_status_t::CONCURRENT_LIMIT; | |
| } | |
| if (primal_status == primal_status_t::OPTIMAL) { | |
| lp_solution.objective = compute_objective(lp, lp_solution.x); | |
| lp_solution.user_objective = compute_user_objective(lp, lp_solution.objective); | |
| std::vector<f_t> residual = lp.rhs; | |
| matrix_vector_multiply(lp.A, 1.0, lp_solution.x, -1.0, residual); | |
| lp_solution.l2_primal_residual = vector_norm2<i_t, f_t>(residual); | |
| 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, lp_solution.y, 1.0, dual_residual); | |
| lp_solution.l2_dual_residual = vector_norm2<i_t, f_t>(dual_residual); | |
| std::vector<f_t> unscaled_x(lp.num_cols); | |
| std::vector<f_t> unscaled_y(lp.num_rows); | |
| std::vector<f_t> unscaled_z(lp.num_cols); | |
| unscale_solution<i_t, f_t>(column_scales, | |
| row_scales, | |
| lp_solution.x, | |
| lp_solution.y, | |
| lp_solution.z, | |
| unscaled_x, | |
| unscaled_y, | |
| unscaled_z); | |
| uncrush_solution(presolve_info, | |
| settings, | |
| original_lp, | |
| unscaled_x, | |
| unscaled_y, | |
| unscaled_z, | |
| original_solution.x, | |
| original_solution.y, | |
| original_solution.z); | |
| original_solution.objective = lp_solution.objective; | |
| original_solution.user_objective = lp_solution.user_objective; | |
| original_solution.l2_primal_residual = lp_solution.l2_primal_residual; | |
| original_solution.l2_dual_residual = lp_solution.l2_dual_residual; | |
| } | |
| uncrush_primal_solution(user_problem, original_lp, original_solution.x, solution.x); | |
| uncrush_dual_solution(user_problem, | |
| original_lp, | |
| original_solution.y, | |
| original_solution.z, | |
| solution.y, | |
| solution.z); | |
| solution.objective = original_solution.objective; | |
| solution.user_objective = original_solution.user_objective; | |
| solution.iterations = original_solution.iterations; | |
| solution.l2_primal_residual = original_solution.l2_primal_residual; | |
| solution.l2_dual_residual = original_solution.l2_dual_residual; | |
| return map_primal_status_to_lp_status(primal_status); | |
| const primal_status_t primal_status = | |
| primal_phase2(2, start_time, lp, settings, vstatus, lp_solution, iter); | |
| lp_solution.iterations = iter; | |
| original_solution.iterations = iter; | |
| if (primal_status == primal_status_t::CONCURRENT_LIMIT) { | |
| solution.iterations = iter; | |
| return lp_status_t::CONCURRENT_LIMIT; | |
| } | |
| if (primal_status != primal_status_t::OPTIMAL) { | |
| // No verified iterate to report. Leave the solution vectors untouched. | |
| solution.iterations = iter; | |
| return map_primal_status_to_lp_status(primal_status); | |
| } | |
| { | |
| lp_solution.objective = compute_objective(lp, lp_solution.x); | |
| lp_solution.user_objective = compute_user_objective(lp, lp_solution.objective); | |
| std::vector<f_t> residual = lp.rhs; | |
| matrix_vector_multiply(lp.A, 1.0, lp_solution.x, -1.0, residual); | |
| lp_solution.l2_primal_residual = vector_norm2<i_t, f_t>(residual); | |
| 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, lp_solution.y, 1.0, dual_residual); | |
| lp_solution.l2_dual_residual = vector_norm2<i_t, f_t>(dual_residual); | |
| std::vector<f_t> unscaled_x(lp.num_cols); | |
| std::vector<f_t> unscaled_y(lp.num_rows); | |
| std::vector<f_t> unscaled_z(lp.num_cols); | |
| unscale_solution<i_t, f_t>(column_scales, | |
| row_scales, | |
| lp_solution.x, | |
| lp_solution.y, | |
| lp_solution.z, | |
| unscaled_x, | |
| unscaled_y, | |
| unscaled_z); | |
| uncrush_solution(presolve_info, | |
| settings, | |
| original_lp, | |
| unscaled_x, | |
| unscaled_y, | |
| unscaled_z, | |
| original_solution.x, | |
| original_solution.y, | |
| original_solution.z); | |
| original_solution.objective = lp_solution.objective; | |
| original_solution.user_objective = lp_solution.user_objective; | |
| original_solution.l2_primal_residual = lp_solution.l2_primal_residual; | |
| original_solution.l2_dual_residual = lp_solution.l2_dual_residual; | |
| } | |
| uncrush_primal_solution(user_problem, original_lp, original_solution.x, solution.x); | |
| uncrush_dual_solution(user_problem, | |
| original_lp, | |
| original_solution.y, | |
| original_solution.z, | |
| solution.y, | |
| solution.z); | |
| solution.objective = original_solution.objective; | |
| solution.user_objective = original_solution.user_objective; | |
| solution.iterations = original_solution.iterations; | |
| solution.l2_primal_residual = original_solution.l2_primal_residual; | |
| solution.l2_dual_residual = original_solution.l2_dual_residual; | |
| return map_primal_status_to_lp_status(primal_status); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/dual_simplex/solve.cpp` around lines 769 - 832, Avoid publishing
constructed zero-valued solution fields for non-optimal results in the solve
flow around primal_phase2 and the subsequent uncrush/copy block. For every
primal status other than OPTIMAL (while preserving the existing CONCURRENT_LIMIT
handling), return the mapped status before uncrushing or copying objective and
solution values, or otherwise mark objectives unavailable consistently with the
dual path. Keep the existing optimal solution computation and propagation
unchanged.
|
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:
📝 WalkthroughWalkthroughChangesThe PR adds an experimental primal simplex method, public solver settings and APIs, work-estimate propagation, PDLP dispatch, and branch-and-bound integer-pivot and feasibility-pump processing. Primal simplex support
Branch-and-bound integration
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (7)
cpp/src/branch_and_bound/branch_and_bound.hpp (1)
346-349: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the append contract of
check_for_dual_degeneracy.The definition appends to
zero_reduced_costs_varsandzero_reduced_costs_vars_nonbasic_indexwithout clearing them first. Both current callers pass freshly declared vectors, so the behavior is correct today. State the contract here, or clear the vectors in the definition, so a future caller that reuses a buffer does not silently accumulate stale indices.The neighboring
apply_delta_x_for_integer_pivotdeclaration already documents its mutation contract; match that level of detail for the other three new methods.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/branch_and_bound/branch_and_bound.hpp` around lines 346 - 349, Document in the declaration of check_for_dual_degeneracy that zero_reduced_costs_vars and zero_reduced_costs_vars_nonbasic_index are output buffers whose values are appended without being cleared, requiring callers to provide empty or intentionally reusable buffers; match the mutation-contract detail used by apply_delta_x_for_integer_pivot.cpp/src/branch_and_bound/branch_and_bound.cpp (1)
3387-3393: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
reduced_edge_norms.
primal_phase2_with_advanced_basistakes no edge-norm argument (seecpp/src/dual_simplex/primal.hpp:45-59).reduced_edge_normsis allocated and filled withlp.num_colsreads ofedge_norms_, then discarded. Delete it, or pass it once the primal solver accepts steepest-edge norms.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 3387 - 3393, Remove the unused reduced_edge_norms allocation and population loop in the surrounding branch-and-bound code, including the reduced_col assignment. Leave edge_norms_ untouched since primal_phase2_with_advanced_basis does not accept or use edge-norm data.cpp/src/dual_simplex/primal.cpp (4)
663-665: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
incoming_xandincoming_vstatusare never read. Both copies costO(n)time and two allocations per call, and the accounted work at line 665 charges2nfor them. No later code uses either value. The caller incpp/src/dual_simplex/phase2.cpp(lines 2384-2390) takes its own snapshot for rollback, so the intended purpose appears unimplemented here.Either remove both copies, or use them to restore state on the non-
OPTIMALreturn paths at lines 984, 905, 1058, and 1136. Do you want me to open an issue to track the rollback behavior?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/dual_simplex/primal.cpp` around lines 663 - 665, Remove the unused incoming_x and incoming_vstatus copies from the relevant primal simplex routine, and remove the associated work_estimate += 2.0 * n accounting. Do not add rollback behavior here, since the caller already snapshots state and no code in this routine consumes these values.
777-780: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winInitialize
direction.phase2_pricingwritesdirectiononly when it selects a candidate. Lines 947 and 955 read it. Every current path that reaches line 936 hasentering_index != -1, so the value is defined today, but the invariant now spans three separate retry paths (lines 839, 886) that reuse the same variable. Initialize it to0so a future path cannot read an indeterminate value.- i_t direction; + i_t direction = 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/dual_simplex/primal.cpp` around lines 777 - 780, Initialize the direction variable to 0 at its declaration before the phase2_pricing call in the primal simplex flow. Keep the existing phase2_pricing and retry-path behavior unchanged while ensuring later reads of direction remain defined if no candidate is selected.
715-728: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
phaseparameter is ignored. Lines 724 and 727 overwritephaseunconditionally from the measured primal infeasibility, so the caller's argument has no effect.cpp/src/dual_simplex/phase2.cppline 2397 passes2, and the declaration incpp/src/dual_simplex/primal.hpppresentsphaseas an input.Remove the parameter, or honor it when the caller already knows the starting phase.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/dual_simplex/primal.cpp` around lines 715 - 728, Update the phase-selection logic in the primal routine to honor the incoming phase value instead of unconditionally overwriting it based on primal infeasibility. Preserve the existing phase 1 setup and logging when phase 1 is selected, and retain phase 2 behavior for callers passing phase 2; update the declaration and call sites consistently if removing the parameter instead.
431-431: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
settings.pivot_tolinstead of a hardcoded constant.simplex_solver_settings_texposespivot_tol(default1e-7), and the dual simplex ratio test reads it. This function hardcodes1e-8, so tuning the setting has no effect on the primal ratio test and the two ratio tests disagree on what counts as a usable pivot.- constexpr f_t pivot_tol = 1e-8; + const f_t pivot_tol = settings.pivot_tol;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/dual_simplex/primal.cpp` at line 431, In the primal ratio-test function containing the local pivot_tol declaration, replace the hardcoded 1e-8 value with settings.pivot_tol from simplex_solver_settings_t. Preserve the existing ratio-test logic while ensuring configured pivot tolerance controls primal pivot usability consistently with the dual simplex path.cpp/src/dual_simplex/phase2.cpp (1)
2335-2348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant
x,y,zparameters, or document that they must aliassol.prepare_optimalitynow mutatessolthrough the primal cleanup at lines 2397-2408, butxis still declaredconst std::vector<f_t>&. The function reads the cleaned values at line 2434 only because the caller bindsx,y, andztosol.x,sol.y, andsol.z(lines 2624-2626). Theconstqualifier hides that requirement. If any future caller passes a copy, the reported primal infeasibility silently describes the pre-cleanup point.Pass
solalone and usesol.x,sol.y,sol.zinside the function.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/dual_simplex/phase2.cpp` around lines 2335 - 2348, Update prepare_optimality to remove the redundant x, y, and z parameters, then use sol.x, sol.y, and sol.z for all corresponding reads inside the function. Update every caller, including the call near the existing sol.x/sol.y/sol.z bindings, to pass only sol and preserve the post-cleanup values used for optimality reporting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 3504-3522: Before calling basis_update.refactor_basis, copy
vstatus, basic_list, nonbasic_list, and basis_update; on every non-zero
refactor_status, restore all four from those snapshots before returning. Remove
the obsolete TODO while preserving the existing concurrent-halt, time-limit, and
logging behavior.
- Around line 3192-3199: Update dual_degenerate_feasibility_pump and
pivot_out_integer_variables to return their accumulated simplex-iteration counts
(iter and work_estimate), then add both returned values to
exploration_stats_.total_simplex_iters at the surrounding cut-pass call sites,
consistent with dual_phase2_with_advanced_basis. Preserve the existing algorithm
behavior while ensuring Iter/Node reporting and the
branch_and_bound_simplex_iteration_limit include this work.
- Around line 3645-3670: The refactorization failure paths in the pivot routine
must signal failure instead of returning as though the pivot succeeded. Update
the enclosing pivot method and its caller around recommend_refactor,
factorize_basis, and the vstatus_copy success check to return or propagate a
boolean failure result for concurrent halt, time limit, invalid rank, or
incomplete rank; ensure the caller stops the pivot pass and does not commit the
mutated basic_list, nonbasic_list, vstatus, or solution.x.
- Line 4101: Guard the work-rate calculation in the logging statement using
root_relax_elapsed_time so zero elapsed time cannot produce an inf or nan value;
retain the existing work-rate output for positive elapsed times and use a finite
fallback when the duration is zero.
- Around line 3309-3341: Derive the reduced column count by scanning all
lp.num_cols with the same BASIC-or-zero-reduced-cost predicate used when
populating A_reduced, rather than using lp.num_rows plus
zero_reduced_costs_vars.size(). Before constructing lp_reduced, compare this
count with the expected basic plus zero-reduced-cost total and return early on
mismatch, preventing out-of-bounds writes in the reduced-column arrays and
reduced_vstatus.
In `@cpp/src/dual_simplex/phase2.cpp`:
- Around line 2409-2426: Update the primal cleanup result handling in
primal_phase2_with_advanced_basis to distinguish TIME_LIMIT and CONCURRENT_LIMIT
from numerical failure: detect exhausted settings.time_limit or an asserted
*settings.concurrent_halt, return the corresponding limit status, and preserve
the existing state restoration for every non-OPTIMAL outcome. Ensure
dual_phase2_with_advanced_basis receives these statuses and maps them to
dual_status_t::TIME_LIMIT or dual_status_t::CONCURRENT_LIMIT instead of
reporting OPTIMAL.
- Around line 2674-2677: Update the phase-2 condition around
phase2::initial_perturbation so the documented initial_perturbation value -1
follows an automatic policy, such as enabling perturbation in phase 2 alongside
value 1. Preserve value 0 as disabled and keep the existing phase == 2 guard.
- Line 2607: Update the phase-2 horizon reporting flow around
record_work_sync_on_horizon so phase2_work_estimate remains cumulative and
caller-owned. Replace the resets at the reporting sites near lines 2883 and 3757
with a separate reported baseline or delta, preserving initialization work from
root_relax_work_estimate while reporting only newly accumulated work.
In `@cpp/src/dual_simplex/primal.cpp`:
- Around line 494-499: Update the ratio-selection logic in both the lower- and
upper-bound branches around basic_leaving, leaving_index, and current_dx so
near-ties do not assign a larger value to min_val. Keep min_val unchanged when
ratio is within the 1e-9 tie tolerance, while still updating the selected
leaving row and current_dx; only a strictly smaller ratio should replace
min_val.
In `@cpp/src/dual_simplex/simplex_solver_settings.hpp`:
- Line 170: Initialize initial_perturbation to -1 in simplex_solver_settings_t,
either alongside ordering(-1) in the constructor or via a default member
initializer, so default-constructed settings use automatic perturbation.
---
Nitpick comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 3387-3393: Remove the unused reduced_edge_norms allocation and
population loop in the surrounding branch-and-bound code, including the
reduced_col assignment. Leave edge_norms_ untouched since
primal_phase2_with_advanced_basis does not accept or use edge-norm data.
In `@cpp/src/branch_and_bound/branch_and_bound.hpp`:
- Around line 346-349: Document in the declaration of check_for_dual_degeneracy
that zero_reduced_costs_vars and zero_reduced_costs_vars_nonbasic_index are
output buffers whose values are appended without being cleared, requiring
callers to provide empty or intentionally reusable buffers; match the
mutation-contract detail used by apply_delta_x_for_integer_pivot.
In `@cpp/src/dual_simplex/phase2.cpp`:
- Around line 2335-2348: Update prepare_optimality to remove the redundant x, y,
and z parameters, then use sol.x, sol.y, and sol.z for all corresponding reads
inside the function. Update every caller, including the call near the existing
sol.x/sol.y/sol.z bindings, to pass only sol and preserve the post-cleanup
values used for optimality reporting.
In `@cpp/src/dual_simplex/primal.cpp`:
- Around line 663-665: Remove the unused incoming_x and incoming_vstatus copies
from the relevant primal simplex routine, and remove the associated
work_estimate += 2.0 * n accounting. Do not add rollback behavior here, since
the caller already snapshots state and no code in this routine consumes these
values.
- Around line 777-780: Initialize the direction variable to 0 at its declaration
before the phase2_pricing call in the primal simplex flow. Keep the existing
phase2_pricing and retry-path behavior unchanged while ensuring later reads of
direction remain defined if no candidate is selected.
- Around line 715-728: Update the phase-selection logic in the primal routine to
honor the incoming phase value instead of unconditionally overwriting it based
on primal infeasibility. Preserve the existing phase 1 setup and logging when
phase 1 is selected, and retain phase 2 behavior for callers passing phase 2;
update the declaration and call sites consistently if removing the parameter
instead.
- Line 431: In the primal ratio-test function containing the local pivot_tol
declaration, replace the hardcoded 1e-8 value with settings.pivot_tol from
simplex_solver_settings_t. Preserve the existing ratio-test logic while ensuring
configured pivot tolerance controls primal pivot usability consistently with the
dual simplex path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 23872084-8518-4198-a1f6-2d3c479d2ba8
📒 Files selected for processing (17)
cpp/include/cuopt/mathematical_optimization/constants.hcpp/include/cuopt/mathematical_optimization/pdlp/solver_settings.hppcpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/branch_and_bound.hppcpp/src/branch_and_bound/pseudo_costs.cppcpp/src/dual_simplex/basis_updates.cppcpp/src/dual_simplex/basis_updates.hppcpp/src/dual_simplex/crossover.cppcpp/src/dual_simplex/phase2.cppcpp/src/dual_simplex/phase2.hppcpp/src/dual_simplex/primal.cppcpp/src/dual_simplex/primal.hppcpp/src/dual_simplex/simplex_solver_settings.hppcpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/solve.hppcpp/src/math_optimization/solver_settings.cucpp/src/pdlp/solve.cu
| dual_degenerate_feasibility_pump(original_lp_, | ||
| basic_list, | ||
| nonbasic_list, | ||
| root_vstatus_, | ||
| root_relax_soln_, | ||
| basis_update, | ||
| num_fractional, | ||
| fractional); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Account for the pump's simplex iterations.
dual_degenerate_feasibility_pump runs up to max_pump_iter primal simplex solves and keeps the count in its local iter. That count is never added to exploration_stats_.total_simplex_iters. The same applies to pivot_out_integer_variables, which accumulates a local work_estimate only.
Two consequences: the reported Iter/Node value understates real work, and settings_.branch_and_bound_simplex_iteration_limit no longer bounds total simplex effort once the pump runs on every cut pass. Return the iteration count from both routines and accumulate it, as the surrounding cut-pass code already does for dual_phase2_with_advanced_basis at line 3140.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 3192 - 3199,
Update dual_degenerate_feasibility_pump and pivot_out_integer_variables to
return their accumulated simplex-iteration counts (iter and work_estimate), then
add both returned values to exploration_stats_.total_simplex_iters at the
surrounding cut-pass call sites, consistent with
dual_phase2_with_advanced_basis. Preserve the existing algorithm behavior while
ensuring Iter/Node reporting and the branch_and_bound_simplex_iteration_limit
include this work.
| const i_t refactor_status = basis_update.refactor_basis(lp.A, | ||
| settings_, | ||
| lp.lower, | ||
| lp.upper, | ||
| exploration_stats_.start_time, | ||
| basic_list, | ||
| nonbasic_list, | ||
| vstatus); | ||
| if (refactor_status == CONCURRENT_HALT_RETURN || refactor_status == TIME_LIMIT_RETURN) { | ||
| // TODO: On failure vstatus, basic_list, and nonbasic_list are in a bad state. | ||
| // We should save copies before the failure and restore them after the failure. | ||
| return; | ||
| } | ||
| if (refactor_status != 0) { | ||
| settings_.log.printf("Failed to refactor basis after dual degenerate feasibility pump. " | ||
| "%d deficient columns.\n", | ||
| refactor_status); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Save and restore the basis state before refactor_basis can fail.
The TODO on line 3513 describes the defect exactly. vstatus, basic_list, and nonbasic_list are already overwritten when refactor_basis returns non-zero. On every failure path this function returns with a basis that no longer matches basis_update, and the caller (root processing at line 4153 or do_cut_pass at line 3192) continues to use it.
Copy the three containers plus basis_update before the translation, and restore them on any non-zero refactor_status. Do you want me to open an issue to track this?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 3504 - 3522,
Before calling basis_update.refactor_basis, copy vstatus, basic_list,
nonbasic_list, and basis_update; on every non-zero refactor_status, restore all
four from those snapshots before returning. Remove the obsolete TODO while
preserving the existing concurrent-halt, time-limit, and logging behavior.
| if (recommend_refactor == 1) { | ||
| csc_matrix_t<i_t, f_t> L(m, m, 1); | ||
| csc_matrix_t<i_t, f_t> U(m, m, 1); | ||
| std::vector<i_t> pinv(m); | ||
| std::vector<i_t> p(m); | ||
| std::vector<i_t> q(m); | ||
| std::vector<i_t> deficient; | ||
| std::vector<i_t> slacks_needed; | ||
| f_t factorize_work_estimate = 0.0; | ||
| const i_t rank = factorize_basis(lp.A, | ||
| settings_, | ||
| basic_list, | ||
| exploration_stats_.start_time, | ||
| L, | ||
| U, | ||
| p, | ||
| pinv, | ||
| q, | ||
| deficient, | ||
| slacks_needed, | ||
| factorize_work_estimate); | ||
| if (rank == CONCURRENT_HALT_RETURN || rank == TIME_LIMIT_RETURN) { return; } | ||
| if (rank < 0 || rank != lp.num_rows) { return; } | ||
| simplex::reorder_basic_list(q, basic_list); | ||
| basis_update.reset(L, U, p); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not leave a degraded factorization in place when refactorization fails.
basis_update.update(...) has already been applied when recommend_refactor == 1. The three early returns on lines 3666-3667 then abandon the refactorization while keeping the mutated basic_list, nonbasic_list, vstatus, and solution.x.
The caller detects success only through vstatus_copy[entering_index] == variable_status_t::BASIC (line 3859). It therefore treats these failures as successful pivots and can commit the degraded factorization at lines 3922-3926 whenever the fractional count improved. Signal the failure to the caller, for example by returning a bool, and stop the pivot pass on failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 3645 - 3670, The
refactorization failure paths in the pivot routine must signal failure instead
of returning as though the pivot succeeded. Update the enclosing pivot method
and its caller around recommend_refactor, factorize_basis, and the vstatus_copy
success check to return or propagate a boolean failure result for concurrent
halt, time limit, invalid rank, or incomplete rank; ensure the caller stops the
pivot pass and does not commit the mutated basic_list, nonbasic_list, vstatus,
or solution.x.
| if (primal_status == primal_status_t::OPTIMAL) { | ||
| // z now prices the original objective, so no perturbation remains. | ||
| settings.log.printf("Primal cleanup successful. Iterations %d\n", iter - dual_iter); | ||
| perturbation = 0.0; | ||
| sol.objective = compute_objective(lp, sol.x); | ||
| sol.user_objective = compute_user_objective(lp, sol.objective); | ||
| } else { | ||
| // Restore the perturbed optimum; a partially pivoted basis is worse than | ||
| // the dual feasible point we started from. | ||
| settings.log.printf("Primal cleanup failed. Reporting the perturbed solution.\n"); | ||
| ft = saved_ft; | ||
| sol.x = saved_x; | ||
| sol.y = saved_y; | ||
| sol.z = saved_z; | ||
| vstatus = saved_vstatus; | ||
| basic_list = saved_basic_list; | ||
| nonbasic_list = saved_nonbasic_list; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate TIME_LIMIT and CONCURRENT_LIMIT from the primal cleanup. The else branch treats every non-OPTIMAL status as a numerical failure. It restores the perturbed point, and the caller then sets dual_status_t::OPTIMAL (line 3056). A cleanup pass that exhausts settings.time_limit, or that observes *settings.concurrent_halt == 1, therefore reports OPTIMAL.
The concurrent path in cpp/src/pdlp/solve.cu acts on OPTIMAL by setting *settings.concurrent_halt = 1, so a halt request is answered with a completion signal. The overrun is amplified because primal_phase2_with_advanced_basis has no per-iteration clock check.
Return the limit statuses to the caller so dual_phase2_with_advanced_basis can map them to dual_status_t::TIME_LIMIT and dual_status_t::CONCURRENT_LIMIT. Keep the state restore in all cases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/dual_simplex/phase2.cpp` around lines 2409 - 2426, Update the primal
cleanup result handling in primal_phase2_with_advanced_basis to distinguish
TIME_LIMIT and CONCURRENT_LIMIT from numerical failure: detect exhausted
settings.time_limit or an asserted *settings.concurrent_halt, return the
corresponding limit status, and preserve the existing state restoration for
every non-OPTIMAL outcome. Ensure dual_phase2_with_advanced_basis receives these
statuses and maps them to dual_status_t::TIME_LIMIT or
dual_status_t::CONCURRENT_LIMIT instead of reporting OPTIMAL.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cpp/src/branch_and_bound/branch_and_bound.cpp (2)
3480-3492: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the per-iteration pump logs to
debug.
dual_degenerate_feasibility_pumpruns once at the root and once per cut pass. Each run prints one construction line (line 3364), up tomax_pump_iterprogress lines (line 3480), and one summary line (line 3492).pivot_out_integer_variablesadds more at lines 3743, 3862, and 3921.settings_.log.printfwrites at the default verbosity, so a model with many cut passes gains dozens of new lines in normal output. Usesettings_.log.debugfor the per-iteration and per-candidate lines. Keep at most the final summary atprintf.♻️ Proposed logging change
- settings_.log.printf( - "Degenerate feasibility pump (%d/%d): primal work estimate %.2e, iter %d, fractional variables %d/%d. Time %.2f\n", pump_iter, max_pump_iter, primal_work_estimate, iter, num_fractional_reduced, num_fractional, toc(dual_degenerate_feasibility_pump_start_time)); + settings_.log.debug( + "Degenerate feasibility pump (%d/%d): primal work estimate %.2e, iter %d, fractional " + "variables %d/%d. Time %.2f\n", + pump_iter, + max_pump_iter, + primal_work_estimate, + iter, + num_fractional_reduced, + num_fractional, + toc(dual_degenerate_feasibility_pump_start_time));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 3480 - 3492, Change the pump construction, progress, and candidate log calls in dual_degenerate_feasibility_pump and pivot_out_integer_variables from settings_.log.printf to settings_.log.debug, while keeping only the final summary in dual_degenerate_feasibility_pump at printf.
3405-3405: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueVary the pump seed across calls, deterministically.
The pump constructs
rngfromsettings_.random_seedon every call. The root call and every cut-pass call therefore draw the same perturbation sequence. When the same stall repeats, the perturbation repeats too, so the stall-breaking logic at lines 3417-3427 loses effect. Mix a per-call counter into the seed so successive calls differ while the run stays reproducible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` at line 3405, Update the RNG initialization in the pump routine to mix a persistent per-call counter with settings_.random_seed, ensuring each successive call gets a distinct but deterministic seed; preserve reproducibility across runs and the existing random-seed behavior otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 3480-3492: Change the pump construction, progress, and candidate
log calls in dual_degenerate_feasibility_pump and pivot_out_integer_variables
from settings_.log.printf to settings_.log.debug, while keeping only the final
summary in dual_degenerate_feasibility_pump at printf.
- Line 3405: Update the RNG initialization in the pump routine to mix a
persistent per-call counter with settings_.random_seed, ensuring each successive
call gets a distinct but deterministic seed; preserve reproducibility across
runs and the existing random-seed behavior otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2ac41597-1477-4f32-a2b5-d677f03bd644
📒 Files selected for processing (17)
cpp/include/cuopt/mathematical_optimization/constants.hcpp/include/cuopt/mathematical_optimization/pdlp/solver_settings.hppcpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/branch_and_bound.hppcpp/src/branch_and_bound/pseudo_costs.cppcpp/src/dual_simplex/basis_updates.cppcpp/src/dual_simplex/basis_updates.hppcpp/src/dual_simplex/crossover.cppcpp/src/dual_simplex/phase2.cppcpp/src/dual_simplex/phase2.hppcpp/src/dual_simplex/primal.cppcpp/src/dual_simplex/primal.hppcpp/src/dual_simplex/simplex_solver_settings.hppcpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/solve.hppcpp/src/math_optimization/solver_settings.cucpp/src/pdlp/solve.cu
🚧 Files skipped from review as they are similar to previous changes (16)
- cpp/src/math_optimization/solver_settings.cu
- cpp/src/dual_simplex/simplex_solver_settings.hpp
- cpp/src/branch_and_bound/pseudo_costs.cpp
- cpp/src/dual_simplex/basis_updates.hpp
- cpp/src/dual_simplex/phase2.hpp
- cpp/src/dual_simplex/primal.hpp
- cpp/include/cuopt/mathematical_optimization/pdlp/solver_settings.hpp
- cpp/src/branch_and_bound/branch_and_bound.hpp
- cpp/include/cuopt/mathematical_optimization/constants.h
- cpp/src/dual_simplex/crossover.cpp
- cpp/src/dual_simplex/solve.cpp
- cpp/src/pdlp/solve.cu
- cpp/src/dual_simplex/basis_updates.cpp
- cpp/src/dual_simplex/solve.hpp
- cpp/src/dual_simplex/phase2.cpp
- cpp/src/dual_simplex/primal.cpp
chris-maes
added
non-breaking
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 (3)
cpp/src/branch_and_bound/branch_and_bound.cpp (3)
3288-3295: 🎯 Functional Correctness | 🟠 MajorInclude feasibility-pump iterations in the global budget.
dual_degenerate_feasibility_pumpaccumulatesiteracross up to ten primal solves but returns no count. The callers at Lines [3194-3201] and [4159-4166] therefore omit this work fromtotal_simplex_iters.Return the accumulated iteration count and add it at both call sites. Otherwise,
Iter/Nodereporting andbranch_and_bound_simplex_iteration_limitignore primal-simplex work.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 3288 - 3295, Update dual_degenerate_feasibility_pump to return its accumulated primal-solve iteration count, then add that returned value to total_simplex_iters at both callers in the branching flow. Preserve the existing feasibility-pump behavior while ensuring the global iteration budget and Iter/Node reporting include this work.
1641-1659: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCount iterations from recovery solves.
When the first solve returns
NUMERICAL, the second call starts a fresh simplex solve. The code adds onlynode_iterfrom the first call. It does not add the iterations recorded by the recovery solve.Add the recovery iteration count before updating
total_simplex_itersorlp_iters_this_dive. Otherwise, iteration reporting andbranch_and_bound_simplex_iteration_limitcan undercount actual work.Also applies to: 5074-5086, 5692-5702
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 1641 - 1659, Update the NUMERICAL recovery paths around solve_linear_program_with_advanced_basis, including the corresponding sections near the other reported locations, to capture and add the recovery solve’s simplex iteration count to the same per-node and per-dive counters as node_iter. Ensure total_simplex_iters and lp_iters_this_dive, where applicable, include both the initial and recovery solves before enforcing iteration limits.
3746-3748: 🚀 Performance & Scalability | 🟠 MajorDo not log node pivots through the shared solver log.
pivot_out_integer_variablesruns from the node path, wherelp_settings.set_log(false)disables simplex logging. Thesesettings_.log.printfcalls still write from worker threads, can interleave, and add output overhead. Use a caller-provided logger that honors the node logging setting, or guard these messages behind the node logging flag.Also applies to: 3865-3867, 3922-3925
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 3746 - 3748, Remove or gate the settings_.log.printf calls in pivot_out_integer_variables, including the messages near fast_candidates and the other noted call sites, so node pivots do not write through the shared solver log. Use the caller-provided logger that respects lp_settings.set_log(false), or condition each message on the node logging flag while preserving the existing diagnostics when node logging is enabled.
♻️ Duplicate comments (7)
cpp/src/branch_and_bound/branch_and_bound.cpp (4)
4107-4107: 🎯 Functional Correctness | 🟡 MinorGuard the root work-rate division.
root_relax_elapsed_timecan be zero for a trivial root solve. The division can then printinfornan. Use a finite fallback when the elapsed time is zero.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` at line 4107, Guard the work-rate calculation in the root relaxation logging statement so root_relax_elapsed_time equal to zero cannot produce inf or nan. Use a finite fallback rate for zero elapsed time while preserving the existing division for positive elapsed times.
3531-3539: 🩺 Stability & Availability | 🟠 MajorIterate over the actual nonbasic list.
assert(superbasic_list.empty())is not a release-mode guard. If the list is shorter thanlp.num_cols - lp.num_rows, the loop readsnonbasic_list[k]out of bounds.Handle a non-empty
superbasic_listexplicitly and iterate overnonbasic_list.size().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 3531 - 3539, Update the nonbasic-variable initialization loop to handle a non-empty superbasic_list explicitly instead of relying on assert(superbasic_list.empty()), and bound iteration by nonbasic_list.size() rather than lp.num_cols - lp.num_rows. Preserve the existing status-based assignment logic for each valid nonbasic_list entry.
3509-3526: 🗄️ Data Integrity & Integration | 🟠 MajorRestore candidate state when refactorization fails.
The pump can leave
vstatus,basic_list,nonbasic_list, andbasis_updatemutated afterrefactor_basisfails. The pivot path can also mutatesolutionand basis state beforefactorize_basisfails. The caller can then commit the corrupted copy when the fractional count improves.Snapshot all affected state before the mutation, restore it on every failure, or return an explicit failure that prevents commit.
Also applies to: 3648-3675
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 3509 - 3526, Update the refactorization and pivot paths around refactor_basis and factorize_basis to snapshot all candidate state they may mutate, including vstatus, basic_list, nonbasic_list, basis_update, solution, and related basis state, then restore the snapshots on every failure path. Alternatively propagate an explicit failure that prevents the caller from committing the candidate; ensure an improved fractional count cannot commit corrupted state.
3311-3343: 🩺 Stability & Availability | 🔴 CriticalSize the reduced LP from the actual selected columns.
nislp.num_rows + zero_reduced_costs_vars.size(), but the population loop selects every column satisfyingBASIC || abs(soln.z) <= 1e-10. If a superbasic or otherwise unlisted column passes that predicate,reduced_colexceedsnand the writes toA_reduced, bounds, or status arrays go out of bounds.Count columns with the same predicate used during population. Reject the construction when the count does not match the expected basis plus zero-reduced-cost columns.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 3311 - 3343, Size the reduced LP using the number of columns that satisfy the same BASIC-or-near-zero predicate used in the population loop, rather than relying on zero_reduced_costs_vars.size(). Before constructing or populating lp_reduced, validate that this count matches the expected lp.num_rows + zero_reduced_costs_vars.size() and reject the construction on mismatch. Update the related n/capacity handling while preserving the existing column population logic.cpp/src/dual_simplex/solve.cpp (3)
80-108: 🩺 Stability & Availability | 🔴 CriticalBuild a complete basis after scaling.
initialize_slack_basis_vstatuscounts singleton columns, but it does not track which row each column covers. Two singleton columns can select the same row and leave another row without a basic column. The assertion does not protect release builds.The helper also receives the scaled
lp. Verify that scaling preserves the exact+/-1slack-coefficient invariant before relying on the equality check. Track row coverage and handle failure before starting primal simplex.Based on learnings, exact
+/-1slack checks are valid only when the coefficient invariant is preserved by the preceding transformation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/dual_simplex/solve.cpp` around lines 80 - 108, Update initialize_slack_basis_vstatus to select a complete row-covering basis: verify that scaling preserves the exact ±1 singleton-coefficient invariant before using the check, track the row index covered by each candidate column, and only mark a column BASIC when its row is not already covered. Replace the assertion-only failure path with explicit handling that prevents primal simplex from starting unless all m rows have distinct basic columns.Source: Learnings
797-860: 🗄️ Data Integrity & Integration | 🟠 MajorDo not publish an unverified primal result.
For non-
OPTIMALstatuses,original_solution.x,y,z, and objective fields remain default constructed. Lines 848-860 then uncrush and copy those fields intosolution. Callers can receive a zero-valued solution forTIME_LIMIT,ITERATION_LIMIT, orNUMERICAL.Return the mapped status before uncrushing, or mark all solution fields unavailable for non-optimal statuses.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/dual_simplex/solve.cpp` around lines 797 - 860, Update the solve flow after primal_phase2 in the function containing primal_status so every non-OPTIMAL status returns map_primal_status_to_lp_status(primal_status) before uncrush_primal_solution and the subsequent solution-field copies. Keep the existing CONCURRENT_LIMIT handling intact, and only uncrush and publish primal/dual solution fields for OPTIMAL results.
351-353: 🩺 Stability & Availability | 🟠 MajorSet concurrent halt only for terminal results.
This block signals concurrent solvers for
NUMERICAL,TIME_LIMIT,ITERATION_LIMIT, andCUTOFFas well as successful results. A failed or incomplete solve can therefore stop a concurrent solver that could still produce a usable answer. Gate the flag on statuses that safely terminate the concurrent solve.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/dual_simplex/solve.cpp` around lines 351 - 353, Update the concurrent_halt assignment in the inside_mip handling block so it is set only when the solve status is a terminal result; exclude NUMERICAL, TIME_LIMIT, ITERATION_LIMIT, and CUTOFF statuses while preserving the existing logging and null-pointer guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 3442-3444: Update the call to
simplex::primal_phase2_with_advanced_basis near the primal_settings.time_limit
assignment to use a local tic() start time matching the remaining duration,
rather than exploration_stats_.start_time. Keep the computed remaining time
limit unchanged and ensure the phase-two routine compares elapsed local time
against that duration.
---
Outside diff comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 3288-3295: Update dual_degenerate_feasibility_pump to return its
accumulated primal-solve iteration count, then add that returned value to
total_simplex_iters at both callers in the branching flow. Preserve the existing
feasibility-pump behavior while ensuring the global iteration budget and
Iter/Node reporting include this work.
- Around line 1641-1659: Update the NUMERICAL recovery paths around
solve_linear_program_with_advanced_basis, including the corresponding sections
near the other reported locations, to capture and add the recovery solve’s
simplex iteration count to the same per-node and per-dive counters as node_iter.
Ensure total_simplex_iters and lp_iters_this_dive, where applicable, include
both the initial and recovery solves before enforcing iteration limits.
- Around line 3746-3748: Remove or gate the settings_.log.printf calls in
pivot_out_integer_variables, including the messages near fast_candidates and the
other noted call sites, so node pivots do not write through the shared solver
log. Use the caller-provided logger that respects lp_settings.set_log(false), or
condition each message on the node logging flag while preserving the existing
diagnostics when node logging is enabled.
---
Duplicate comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Line 4107: Guard the work-rate calculation in the root relaxation logging
statement so root_relax_elapsed_time equal to zero cannot produce inf or nan.
Use a finite fallback rate for zero elapsed time while preserving the existing
division for positive elapsed times.
- Around line 3531-3539: Update the nonbasic-variable initialization loop to
handle a non-empty superbasic_list explicitly instead of relying on
assert(superbasic_list.empty()), and bound iteration by nonbasic_list.size()
rather than lp.num_cols - lp.num_rows. Preserve the existing status-based
assignment logic for each valid nonbasic_list entry.
- Around line 3509-3526: Update the refactorization and pivot paths around
refactor_basis and factorize_basis to snapshot all candidate state they may
mutate, including vstatus, basic_list, nonbasic_list, basis_update, solution,
and related basis state, then restore the snapshots on every failure path.
Alternatively propagate an explicit failure that prevents the caller from
committing the candidate; ensure an improved fractional count cannot commit
corrupted state.
- Around line 3311-3343: Size the reduced LP using the number of columns that
satisfy the same BASIC-or-near-zero predicate used in the population loop,
rather than relying on zero_reduced_costs_vars.size(). Before constructing or
populating lp_reduced, validate that this count matches the expected lp.num_rows
+ zero_reduced_costs_vars.size() and reject the construction on mismatch. Update
the related n/capacity handling while preserving the existing column population
logic.
In `@cpp/src/dual_simplex/solve.cpp`:
- Around line 80-108: Update initialize_slack_basis_vstatus to select a complete
row-covering basis: verify that scaling preserves the exact ±1
singleton-coefficient invariant before using the check, track the row index
covered by each candidate column, and only mark a column BASIC when its row is
not already covered. Replace the assertion-only failure path with explicit
handling that prevents primal simplex from starting unless all m rows have
distinct basic columns.
- Around line 797-860: Update the solve flow after primal_phase2 in the function
containing primal_status so every non-OPTIMAL status returns
map_primal_status_to_lp_status(primal_status) before uncrush_primal_solution and
the subsequent solution-field copies. Keep the existing CONCURRENT_LIMIT
handling intact, and only uncrush and publish primal/dual solution fields for
OPTIMAL results.
- Around line 351-353: Update the concurrent_halt assignment in the inside_mip
handling block so it is set only when the solve status is a terminal result;
exclude NUMERICAL, TIME_LIMIT, ITERATION_LIMIT, and CUTOFF statuses while
preserving the existing logging and null-pointer guard.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 606355a9-fb47-413a-963b-54318fed0484
📒 Files selected for processing (6)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/dual_simplex/primal.cppcpp/src/dual_simplex/primal.hppcpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/solve.hpppython/cuopt/cuopt/linear_programming/solver_settings/solver_settings.pyx
🚧 Files skipped from review as they are similar to previous changes (2)
- cpp/src/dual_simplex/solve.hpp
- cpp/src/dual_simplex/primal.cpp
| primal_settings.time_limit = settings_.time_limit - toc(exploration_stats_.start_time); | ||
| simplex::primal_status_t lp_status = simplex::primal_phase2_with_advanced_basis(2, | ||
| exploration_stats_.start_time, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -A120 -B20 'primal_phase2_with_advanced_basis' \
cpp/src/dual_simplex/primal.cpp cpp/src/dual_simplex/primal.hpp
rg -n -C5 'toc\(start_time\)|time_limit' cpp/src/dual_simplex/primal.cppRepository: NVIDIA/cuopt
Length of output: 22100
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- branch-and-bound caller ---'
sed -n '3400,3470p' cpp/src/branch_and_bound/branch_and_bound.cpp
printf '%s\n' '--- timer definitions and uses ---'
rg -n -C4 '\b(tic|toc)\s*\(' cpp/src | head -240
printf '%s\n' '--- primal settings time-limit callers ---'
rg -n -C8 'primal_settings\.time_limit|primal_phase2(_with_advanced_basis)?\(' cpp/srcRepository: NVIDIA/cuopt
Length of output: 34457
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- timer definitions ---'
rg -n -C8 'double\s+tic|f_t\s+tic|tic\s*\(\)|toc\s*\(' cpp include 2>/dev/null | head -220
printf '%s\n' '--- timing-contract probe ---'
python3 - <<'PY'
def primal_stops(global_elapsed, global_limit):
remaining = global_limit - global_elapsed
return global_elapsed > remaining
for elapsed, limit in [(1.0, 10.0), (5.1, 10.0), (10.1, 10.0)]:
print({
"global_elapsed": elapsed,
"global_limit": limit,
"remaining_limit": limit - elapsed,
"primal_time_check": primal_stops(elapsed, limit),
})
print("A local timer starts at zero for each pump call; its elapsed value is independent of global_elapsed.")
PYRepository: NVIDIA/cuopt
Length of output: 18214
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
def primal_time_limit_hit(global_elapsed, global_limit):
remaining_limit = global_limit - global_elapsed
# Equivalent to primal.cpp: toc(start_time) > settings.time_limit
return global_elapsed > remaining_limit
for elapsed, limit in ((1.0, 10.0), (5.1, 10.0), (10.1, 10.0)):
print(
f"global_elapsed={elapsed:.1f}, "
f"remaining_limit={limit - elapsed:.1f}, "
f"time_limit_hit={primal_time_limit_hit(elapsed, limit)}"
)
PY
printf '%s\n' '--- exact primal timeout check ---'
sed -n '1110,1136p' cpp/src/dual_simplex/primal.cpp
printf '%s\n' '--- exact caller ---'
sed -n '3438,3450p' cpp/src/branch_and_bound/branch_and_bound.cppRepository: NVIDIA/cuopt
Length of output: 2344
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
def primal_time_limit_hit(global_elapsed, global_limit):
remaining_limit = global_limit - global_elapsed
return global_elapsed > remaining_limit
for elapsed, limit in ((1.0, 10.0), (5.1, 10.0), (10.1, 10.0)):
print(
f"global_elapsed={elapsed:.1f}, "
f"remaining_limit={limit - elapsed:.1f}, "
f"time_limit_hit={primal_time_limit_hit(elapsed, limit)}"
)
PY
printf '%s\n' '--- exact primal timeout check ---'
sed -n '1110,1136p' cpp/src/dual_simplex/primal.cpp
printf '%s\n' '--- exact caller ---'
sed -n '3438,3450p' cpp/src/branch_and_bound/branch_and_bound.cppRepository: NVIDIA/cuopt
Length of output: 2344
Use a local start time for the remaining limit.
primal_phase2_with_advanced_basis compares toc(start_time) with settings.time_limit. This call passes the global B&B start time with a remaining duration, so the pump can stop early when elapsed time exceeds the remaining duration. Pass a local tic() value, or pass an absolute limit with the global start time.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 3442 - 3444,
Update the call to simplex::primal_phase2_with_advanced_basis near the
primal_settings.time_limit assignment to use a local tic() start time matching
the remaining duration, rather than exploration_stats_.start_time. Keep the
computed remaining time limit unchanged and ensure the phase-two routine
compares elapsed local time against that duration.
| 100.0 * dense_delta_z / (sparse_delta_z + dense_delta_z)); | ||
| ft.print_stats(); | ||
| } | ||
| if (settings.inside_mip == 1 && settings.concurrent_halt != nullptr) { |
There was a problem hiding this comment.
I think this needs to be restored. Not sure why it was deleted
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/dual_simplex/simplex_solver_settings.hpp (1)
175-175: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winResolve automatic perturbation before phase-two execution.
initial_perturbationdefaults to-1, but phase two callsinitial_perturbation()only when the value is1. Convert-1to the intended automatic choice before this check; otherwise automatic mode disables perturbation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/dual_simplex/simplex_solver_settings.hpp` at line 175, Update the phase-two setup around initial_perturbation() so the default value -1 is resolved to the intended automatic perturbation choice before checking whether the value is 1. Preserve explicit 0 as no perturbation and 1 as perturbation, ensuring the existing phase-two check behaves correctly for automatic mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cpp/src/dual_simplex/simplex_solver_settings.hpp`:
- Line 175: Update the phase-two setup around initial_perturbation() so the
default value -1 is resolved to the intended automatic perturbation choice
before checking whether the value is 1. Preserve explicit 0 as no perturbation
and 1 as perturbation, ensuring the existing phase-two check behaves correctly
for automatic mode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6c22f5ec-a5b4-42f8-bd5c-8e2dd383296c
📒 Files selected for processing (1)
cpp/src/dual_simplex/simplex_solver_settings.hpp
| primal_phase2(2, start_time, lp, settings, vstatus, solution, iter); | ||
| // TODO: We need to update ft if the basis changed | ||
| } | ||
| if (settings.inside_mip && settings.concurrent_halt != nullptr) { |
There was a problem hiding this comment.
Not sure why this got added here
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)
1641-1659: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAccount for scratch-solve iterations.
Each numerical-recovery path performs a second LP solve. Each path later accounts only for
node_iterfrom the failed dual solve. Addworker->leaf_solution.iterationstonode_iterafter successful recovery.
cpp/src/branch_and_bound/branch_and_bound.cpp#L1641-L1659: Add recovery iterations beforestats.total_simplex_iters += node_iter.cpp/src/branch_and_bound/branch_and_bound.cpp#L5074-L5095: Add recovery iterations before updatingexploration_stats_.total_simplex_iters.cpp/src/branch_and_bound/branch_and_bound.cpp#L5692-L5710: Add recovery iterations before updatingworker.lp_iters_this_dive.Otherwise, reported iterations and iteration limits exclude scratch recovery work. The cut-recovery path already applies this pattern at lines 3165-3169.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 1641 - 1659, The numerical-recovery scratch solves are not included in simplex iteration totals. In cpp/src/branch_and_bound/branch_and_bound.cpp lines 1641-1659, add worker->leaf_solution.iterations to node_iter after successful recovery and before stats.total_simplex_iters is updated; apply the same recovery-iteration accumulation at lines 5074-5095 before exploration_stats_.total_simplex_iters and at lines 5692-5710 before worker.lp_iters_this_dive. Preserve the existing cut-recovery pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 1641-1659: The numerical-recovery scratch solves are not included
in simplex iteration totals. In cpp/src/branch_and_bound/branch_and_bound.cpp
lines 1641-1659, add worker->leaf_solution.iterations to node_iter after
successful recovery and before stats.total_simplex_iters is updated; apply the
same recovery-iteration accumulation at lines 5074-5095 before
exploration_stats_.total_simplex_iters and at lines 5692-5710 before
worker.lp_iters_this_dive. Preserve the existing cut-recovery pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d8c6cfcd-fced-46a5-9db6-9d071364406b
📒 Files selected for processing (1)
cpp/src/branch_and_bound/branch_and_bound.cpp
| const f_t primal_tol = settings.primal_tol; | ||
| for (i_t j = 0; j < n; ++j) { | ||
| if (x[j] < lp.lower[j]) { | ||
| // x_j < l_j => -x_j > -l_j => -x_j + l_j > 0 |
| if (x[j] < lp.lower[j]) { | ||
| // x_j < l_j => -x_j > -l_j => -x_j + l_j > 0 | ||
| if (x[j] < lp.lower[j] - primal_tol) { | ||
| // x_j < l_j - tol => violation exceeds per-variable threshold |
| } | ||
| } | ||
| if (x[j] > lp.upper[j]) { | ||
| // x_j > u_j => x_j - u_j > 0 |
| if (x[j] > lp.upper[j]) { | ||
| // x_j > u_j => x_j - u_j > 0 | ||
| if (x[j] > lp.upper[j] + primal_tol) { | ||
| // x_j > u_j + tol => violation exceeds per-variable threshold |
| * PDLP: Use the PDLP method. | ||
| * DualSimplex: Use the dual simplex method. | ||
| * Barrier: Use the barrier method | ||
| * Primal: Use the (experimental) primal simplex method. |
There was a problem hiding this comment.
Remove experimental
| #define CUOPT_ELIMINATE_DENSE_COLUMNS "eliminate_dense_columns" | ||
| #define CUOPT_CUDSS_DETERMINISTIC "cudss_deterministic" | ||
| #define CUOPT_PRESOLVE "presolve" | ||
| #define CUOPT_INITIAL_PERTURBATION "initial_perturbation" |
There was a problem hiding this comment.
Make this more explicit (like CUOPT_INITIAL_PRIMAL_PERTURBATION) if its related to primal only.
| simplex_solver_settings_t<i_t, f_t> primal_settings = settings; | ||
| primal_settings.iteration_limit = std::numeric_limits<i_t>::max(); | ||
| primal_status_t primal_status = | ||
| primal_phase2(2, start_time, lp, primal_settings, vstatus, solution, primal_iter); |
There was a problem hiding this comment.
work estimates are not piped in here?
| case primal_status_t::CONCURRENT_LIMIT: return lp_status_t::CONCURRENT_LIMIT; | ||
| case primal_status_t::NUMERICAL: | ||
| case primal_status_t::NOT_LOADED: | ||
| default: return lp_status_t::NUMERICAL_ISSUES; |
There was a problem hiding this comment.
Do not implement the default case. That way, you will get a compilation error when a new status is added.
| } else if (settings.method == method_t::Barrier) { | ||
| return run_barrier(problem, settings, timer); | ||
| } else if (settings.method == method_t::Concurrent) { | ||
| return run_concurrent(problem, settings, timer, is_batch_mode); |
There was a problem hiding this comment.
Should we include primal simplex in concurrent?
| const i_t m = L0_.m; | ||
| // Scatter x into a dense workspace, compute U0 * x, gather back to sparse. | ||
| std::vector<f_t> x_dense; | ||
| x.to_dense(x_dense); |
There was a problem hiding this comment.
Is this intentional? why not just work with sparse version?
2 participants
This PR includes the following: