Add constraints and warm start dual simplex from current basis #599
Conversation
WalkthroughAdds a cutting-plane framework (Gomory/MIR) to dual simplex with cut pool/generation, LP integration (add/remove cuts), basis-update extensions, CSR/CSC utilities, branch-and-bound integration for cut passes, new solver settings, and related tests and build updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
chris-maes
added
feature request
chris-maes
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
cpp/src/dual_simplex/basis_updates.hpp (1)
294-295: New basis_update_mpf_t::append_cuts API looks consistentThe new
append_cutsdeclaration cleanly extends the basis update interface and matches the described usage fromsolve_linear_program_with_cuts. No header-level issues spotted; just ensure the implementation and callers clearly document/interpret the return code (0 vs error) consistently with otheri_t-returning methods.cpp/src/dual_simplex/sparse_matrix.hpp (1)
139-140: CSR append_rows API is reasonableAdding
csr_matrix_t::append_rowshere is a natural extension of the CSR interface and matches the intended use in the cut workflow. At the interface level it looks fine; just make sure callers only use it whenC.nmatchesthis->n(or clearly document if smallerC.nis intentionally supported).cpp/src/dual_simplex/sparse_vector.cpp (1)
31-44: CSR-row constructor is correct; consider adding a bounds assertThe implementation correctly builds a length‑
A.nsparse vector whose indices are the column indices from rowrowof the CSR matrix. To match existing defensive style (e.g., in other methods usingassertfor dimension checks), it would be helpful to add a guard:template <typename i_t, typename f_t> sparse_vector_t<i_t, f_t>::sparse_vector_t(const csr_matrix_t<i_t, f_t>& A, i_t row) { - const i_t row_start = A.row_start[row]; - const i_t row_end = A.row_start[row + 1]; + assert(row >= 0 && row + 1 <= A.m); + const i_t row_start = A.row_start[row]; + const i_t row_end = A.row_start[row + 1]; const i_t nz = row_end - row_start; n = A.n; i.reserve(nz); x.reserve(nz); for (i_t k = row_start; k < row_end; ++k) { i.push_back(A.j[k]); x.push_back(A.x[k]); } }cpp/src/dual_simplex/sparse_matrix.cpp (1)
360-400: append_rows implementation is sound; consider stricter dimension checksThe
append_rowslogic (row_start updates, contiguous copy ofC.j/C.x, andm/nz_maxupdates) looks correct and preserves CSR invariants for valid inputs.Two small improvements to consider:
- Enforce exact column compatibility instead of just
C.n > n:- if (C.n > n) { - return -1; - } + if (C.n != n) { + return -1; + }unless you explicitly intend to support
C.n < n.
- Document in the header that the function returns
0on success and-1on dimension mismatch, so callers know they must check the return value.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cpp/src/dual_simplex/basis_updates.cpp(1 hunks)cpp/src/dual_simplex/basis_updates.hpp(1 hunks)cpp/src/dual_simplex/solve.cpp(2 hunks)cpp/src/dual_simplex/solve.hpp(1 hunks)cpp/src/dual_simplex/sparse_matrix.cpp(1 hunks)cpp/src/dual_simplex/sparse_matrix.hpp(1 hunks)cpp/src/dual_simplex/sparse_vector.cpp(1 hunks)cpp/src/dual_simplex/sparse_vector.hpp(1 hunks)cpp/tests/dual_simplex/unit_tests/solve.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
cpp/src/dual_simplex/sparse_vector.cpp (2)
cpp/src/dual_simplex/sparse_vector.hpp (6)
sparse_vector_t(21-21)sparse_vector_t(23-23)sparse_vector_t(25-25)sparse_vector_t(27-27)sparse_vector_t(29-29)A(33-33)cpp/src/dual_simplex/sparse_matrix.hpp (4)
row(87-87)nz(78-78)x(70-70)x(74-74)
cpp/src/dual_simplex/sparse_vector.hpp (1)
cpp/src/dual_simplex/barrier.cu (2)
A(912-1172)A(912-917)
cpp/src/dual_simplex/sparse_matrix.cpp (1)
cpp/src/dual_simplex/sparse_matrix.hpp (1)
C(139-139)
cpp/tests/dual_simplex/unit_tests/solve.cpp (2)
cpp/src/dual_simplex/presolve.cpp (3)
convert_user_problem(773-1000)convert_user_problem(773-777)convert_user_problem(1558-1563)cpp/src/dual_simplex/solve.cpp (7)
basic_list(113-113)solve_linear_program_with_advanced_basis(128-294)solve_linear_program_with_advanced_basis(128-137)solve_linear_program_with_advanced_basis(803-812)solve_linear_program_with_cuts(297-444)solve_linear_program_with_cuts(297-307)solve_linear_program_with_cuts(814-825)
cpp/src/dual_simplex/solve.cpp (2)
cpp/src/dual_simplex/phase2.cpp (6)
settings(2119-2141)settings(2119-2119)basic_list(2176-2176)dual_phase2_with_advanced_basis(2197-2984)dual_phase2_with_advanced_basis(2197-2209)dual_phase2_with_advanced_basis(2999-3012)cpp/src/dual_simplex/phase2.hpp (1)
dual_phase2_with_advanced_basis(61-73)
cpp/src/dual_simplex/basis_updates.cpp (2)
cpp/src/dual_simplex/basis_updates.hpp (22)
cuts_basic(294-294)L0_(116-120)L0_(367-371)rhs(59-59)rhs(62-62)rhs(66-68)rhs(72-74)rhs(77-77)rhs(79-80)rhs(83-83)rhs(86-86)rhs(89-89)rhs(92-92)rhs(95-95)rhs(98-98)rhs(101-101)rhs(104-104)rhs(307-307)rhs(308-308)nz(149-149)nz(397-397)nz(411-411)cpp/src/dual_simplex/vector_math.cpp (3)
inverse_permutation(154-162)inverse_permutation(154-154)inverse_permutation(206-206)
🔇 Additional comments (1)
cpp/src/dual_simplex/sparse_vector.hpp (1)
28-29: CSR-row constructor declaration is consistentThe added constructor from a CSR row is a good complement to the existing CSC-column constructor and fits the class API. No issues at the declaration level.
| sparse_vector_t rhs(WT, h); | ||
| scatter_into_workspace(rhs); | ||
| i_t nz = rhs.i.size(); | ||
| for (i_t k = num_updates_ - 1; k >= 0; --k) { | ||
| // T_k^{-T} = ( I - v u^T/(1 + u^T v)) | ||
| // T_k^{-T} * b = b - v * (u^T * b) / (1 + u^T * v) = b - theta * v, theta = u^T b / mu | ||
|
|
||
| const i_t u_col = 2 * k; | ||
| const i_t v_col = 2 * k + 1; | ||
| const f_t mu = mu_values_[k]; | ||
|
|
||
| // dot = u^T * b | ||
| f_t dot = dot_product(u_col, xi_workspace_, x_workspace_); | ||
| const f_t theta = dot / mu; | ||
| if (std::abs(theta) > zero_tol) { | ||
| add_sparse_column(S_, v_col, -theta, xi_workspace_, nz, x_workspace_); | ||
| } | ||
| } | ||
| gather_into_sparse_vector(nz, rhs); | ||
| V_row.row_start[h] = V_nz; | ||
| for (i_t q = 0; q < rhs.i.size(); q++) { | ||
| V_row.j.push_back(rhs.i[q]); | ||
| V_row.x.push_back(rhs.x[q]); | ||
| V_nz++; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add template arguments to sparse_vector_t construction
@basis_update_mpf_t<i_t, f_t>::append_cuts won’t compile as written because sparse_vector_t is a class template—there’s no deduction guide for the (WT, h) constructor, so the compiler can’t infer i_t/f_t. Please instantiate the template explicitly.
- sparse_vector_t rhs(WT, h);
+ sparse_vector_t<i_t, f_t> rhs(WT, h);📝 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.
| sparse_vector_t rhs(WT, h); | |
| scatter_into_workspace(rhs); | |
| i_t nz = rhs.i.size(); | |
| for (i_t k = num_updates_ - 1; k >= 0; --k) { | |
| // T_k^{-T} = ( I - v u^T/(1 + u^T v)) | |
| // T_k^{-T} * b = b - v * (u^T * b) / (1 + u^T * v) = b - theta * v, theta = u^T b / mu | |
| const i_t u_col = 2 * k; | |
| const i_t v_col = 2 * k + 1; | |
| const f_t mu = mu_values_[k]; | |
| // dot = u^T * b | |
| f_t dot = dot_product(u_col, xi_workspace_, x_workspace_); | |
| const f_t theta = dot / mu; | |
| if (std::abs(theta) > zero_tol) { | |
| add_sparse_column(S_, v_col, -theta, xi_workspace_, nz, x_workspace_); | |
| } | |
| } | |
| gather_into_sparse_vector(nz, rhs); | |
| V_row.row_start[h] = V_nz; | |
| for (i_t q = 0; q < rhs.i.size(); q++) { | |
| V_row.j.push_back(rhs.i[q]); | |
| V_row.x.push_back(rhs.x[q]); | |
| V_nz++; | |
| } | |
| } | |
| sparse_vector_t<i_t, f_t> rhs(WT, h); | |
| scatter_into_workspace(rhs); | |
| i_t nz = rhs.i.size(); | |
| for (i_t k = num_updates_ - 1; k >= 0; --k) { | |
| // T_k^{-T} = ( I - v u^T/(1 + u^T v)) | |
| // T_k^{-T} * b = b - v * (u^T * b) / (1 + u^T * v) = b - theta * v, theta = u^T b / mu | |
| const i_t u_col = 2 * k; | |
| const i_t v_col = 2 * k + 1; | |
| const f_t mu = mu_values_[k]; | |
| // dot = u^T * b | |
| f_t dot = dot_product(u_col, xi_workspace_, x_workspace_); | |
| const f_t theta = dot / mu; | |
| if (std::abs(theta) > zero_tol) { | |
| add_sparse_column(S_, v_col, -theta, xi_workspace_, nz, x_workspace_); | |
| } | |
| } | |
| gather_into_sparse_vector(nz, rhs); | |
| V_row.row_start[h] = V_nz; | |
| for (i_t q = 0; q < rhs.i.size(); q++) { | |
| V_row.j.push_back(rhs.i[q]); | |
| V_row.x.push_back(rhs.x[q]); | |
| V_nz++; | |
| } | |
| } |
🤖 Prompt for AI Agents
In cpp/src/dual_simplex/basis_updates.cpp around lines 1149 to 1174, the
construction of sparse_vector_t using sparse_vector_t(WT, h) fails because
template parameters i_t and f_t cannot be deduced; explicitly instantiate the
template when constructing the object (e.g. sparse_vector_t<i_t, f_t> rhs(WT,
h)) so the compiler knows the element/index types, leaving the rest of the code
unchanged.
|
@chris-maes Aspirational for 25.12? |
|
This does not need to be part of 25.12. |
…eursitics and MIP presolve for experimentation
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
cpp/src/dual_simplex/basis_updates.cpp (1)
1172-1172: Add template arguments tosparse_vector_tconstruction.As previously noted,
sparse_vector_tis a class template and requires explicit template arguments since there's no deduction guide for the(WT, h)constructor.- sparse_vector_t rhs(WT, h); + sparse_vector_t<i_t, f_t> rhs(WT, h);
🧹 Nitpick comments (6)
cpp/src/dual_simplex/sparse_matrix.cpp (1)
402-427: Consider adding bounds validation for the sparse vector.Unlike
append_rows, this method doesn't validate that the sparse vector's indices are within the matrix's column bounds. Consider adding validation similar to line 367-369 inappend_rows:template <typename i_t, typename f_t> i_t csr_matrix_t<i_t, f_t>::append_row(const sparse_vector_t<i_t, f_t>& c) { + // Validate column indices are within bounds + for (i_t k = 0; k < c.i.size(); k++) { + if (c.i[k] >= this->n) { + return -1; + } + } const i_t old_m = this->m;Alternatively, if callers are trusted, an
assertwould suffice for debug builds.cpp/src/dual_simplex/branch_and_bound.cpp (3)
1131-1161: Debug logging should be gated by a compile-time flag or log level.These logging statements print detailed information about every integer variable and fractional variable. Consider wrapping them in a debug macro or using a debug log level.
if (num_fractional == 0) { +#ifdef PRINT_INTEGER_VARS for (i_t j = 0; j < original_lp_.num_cols; j++) { if (var_types_[j] == variable_type_t::INTEGER) { settings_.log.printf("Variable %d type %d val %e\n", j, var_types_[j], root_relax_soln_.x[j]); } } +#endif // ... rest of optimal case } else { - settings_.log.printf("Found %d fractional variables on cut pass %d\n", num_fractional, cut_pass); - for (i_t j: fractional) { - settings_.log.printf("Fractional variable %d lower %e value %e upper %e\n", j, original_lp_.lower[j], root_relax_soln_.x[j], original_lp_.upper[j]); - } + settings_.log.debug("Found %d fractional variables on cut pass %d\n", num_fractional, cut_pass);
1237-1249: Debug assertion withexit(1)should use assert or be removed.This validation block checks that B^T * u_bar = e_i. While important for debugging, using
exit(1)in production code is inappropriate.+#ifndef NDEBUG std::vector<f_t> BTu_bar(original_lp_.num_rows); b_transpose_multiply(original_lp_, basic_list, u_bar_dense, BTu_bar); for (i_t k = 0; k < original_lp_.num_rows; k++) { if (k == i) { - if (std::abs(BTu_bar[k] - 1.0) > 1e-6) { - settings_.log.printf("BTu_bar[%d] = %e i %d\n", k, BTu_bar[k], i); - exit(1); - } + assert(std::abs(BTu_bar[k] - 1.0) <= 1e-6 && "BTu_bar diagonal should be 1"); } else { - if (std::abs(BTu_bar[k]) > 1e-6) { - settings_.log.printf("BTu_bar[%d] = %e i %d\n", k, BTu_bar[k], i); - exit(1); - } + assert(std::abs(BTu_bar[k]) <= 1e-6 && "BTu_bar off-diagonal should be 0"); } } +#endif
1218-1218: Debug print should be gated.This debug print for cut generation should be wrapped in a debug macro.
- settings_.log.printf("Generating cut for variable %d relaxed value %e row %d\n", j, x_j, i); + settings_.log.debug("Generating cut for variable %d relaxed value %e row %d\n", j, x_j, i);cpp/src/dual_simplex/solve.cpp (2)
315-327: Consider removing or guarding debug verification code.This block performs a sanity check on the basis factorization and calls
exit(1)on failure. Hard process termination is inappropriate for library code - it prevents callers from handling errors gracefully. Consider:
- Converting to
CUOPT_ASSERTfor debug builds only- Returning an error status instead of terminating
- Wrapping with a
constexpr bool debug_verify = false;guard similar towrite_out_matlab+ constexpr bool verify_basis = false; + if (verify_basis) { csc_matrix_t<i_t, f_t> Btest(lp.num_rows, lp.num_rows, 1); basis_update.multiply_lu(Btest); csc_matrix_t<i_t, f_t> B(lp.num_rows, lp.num_rows, 1); form_b(lp.A, basic_list, B); csc_matrix_t<i_t, f_t> Diff(lp.num_rows, lp.num_rows, 1); add(Btest, B, 1.0, -1.0, Diff); const f_t err = Diff.norm1(); settings.log.printf("Before || B - L*U || %e\n", err); - if (err > 1e-6) { - exit(1); - } + CUOPT_ASSERT(err <= 1e-6); }
435-449: Second debug block should also be guarded.Same concern as the earlier verification block -
exit(1)is inappropriate for library code. Additionally, there's inconsistent indentation at line 448.// Check the basis update + if (verify_basis) { csc_matrix_t<i_t, f_t> Btest(lp.num_rows, lp.num_rows, 1); basis_update.multiply_lu(Btest); // ... if (err > 1e-6) { Diff.print_matrix(); - exit(1); + CUOPT_ASSERT(false); } + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cpp/src/dual_simplex/basis_updates.cpp(1 hunks)cpp/src/dual_simplex/branch_and_bound.cpp(4 hunks)cpp/src/dual_simplex/phase2.cpp(1 hunks)cpp/src/dual_simplex/solve.cpp(3 hunks)cpp/src/dual_simplex/sparse_matrix.cpp(1 hunks)cpp/src/dual_simplex/sparse_matrix.hpp(1 hunks)cpp/src/mip/solver.cu(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- cpp/src/dual_simplex/phase2.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-22T14:25:22.899Z
Learnt from: aliceb-nv
Repo: NVIDIA/cuopt PR: 527
File: cpp/src/mip/diversity/lns/rins.cu:167-175
Timestamp: 2025-10-22T14:25:22.899Z
Learning: In MIP (Mixed Integer Programming) problems in the cuOPT codebase, `n_integer_vars == 0` is impossible by definition—MIP problems must have at least one integer variable. If there are no integer variables, it would be a pure Linear Programming (LP) problem, not a MIP problem.
Applied to files:
cpp/src/mip/solver.cucpp/src/dual_simplex/branch_and_bound.cpp
🧬 Code graph analysis (3)
cpp/src/dual_simplex/sparse_matrix.cpp (1)
cpp/src/dual_simplex/sparse_matrix.hpp (2)
C(140-140)c(143-143)
cpp/src/mip/solver.cu (1)
cpp/tests/mip/elim_var_remap_test.cu (1)
sol(110-110)
cpp/src/dual_simplex/branch_and_bound.cpp (3)
cpp/src/dual_simplex/solve.cpp (16)
basic_list(114-114)solve_linear_program_with_advanced_basis(129-295)solve_linear_program_with_advanced_basis(129-138)solve_linear_program_with_advanced_basis(847-856)compute_user_objective(89-94)compute_user_objective(89-89)compute_user_objective(97-101)compute_user_objective(97-97)compute_user_objective(834-835)compute_user_objective(837-837)solve_linear_program_with_cuts(298-488)solve_linear_program_with_cuts(298-308)solve_linear_program_with_cuts(858-869)compute_objective(77-86)compute_objective(77-77)compute_objective(831-832)cpp/src/dual_simplex/solve.hpp (5)
solve_linear_program_with_advanced_basis(53-62)compute_user_objective(37-37)compute_user_objective(40-40)solve_linear_program_with_cuts(65-75)compute_objective(34-34)cpp/src/dual_simplex/presolve.hpp (1)
uncrush_primal_solution(160-163)
🔇 Additional comments (8)
cpp/src/dual_simplex/sparse_matrix.hpp (1)
139-143: LGTM!The new method declarations for
append_rowsandappend_rowfollow the existing class conventions and provide a clean API for extending CSR matrices with additional rows.cpp/src/dual_simplex/basis_updates.cpp (1)
1113-1315: Implementation ofappend_cutslooks structurally sound.The method correctly:
- Solves U^T W^T = C_B^T row-by-row
- Computes V from W using the inverse transforms
- Extends L and U with the appropriate block structure
- Updates permutations and workspace sizes
Note: Ensure the template argument fix at line 1172 is applied (see previous comment).
cpp/src/dual_simplex/sparse_matrix.cpp (1)
360-400: LGTM!The
append_rowsimplementation correctly:
- Validates that the input matrix has compatible column dimensions
- Resizes storage appropriately
- Updates row pointers and copies nonzero data
cpp/src/dual_simplex/branch_and_bound.cpp (1)
1070-1075: Settingscale_columns = falsemay affect numerical stability.Disabling column scaling could impact numerical stability for certain problem instances. Consider adding a comment explaining why scaling is disabled here, or making this configurable.
cpp/src/mip/solver.cu (1)
130-130: Debug code: LP-concurrent path is disabled.The
0 &&prefix disables the concurrent LP path when the problem is reduced to a pure LP.- if (0 && context.problem_ptr->n_integer_vars == 0) { + if (context.problem_ptr->n_integer_vars == 0) {⛔ Skipped due to learnings
Learnt from: aliceb-nv Repo: NVIDIA/cuopt PR: 527 File: cpp/src/mip/diversity/lns/rins.cu:167-175 Timestamp: 2025-10-22T14:25:22.899Z Learning: In MIP (Mixed Integer Programming) problems in the cuOPT codebase, `n_integer_vars == 0` is impossible by definition—MIP problems must have at least one integer variable. If there are no integer variables, it would be a pure Linear Programming (LP) problem, not a MIP problem.cpp/src/dual_simplex/solve.cpp (3)
11-11: LGTM!The new include for
basis_solves.hppis required to support theappend_cutsmethod used in the new function.
346-378: LGTM!The slack variable addition and LP dimension updates are implemented correctly. Each new cut constraint
C_i * x <= d_igets a slack variables_i >= 0such thatC_i * x + s_i = d_i, with identity columns appended to the constraint matrix.
858-869: LGTM!The explicit template instantiation is correctly specified and follows the existing pattern in the file.
| settings_.log.printf( | ||
| "Variable %d has negative lower bound %e\n", j, original_lp_.lower[j]); | ||
| exit(1); | ||
| } |
There was a problem hiding this comment.
Critical: exit(1) will terminate the application.
This exit(1) is not inside a debug block and will terminate the application if any variable has a negative lower bound. Consider throwing an exception or returning an error status instead.
if (original_lp_.lower[j] < 0) {
- settings_.log.printf(
- "Variable %d has negative lower bound %e\n", j, original_lp_.lower[j]);
- exit(1);
+ settings_.log.printf(
+ "Variable %d has negative lower bound %e. Skipping cut generation.\n", j, original_lp_.lower[j]);
+ break; // or handle appropriately
}📝 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.
| settings_.log.printf( | |
| "Variable %d has negative lower bound %e\n", j, original_lp_.lower[j]); | |
| exit(1); | |
| } | |
| settings_.log.printf( | |
| "Variable %d has negative lower bound %e. Skipping cut generation.\n", j, original_lp_.lower[j]); | |
| break; // or handle appropriately | |
| } |
🤖 Prompt for AI Agents
In cpp/src/dual_simplex/branch_and_bound.cpp around lines 1182 to 1185, the code
logs a negative lower bound and calls exit(1) which abruptly terminates the
process; replace this with non-fatal error handling by either throwing a
descriptive C++ exception (e.g., std::runtime_error with the same log message)
or returning/propagating an error status from the function so the caller can
handle it; ensure any created log entry remains but remove exit(1), and update
callers/signatures as needed to handle the thrown exception or returned error
code.
| if (Cx[k] <= cut_rhs[k]) { | ||
| lp_settings.log.printf("C*x <= d for cut %d\n", k); | ||
| exit(1); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: exit(1) will terminate the application.
This exit(1) is not inside a debug block and will terminate the application if cut validation fails. This should be handled gracefully.
if (Cx[k] <= cut_rhs[k]) {
- lp_settings.log.printf("C*x <= d for cut %d\n", k);
- exit(1);
+ lp_settings.log.printf("Warning: C*x <= d for cut %d, skipping this cut round\n", k);
+ // Handle the error condition appropriately
}📝 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 (Cx[k] <= cut_rhs[k]) { | |
| lp_settings.log.printf("C*x <= d for cut %d\n", k); | |
| exit(1); | |
| } | |
| } | |
| if (Cx[k] <= cut_rhs[k]) { | |
| lp_settings.log.printf("Warning: C*x <= d for cut %d, skipping this cut round\n", k); | |
| // Handle the error condition appropriately | |
| } | |
| } |
🤖 Prompt for AI Agents
In cpp/src/dual_simplex/branch_and_bound.cpp around lines 1467-1471, the code
calls exit(1) when a cut validation fails which will unconditionally terminate
the whole application; replace this with graceful error handling: log a clear
error including cut index and values, then propagate the failure to the caller
instead of exiting (e.g., return a boolean/error code, set an error status on
the object, or throw a std::runtime_error) so the caller can decide how to
handle the invalid cut; ensure any resources are not leaked and update the
callers to handle the propagated error if necessary.
| if (cut_status != lp_status_t::OPTIMAL) { | ||
| lp_settings.log.printf("Cut status %d\n", cut_status); | ||
| exit(1); | ||
| } |
There was a problem hiding this comment.
Critical: exit(1) will terminate the application.
This exit(1) is not inside a debug block and will terminate the application if the cut solve returns a non-optimal status. This should be handled gracefully, e.g., by breaking out of the cut loop or returning an appropriate status.
if (cut_status != lp_status_t::OPTIMAL) {
lp_settings.log.printf("Cut status %d\n", cut_status);
- exit(1);
+ break; // Exit cut generation loop and proceed with B&B
}📝 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 (cut_status != lp_status_t::OPTIMAL) { | |
| lp_settings.log.printf("Cut status %d\n", cut_status); | |
| exit(1); | |
| } | |
| if (cut_status != lp_status_t::OPTIMAL) { | |
| lp_settings.log.printf("Cut status %d\n", cut_status); | |
| break; // Exit cut generation loop and proceed with B&B | |
| } |
🤖 Prompt for AI Agents
In cpp/src/dual_simplex/branch_and_bound.cpp around lines 1490 to 1493, replace
the unconditional exit(1) when cut_status != lp_status_t::OPTIMAL with graceful
handling: log the status as you already do, then either break out of the current
cut/solve loop or propagate the non-optimal status to the caller (e.g., set a
return/overall status variable and return that status or throw a checked
exception) so the caller can decide next steps; do not terminate the process
directly.
cpp/src/dual_simplex/solve.cpp
Outdated
| lp_status_t lp_status; | ||
| if (status == dual::status_t::OPTIMAL) { lp_status = lp_status_t::OPTIMAL; } | ||
| if (status == dual::status_t::DUAL_UNBOUNDED) { lp_status = lp_status_t::INFEASIBLE; } | ||
| if (status == dual::status_t::TIME_LIMIT) { lp_status = lp_status_t::TIME_LIMIT; } | ||
| if (status == dual::status_t::ITERATION_LIMIT) { lp_status = lp_status_t::ITERATION_LIMIT; } | ||
| if (status == dual::status_t::CONCURRENT_LIMIT) { lp_status = lp_status_t::CONCURRENT_LIMIT; } | ||
| if (status == dual::status_t::NUMERICAL) { lp_status = lp_status_t::NUMERICAL_ISSUES; } | ||
| if (status == dual::status_t::CUTOFF) { lp_status = lp_status_t::CUTOFF; } | ||
| return lp_status; |
There was a problem hiding this comment.
Uninitialized lp_status leads to undefined behavior.
lp_status is declared without initialization. If status doesn't match any of the handled values, the function returns an indeterminate value, which is undefined behavior. Use a switch statement with a default case or initialize with a fallback value.
- lp_status_t lp_status;
- if (status == dual::status_t::OPTIMAL) { lp_status = lp_status_t::OPTIMAL; }
- if (status == dual::status_t::DUAL_UNBOUNDED) { lp_status = lp_status_t::INFEASIBLE; }
- if (status == dual::status_t::TIME_LIMIT) { lp_status = lp_status_t::TIME_LIMIT; }
- if (status == dual::status_t::ITERATION_LIMIT) { lp_status = lp_status_t::ITERATION_LIMIT; }
- if (status == dual::status_t::CONCURRENT_LIMIT) { lp_status = lp_status_t::CONCURRENT_LIMIT; }
- if (status == dual::status_t::NUMERICAL) { lp_status = lp_status_t::NUMERICAL_ISSUES; }
- if (status == dual::status_t::CUTOFF) { lp_status = lp_status_t::CUTOFF; }
+ lp_status_t lp_status = lp_status_t::NUMERICAL_ISSUES; // default fallback
+ switch (status) {
+ case dual::status_t::OPTIMAL: lp_status = lp_status_t::OPTIMAL; break;
+ case dual::status_t::DUAL_UNBOUNDED: lp_status = lp_status_t::INFEASIBLE; break;
+ case dual::status_t::TIME_LIMIT: lp_status = lp_status_t::TIME_LIMIT; break;
+ case dual::status_t::ITERATION_LIMIT: lp_status = lp_status_t::ITERATION_LIMIT; break;
+ case dual::status_t::CONCURRENT_LIMIT: lp_status = lp_status_t::CONCURRENT_LIMIT; break;
+ case dual::status_t::NUMERICAL: lp_status = lp_status_t::NUMERICAL_ISSUES; break;
+ case dual::status_t::CUTOFF: lp_status = lp_status_t::CUTOFF; break;
+ default: lp_status = lp_status_t::NUMERICAL_ISSUES; break;
+ }
return lp_status;🤖 Prompt for AI Agents
cpp/src/dual_simplex/solve.cpp lines 479-487: lp_status is declared
uninitialized and may be returned without being set; replace the chain of ifs
with a switch(status) that explicitly maps each dual::status_t case to the
corresponding lp_status_t and add a default case that assigns a sensible
fallback (e.g. lp_status_t::NUMERICAL_ISSUES or an appropriate "UNKNOWN"/error
status), and optionally log or assert on the default to catch unexpected enum
values.
| //bool presolve_success = dm.run_presolve(timer_.remaining_time()); | ||
| bool presolve_success = true; |
There was a problem hiding this comment.
Debug code: presolve is disabled.
The presolve step is commented out and presolve_success is hardcoded to true. This bypasses important infeasibility detection and optimization. This appears to be debug code that should be removed before merging.
- //bool presolve_success = dm.run_presolve(timer_.remaining_time());
- bool presolve_success = true;
+ bool presolve_success = dm.run_presolve(timer_.remaining_time());📝 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.
| //bool presolve_success = dm.run_presolve(timer_.remaining_time()); | |
| bool presolve_success = true; | |
| bool presolve_success = dm.run_presolve(timer_.remaining_time()); |
🤖 Prompt for AI Agents
In cpp/src/mip/solver.cu around lines 112-113, the presolve call was commented
out and presolve_success hardcoded to true; restore the original presolve
invocation and use its boolean result instead of the hardcoded true. Replace the
hardcoded assignment with the actual call (passing timer_.remaining_time() as
before) and propagate/check presolve_success as the code originally intended so
infeasibility and early exits are handled properly.
| return sol; | ||
| } | ||
| if (context.problem_ptr->empty) { | ||
| if (0 && context.problem_ptr->empty) { |
There was a problem hiding this comment.
Debug code: condition is always false.
The 0 && prefix makes this condition always evaluate to false, disabling the "problem fully reduced" check. This appears to be debug code.
- if (0 && context.problem_ptr->empty) {
+ if (context.problem_ptr->empty) {📝 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 (0 && context.problem_ptr->empty) { | |
| if (context.problem_ptr->empty) { |
🤖 Prompt for AI Agents
In cpp/src/mip/solver.cu around line 121, the condition `if (0 &&
context.problem_ptr->empty)` contains a debug `0 &&` that forces the branch to
never run; remove the `0 &&` so the check becomes `if
(context.problem_ptr->empty)` (or replace with an appropriate runtime flag) and
ensure any associated handling/logging for the "problem fully reduced" case
remains intact and compiles correctly.
cpp/src/mip/solver.cu
Outdated
| auto bb_status = branch_and_bound_status_future.get(); | ||
| exit(1); |
There was a problem hiding this comment.
Critical: exit(1) terminates the process unconditionally.
This exit(1) call will terminate the entire application immediately after the branch-and-bound solve completes. This is debug code that must be removed before merging.
Additionally, the code at line 235 calls branch_and_bound_status_future.get() again, which would be undefined behavior (double-get on a future) if the exit(1) weren't present.
- auto bb_status = branch_and_bound_status_future.get();
- exit(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.
| auto bb_status = branch_and_bound_status_future.get(); | |
| exit(1); |
🤖 Prompt for AI Agents
In cpp/src/mip/solver.cu around lines 228-229, remove the unconditional exit(1)
that was left from debugging; instead rely on the retrieved bb_status value and
ensure you do not call branch_and_bound_status_future.get() a second time (line
~235) — call get() only once, store its result in bb_status and reuse that
variable for subsequent logic, or check future.valid() before getting if
conditional behavior is needed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cpp/tests/dual_simplex/unit_tests/solve.cpp (1)
440-451: Setcuts.nz_maxand explicitly initialize all CSR fields for clarity.The memory safety issue from the previous review has been addressed by resizing
cuts.jandcuts.x. However, two issues remain:
Missing
cuts.nz_maxupdate: After resizing to 2 nonzeros,cuts.nz_maxshould be set to 2. It still holds the initial value of 1, which is inconsistent with the resized capacity.Implicit reliance on previous values: The code mutates the CSR matrix in place without explicitly setting all fields. Specifically:
cuts.x[0]is not set (relies on -1.0 from the first round)cuts.row_start[0]andcuts.row_start[1]are not set (rely on values from the first round)cut_rhs[0]is not set (relies on -1.0/3.0 from the first round)This makes the code harder to understand and maintain.
Apply this diff to make the initialization explicit:
cuts.row_start.resize(3); cuts.j.resize(2); cuts.x.resize(2); -// Add cut y >= 1/3 +cuts.m = 2; +cuts.nz_max = 2; +cuts.row_start[0] = 0; +cuts.row_start[1] = 1; +cuts.row_start[2] = 2; +// Row 0: -y <= -1/3 (or y >= 1/3) cuts.j[0] = 1; -cuts.row_start[2] = 2; -// Add cut x <= 0.0 +cuts.x[0] = -1.0; +// Row 1: x <= 0 cuts.j[1] = 0; cuts.x[1] = 1.0; -cuts.m = 2; + cut_rhs.resize(2); +cut_rhs[0] = -1.0 / 3.0; cut_rhs[1] = 0.0;
🧹 Nitpick comments (1)
cpp/tests/dual_simplex/unit_tests/solve.cpp (1)
465-468: Consider adding assertions for individual variable values.The test verifies the objective after applying cuts but doesn't check the individual variable values. Adding assertions would strengthen the test and catch potential issues with how cuts affect the solution.
Add these checks after line 467:
EXPECT_NEAR(solution.x[0], 0.0, 1e-6); EXPECT_NEAR(solution.x[1], 2.0 / 3.0, 1e-6); EXPECT_NEAR(solution.x[2], 1.0 / 3.0, 1e-6);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cpp/tests/dual_simplex/unit_tests/solve.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cpp/tests/dual_simplex/unit_tests/solve.cpp (3)
cpp/src/dual_simplex/presolve.cpp (3)
convert_user_problem(773-1000)convert_user_problem(773-777)convert_user_problem(1558-1563)cpp/src/dual_simplex/solve.cpp (7)
basic_list(114-114)solve_linear_program_with_advanced_basis(129-295)solve_linear_program_with_advanced_basis(129-138)solve_linear_program_with_advanced_basis(847-856)solve_linear_program_with_cuts(298-488)solve_linear_program_with_cuts(298-308)solve_linear_program_with_cuts(858-869)cpp/src/dual_simplex/solve.hpp (2)
solve_linear_program_with_advanced_basis(53-62)solve_linear_program_with_cuts(65-75)
🔇 Additional comments (2)
cpp/tests/dual_simplex/unit_tests/solve.cpp (2)
329-413: LGTM: Test setup and initial solve are correct.The LP formulation is clear, and the advanced-basis solve correctly produces the expected optimal solution (objective = 1.0, x = 1.0).
415-438: LGTM: First round of cuts is correctly implemented.The cut z >= 1/3 is properly reformulated as -z <= -1/3, and the expected objective of 4/3 is correct.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cpp/src/dual_simplex/solve.cpp (2)
422-422: Unreachable return statement and hard exit.The code calls
exit(1)followed by an unreachablereturn. Library code should not callexit(1)and should instead return an error status.This issue was previously flagged in past review comments.
479-487: Uninitializedlp_statusleads to undefined behavior.
lp_statusis declared without initialization. Ifstatusdoesn't match any of the handled values, the function returns an indeterminate value, which is undefined behavior.This critical issue was previously flagged in past review comments. The suggested fix is to use a switch statement with a default case or initialize with a fallback value.
🧹 Nitpick comments (2)
cpp/src/dual_simplex/solve.cpp (2)
315-327: Consider guarding debug code or returning error status instead of exit(1).The debug block verifying
B = L*Ucallsexit(1)on line 325, which terminates the entire process from library code. Consider either:
- Guarding this block with a compile-time debug flag
- Returning
lp_status_t::NUMERICAL_ISSUESinstead of callingexit(1)- Removing the block if it's temporary verification code
Apply this diff to return an error status instead:
if (err > 1e-6) { - exit(1); + settings.log.printf("ERROR: Initial basis check failed: || B - L*U || = %e\n", err); + return lp_status_t::NUMERICAL_ISSUES; }
435-449: Consider guarding debug code or returning error status instead of exit(1).Similar to the earlier debug block, this verification of
B = L*Uafter appending cuts callsexit(1)on line 448. Consider the same alternatives: guard with a compile-time flag, return an error status, or remove if temporary.Apply this diff to return an error status instead:
if (err > 1e-6) { Diff.print_matrix(); - exit(1); + settings.log.printf("ERROR: Post-cut basis check failed: || B - L*U || = %e\n", err); + return lp_status_t::NUMERICAL_ISSUES; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cpp/src/dual_simplex/solve.cpp(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cpp/src/dual_simplex/solve.cpp (2)
cpp/src/dual_simplex/basis_solves.cpp (3)
form_b(655-684)form_b(655-657)form_b(845-847)cpp/src/dual_simplex/phase2.hpp (1)
dual_phase2_with_advanced_basis(61-73)
🔇 Additional comments (3)
cpp/src/dual_simplex/solve.cpp (3)
11-11: LGTM!The
basis_solves.hppinclude is necessary for theform_bfunction used in the new cut-appending workflow.
858-869: LGTM!The explicit instantiation for
solve_linear_program_with_cuts<int, double>is correctly declared and follows the same pattern as other template instantiations in this compilation unit.
297-488: Solid implementation of cut-appending workflow.The overall logic correctly implements the cutting plane augmentation:
- Validates that cut constraints are provided consistently
- Converts cuts
C*x <= dto equality form by appending slack variables- Extends the constraint matrix and LP structures
- Constructs
C_B(cuts restricted to basic columns) for basis update- Warm-starts dual simplex from the augmented basis
The approach preserves dual feasibility by adding violated cuts with slack variables set as basic, which is the standard technique for incorporating cuts into simplex-based MIP solvers.
cpp/src/dual_simplex/solve.cpp
Outdated
| if (j >= lp.num_cols) { | ||
| settings.log.printf("j %d is greater than p %d\n", j, p); | ||
| exit(1); | ||
| } | ||
| C_col_degree[j]++; |
There was a problem hiding this comment.
Fix error message and replace exit(1) with error return.
Two issues here:
- The error message claims "j %d is greater than p %d" but the code checks
j >= lp.num_cols, notj >= p. This is confusing becauselp.num_colshas been updated toold_cols + pat line 371. exit(1)terminates the process from library code and should be replaced with an error return.
Apply this diff:
if (j >= lp.num_cols) {
- settings.log.printf("j %d is greater than p %d\n", j, p);
- exit(1);
+ settings.log.printf("Invalid column index j=%d exceeds lp.num_cols=%d\n", j, lp.num_cols);
+ return lp_status_t::NUMERICAL_ISSUES;
}🤖 Prompt for AI Agents
In cpp/src/dual_simplex/solve.cpp around lines 385-389, the error message and
termination behavior are wrong: change the printf to report the actual bound
being checked (use lp.num_cols, e.g. "j %d is >= lp.num_cols (%d)\n", j,
lp.num_cols) instead of referencing p, and replace the call to exit(1) with a
non-zero error return (e.g. return -1) or otherwise propagate an error according
to the function's return type so the library does not terminate the process.
There was a problem hiding this comment.
Actionable comments posted: 6
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/mip_node.hpp (1)
201-211: Missinginteger_infeasibleindetach_copy.The new
integer_infeasiblefield is not copied indetach_copy. This method creates a copy of the node for diving threads, and omitting this field means the copy will haveinteger_infeasible = -1instead of the actual value.mip_node_t<i_t, f_t> detach_copy() const { mip_node_t<i_t, f_t> copy(lower_bound, vstatus); copy.branch_var = branch_var; copy.branch_dir = branch_dir; copy.branch_var_lower = branch_var_lower; copy.branch_var_upper = branch_var_upper; copy.fractional_val = fractional_val; copy.node_id = node_id; + copy.integer_infeasible = integer_infeasible; return copy; }
♻️ Duplicate comments (10)
cpp/src/mip/solver.cu (3)
112-113: Debug code: presolve is disabled.This issue was previously flagged. The presolve step is commented out and
presolve_successis hardcoded totrue, bypassing important infeasibility detection. This must be restored before merging.
121-121: Debug code: condition is always false.This issue was previously flagged. The
0 &&prefix disables the "problem fully reduced" check. This must be removed before merging.
229-230: Critical:exit(1)terminates the process unconditionally.This issue was previously flagged. The
exit(1)call will terminate the entire application immediately after B&B solve completes. This must be removed before merging. Additionally, line 236 callsbranch_and_bound_status_future.get()again, which would be undefined behavior (double-get on a future) if theexit(1)weren't present.- auto bb_status = branch_and_bound_status_future.get(); - exit(1); -cpp/src/dual_simplex/solve.cpp (3)
387-390: Fix error message and replaceexit(1)with error return.The error message references
pbut the check is againstlp.num_cols. Also,exit(1)should not be used in library code. This was flagged in a previous review.if (j >= lp.num_cols) { - settings.log.printf("j %d is greater than p %d\n", j, p); - exit(1); + settings.log.printf("Invalid column index j=%d exceeds lp.num_cols=%d\n", j, lp.num_cols); + return lp_status_t::NUMERICAL_ISSUES; }
429-429: Removeexit(1)- the return after it is unreachable.The
exit(1)prevents the subsequent return from executing. This was flagged in a previous review.- if (nz != C_B_nz) { exit(1); return lp_status_t::NUMERICAL_ISSUES; } + if (nz != C_B_nz) { + settings.log.printf("C_B_nz mismatch: predicted %d, actual %d\n", C_B_nz, nz); + return lp_status_t::NUMERICAL_ISSUES; + }
488-496: Uninitializedlp_statusleads to undefined behavior.
lp_statusis declared without initialization. Ifstatusdoesn't match any handled value, the function returns an indeterminate value. This was flagged in a previous review.- lp_status_t lp_status; - if (status == dual::status_t::OPTIMAL) { lp_status = lp_status_t::OPTIMAL; } - if (status == dual::status_t::DUAL_UNBOUNDED) { lp_status = lp_status_t::INFEASIBLE; } - if (status == dual::status_t::TIME_LIMIT) { lp_status = lp_status_t::TIME_LIMIT; } - if (status == dual::status_t::ITERATION_LIMIT) { lp_status = lp_status_t::ITERATION_LIMIT; } - if (status == dual::status_t::CONCURRENT_LIMIT) { lp_status = lp_status_t::CONCURRENT_LIMIT; } - if (status == dual::status_t::NUMERICAL) { lp_status = lp_status_t::NUMERICAL_ISSUES; } - if (status == dual::status_t::CUTOFF) { lp_status = lp_status_t::CUTOFF; } + lp_status_t lp_status = lp_status_t::NUMERICAL_ISSUES; // default fallback + switch (status) { + case dual::status_t::OPTIMAL: lp_status = lp_status_t::OPTIMAL; break; + case dual::status_t::DUAL_UNBOUNDED: lp_status = lp_status_t::INFEASIBLE; break; + case dual::status_t::TIME_LIMIT: lp_status = lp_status_t::TIME_LIMIT; break; + case dual::status_t::ITERATION_LIMIT: lp_status = lp_status_t::ITERATION_LIMIT; break; + case dual::status_t::CONCURRENT_LIMIT: lp_status = lp_status_t::CONCURRENT_LIMIT; break; + case dual::status_t::NUMERICAL: lp_status = lp_status_t::NUMERICAL_ISSUES; break; + case dual::status_t::CUTOFF: lp_status = lp_status_t::CUTOFF; break; + default: break; // keeps NUMERICAL_ISSUES + } return lp_status;cpp/src/dual_simplex/basis_updates.cpp (1)
1172-1172: Add template arguments tosparse_vector_tconstruction
sparse_vector_tis a class template and requires explicit template parameters here. The compiler cannot deducei_t/f_tfrom the constructor arguments.- sparse_vector_t rhs(WT, h); + sparse_vector_t<i_t, f_t> rhs(WT, h);cpp/src/dual_simplex/branch_and_bound.cpp (3)
1183-1185: Critical:exit(1)will terminate the application.This
exit(1)is not inside a debug block and will terminate the application if any variable has a negative lower bound.if (original_lp_.lower[j] < 0) { - settings_.log.printf( - "Variable %d has negative lower bound %e\n", j, original_lp_.lower[j]); - exit(1); + settings_.log.printf( + "Variable %d has negative lower bound %e. Skipping cut generation.\n", j, original_lp_.lower[j]); + break; // Skip cut generation for this pass }
1512-1515: Critical:exit(1)will terminate the application.This
exit(1)is not inside a debug block and will terminate the application if cut validation fails.if (Cx[k] <= cut_rhs[k]) { - settings_.log.printf("C*x <= d for cut %d\n", k); - exit(1); + settings_.log.printf("Warning: Cut %d not violated (C*x=%e <= d=%e), skipping cut round\n", + k, Cx[k], cut_rhs[k]); + break; // Exit cut generation and proceed with B&B }
1541-1544: Critical:exit(1)will terminate the application.This
exit(1)is not inside a debug block and will terminate the application if the cut solve returns a non-optimal status.if (cut_status != lp_status_t::OPTIMAL) { lp_settings.log.printf("Cut status %d\n", cut_status); - exit(1); + break; // Exit cut generation loop and proceed with B&B }
🧹 Nitpick comments (2)
cpp/tests/dual_simplex/unit_tests/solve.cpp (1)
440-451: Consider making the second round of cuts more explicit for clarity.The CSR mutation relies on
cuts.x[0]retaining its value of-1.0from line 419. While the resize preserves existing values, this implicit behavior could confuse future maintainers. Consider explicitly settingcuts.x[0] = -1.0;in the second round for clarity:cuts.row_start.resize(3); cuts.j.resize(2); cuts.x.resize(2); // Add cut y >= 1/3 cuts.j[0] = 1; + cuts.x[0] = -1.0; // -y <= -1/3 cuts.row_start[2] = 2;cpp/src/dual_simplex/branch_and_bound.cpp (1)
1579-1669: Cut removal logic is complex but functionally correct.The cut removal workflow:
- Identifies inactive cuts by dual value tolerance
- Identifies associated slack variables
- Rebuilds matrices and arrays by filtering marked rows/columns
- Refactors the basis
However, the logic is quite involved. Consider extracting this into a helper method like
remove_inactive_cuts(...)for better modularity and testability.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cpp/include/cuopt/linear_programming/constants.h(1 hunks)cpp/include/cuopt/linear_programming/mip/solver_settings.hpp(1 hunks)cpp/src/dual_simplex/basis_updates.cpp(2 hunks)cpp/src/dual_simplex/branch_and_bound.cpp(10 hunks)cpp/src/dual_simplex/mip_node.hpp(6 hunks)cpp/src/dual_simplex/simplex_solver_settings.hpp(2 hunks)cpp/src/dual_simplex/solve.cpp(3 hunks)cpp/src/math_optimization/solver_settings.cu(1 hunks)cpp/src/mip/solver.cu(4 hunks)cpp/tests/dual_simplex/unit_tests/solve.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{cu,cuh,cpp,hpp,h}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.{cu,cuh,cpp,hpp,h}: Track GPU device memory allocations and deallocations to prevent memory leaks; ensure cudaMalloc/cudaFree balance and cleanup of streams/events
Validate algorithm correctness in optimization logic: simplex pivots, branch-and-bound decisions, routing heuristics, and constraint/objective handling must produce correct results
Check numerical stability: prevent overflow/underflow, precision loss, division by zero/near-zero, and use epsilon comparisons for floating-point equality checks
Validate correct initialization of variable bounds, constraint coefficients, and algorithm state before solving; ensure reset when transitioning between algorithm phases (presolve, simplex, diving, crossover)
Ensure variables and constraints are accessed from the correct problem context (original vs presolve vs folded vs postsolve); verify index mapping consistency across problem transformations
For concurrent CUDA operations (barriers, async operations), explicitly create and manage dedicated streams instead of reusing the default stream; document stream lifecycle
Eliminate unnecessary host-device synchronization (cudaDeviceSynchronize) in hot paths that blocks GPU pipeline; use streams and events for async execution
Assess algorithmic complexity for large-scale problems (millions of variables/constraints); ensure O(n log n) or better complexity, not O(n²) or worse
Verify correct problem size checks before expensive GPU/CPU operations; prevent resource exhaustion on oversized problems
Identify assertions with overly strict numerical tolerances that fail on legitimate degenerate/edge cases (near-zero pivots, singular matrices, empty problems)
Ensure race conditions are absent in multi-GPU code and multi-threaded server implementations; verify proper synchronization of shared state
Refactor code duplication in solver components (3+ occurrences) into shared utilities; for GPU kernels, use templated device functions to avoid duplication
Check that hard-coded GPU de...
Files:
cpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/include/cuopt/linear_programming/constants.hcpp/src/math_optimization/solver_settings.cucpp/src/dual_simplex/simplex_solver_settings.hppcpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/mip_node.hppcpp/src/dual_simplex/basis_updates.cppcpp/src/mip/solver.cucpp/tests/dual_simplex/unit_tests/solve.cppcpp/src/dual_simplex/branch_and_bound.cpp
**/*.{h,hpp,py}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
Verify C API does not break ABI stability (no struct layout changes, field reordering); maintain backward compatibility in Python and server APIs with deprecation warnings
Files:
cpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/include/cuopt/linear_programming/constants.hcpp/src/dual_simplex/simplex_solver_settings.hppcpp/src/dual_simplex/mip_node.hpp
**/*.{cpp,hpp,h}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.{cpp,hpp,h}: Check for unclosed file handles when reading MPS/QPS problem files; ensure RAII patterns or proper cleanup in exception paths
Validate input sanitization to prevent buffer overflows and resource exhaustion attacks; avoid unsafe deserialization of problem files
Prevent thread-unsafe use of global and static variables; use proper mutex/synchronization in server code accessing shared solver state
Files:
cpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/include/cuopt/linear_programming/constants.hcpp/src/dual_simplex/simplex_solver_settings.hppcpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/mip_node.hppcpp/src/dual_simplex/basis_updates.cppcpp/tests/dual_simplex/unit_tests/solve.cppcpp/src/dual_simplex/branch_and_bound.cpp
**/*.{cu,cpp,hpp,h}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
Avoid inappropriate use of exceptions in performance-critical GPU operation paths; prefer error codes or CUDA error checking for latency-sensitive code
Files:
cpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/include/cuopt/linear_programming/constants.hcpp/src/math_optimization/solver_settings.cucpp/src/dual_simplex/simplex_solver_settings.hppcpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/mip_node.hppcpp/src/dual_simplex/basis_updates.cppcpp/src/mip/solver.cucpp/tests/dual_simplex/unit_tests/solve.cppcpp/src/dual_simplex/branch_and_bound.cpp
**/*.{cu,cuh}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.{cu,cuh}: Every CUDA kernel launch and memory operation must have error checking with CUDA_CHECK or equivalent verification
Avoid reinventing functionality already available in Thrust, CCCL, or RMM libraries; prefer standard library utilities over custom implementations
Files:
cpp/src/math_optimization/solver_settings.cucpp/src/mip/solver.cu
**/*.cu
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.cu: Verify race conditions and correctness of GPU kernel shared memory, atomics, and warp-level operations
Detect inefficient GPU kernel launches with low occupancy or poor memory access patterns; optimize for coalesced memory access and minimize warp divergence in hot paths
Files:
cpp/src/math_optimization/solver_settings.cucpp/src/mip/solver.cu
🧠 Learnings (17)
📓 Common learnings
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.811Z
Learning: Reduce tight coupling between solver components (presolve, simplex, basis, barrier); increase modularity and reusability of optimization algorithms
📚 Learning: 2025-11-25T10:20:49.810Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.810Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Validate correct initialization of variable bounds, constraint coefficients, and algorithm state before solving; ensure reset when transitioning between algorithm phases (presolve, simplex, diving, crossover)
Applied to files:
cpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/src/math_optimization/solver_settings.cucpp/src/dual_simplex/simplex_solver_settings.hppcpp/src/dual_simplex/solve.cppcpp/src/mip/solver.cucpp/tests/dual_simplex/unit_tests/solve.cppcpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-11-25T10:20:49.810Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.810Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Validate algorithm correctness in optimization logic: simplex pivots, branch-and-bound decisions, routing heuristics, and constraint/objective handling must produce correct results
Applied to files:
cpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/include/cuopt/linear_programming/constants.hcpp/src/math_optimization/solver_settings.cucpp/src/dual_simplex/simplex_solver_settings.hppcpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/mip_node.hppcpp/src/dual_simplex/basis_updates.cppcpp/src/mip/solver.cucpp/tests/dual_simplex/unit_tests/solve.cppcpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-10-22T14:25:22.899Z
Learnt from: aliceb-nv
Repo: NVIDIA/cuopt PR: 527
File: cpp/src/mip/diversity/lns/rins.cu:167-175
Timestamp: 2025-10-22T14:25:22.899Z
Learning: In MIP (Mixed Integer Programming) problems in the cuOPT codebase, `n_integer_vars == 0` is impossible by definition—MIP problems must have at least one integer variable. If there are no integer variables, it would be a pure Linear Programming (LP) problem, not a MIP problem.
Applied to files:
cpp/include/cuopt/linear_programming/constants.hcpp/src/math_optimization/solver_settings.cucpp/src/dual_simplex/mip_node.hppcpp/src/mip/solver.cucpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-11-25T10:20:49.810Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.810Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Ensure variables and constraints are accessed from the correct problem context (original vs presolve vs folded vs postsolve); verify index mapping consistency across problem transformations
Applied to files:
cpp/include/cuopt/linear_programming/constants.hcpp/src/dual_simplex/solve.cppcpp/src/mip/solver.cucpp/tests/dual_simplex/unit_tests/solve.cppcpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-11-25T10:20:49.811Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.811Z
Learning: Applies to **/*test*.{cpp,cu,py} : Add tests for algorithm phase transitions: verify correct initialization of bounds and state when transitioning from presolve to simplex to diving to crossover
Applied to files:
cpp/src/dual_simplex/simplex_solver_settings.hppcpp/src/dual_simplex/solve.cppcpp/src/mip/solver.cucpp/tests/dual_simplex/unit_tests/solve.cppcpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-11-25T10:20:49.811Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.811Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Refactor code duplication in solver components (3+ occurrences) into shared utilities; for GPU kernels, use templated device functions to avoid duplication
Applied to files:
cpp/src/dual_simplex/solve.cppcpp/src/mip/solver.cucpp/tests/dual_simplex/unit_tests/solve.cppcpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-11-25T10:20:49.811Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.811Z
Learning: Reduce tight coupling between solver components (presolve, simplex, basis, barrier); increase modularity and reusability of optimization algorithms
Applied to files:
cpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/basis_updates.cppcpp/src/mip/solver.cucpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-11-25T10:20:49.811Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.811Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Assess algorithmic complexity for large-scale problems (millions of variables/constraints); ensure O(n log n) or better complexity, not O(n²) or worse
Applied to files:
cpp/src/dual_simplex/solve.cppcpp/src/mip/solver.cucpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-11-25T10:20:49.811Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.811Z
Learning: Applies to **/*test*.{cpp,cu,py} : Add tests for problem transformations: verify correctness of original→transformed→postsolve mappings and index consistency across problem representations
Applied to files:
cpp/src/dual_simplex/solve.cppcpp/tests/dual_simplex/unit_tests/solve.cpp
📚 Learning: 2025-11-25T10:20:49.811Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.811Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Verify correct problem size checks before expensive GPU/CPU operations; prevent resource exhaustion on oversized problems
Applied to files:
cpp/src/dual_simplex/solve.cppcpp/src/mip/solver.cucpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-11-25T10:20:49.811Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.811Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Identify assertions with overly strict numerical tolerances that fail on legitimate degenerate/edge cases (near-zero pivots, singular matrices, empty problems)
Applied to files:
cpp/src/dual_simplex/solve.cppcpp/src/mip/solver.cucpp/tests/dual_simplex/unit_tests/solve.cppcpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-11-25T10:20:49.811Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.811Z
Learning: Applies to **/*test*.{cpp,cu,py} : Test with free variables, singleton problems, and extreme problem dimensions near resource limits to validate edge case handling
Applied to files:
cpp/src/dual_simplex/solve.cppcpp/src/mip/solver.cucpp/tests/dual_simplex/unit_tests/solve.cppcpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-11-25T10:20:49.811Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.811Z
Learning: Applies to **/*test*.{cpp,cu,py} : Write tests validating numerical correctness of optimization results (not just 'runs without error'); test degenerate cases (infeasible, unbounded, empty, singleton problems)
Applied to files:
cpp/src/dual_simplex/solve.cppcpp/tests/dual_simplex/unit_tests/solve.cpp
📚 Learning: 2025-11-25T10:20:49.811Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.811Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Check that hard-coded GPU device IDs and resource limits are made configurable; abstract multi-backend support for different CUDA versions
Applied to files:
cpp/src/mip/solver.cu
📚 Learning: 2025-11-25T10:20:49.811Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.811Z
Learning: Applies to **/*.{cu,cpp,hpp,h} : Avoid inappropriate use of exceptions in performance-critical GPU operation paths; prefer error codes or CUDA error checking for latency-sensitive code
Applied to files:
cpp/src/mip/solver.cu
📚 Learning: 2025-11-25T10:20:49.810Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.810Z
Learning: Applies to **/*.{cpp,hpp,h} : Check for unclosed file handles when reading MPS/QPS problem files; ensure RAII patterns or proper cleanup in exception paths
Applied to files:
cpp/src/dual_simplex/branch_and_bound.cpp
🧬 Code graph analysis (2)
cpp/tests/dual_simplex/unit_tests/solve.cpp (4)
cpp/src/linear_programming/translate.hpp (1)
user_problem(23-23)cpp/src/dual_simplex/presolve.cpp (3)
convert_user_problem(773-1000)convert_user_problem(773-777)convert_user_problem(1558-1563)cpp/src/dual_simplex/solve.cpp (7)
basic_list(114-114)solve_linear_program_with_advanced_basis(129-295)solve_linear_program_with_advanced_basis(129-138)solve_linear_program_with_advanced_basis(856-865)solve_linear_program_with_cuts(298-497)solve_linear_program_with_cuts(298-308)solve_linear_program_with_cuts(867-878)cpp/src/dual_simplex/solve.hpp (2)
solve_linear_program_with_advanced_basis(53-62)solve_linear_program_with_cuts(65-75)
cpp/src/dual_simplex/branch_and_bound.cpp (3)
cpp/src/dual_simplex/solve.hpp (4)
solve_linear_program_with_advanced_basis(53-62)compute_user_objective(37-37)compute_user_objective(40-40)compute_objective(34-34)cpp/src/dual_simplex/presolve.hpp (1)
uncrush_primal_solution(160-163)cpp/src/dual_simplex/basis_solves.cpp (3)
b_transpose_multiply(709-727)b_transpose_multiply(709-712)b_transpose_multiply(854-857)
🔇 Additional comments (13)
cpp/include/cuopt/linear_programming/constants.h (1)
60-60: LGTM!The new
CUOPT_MIP_CUT_PASSESconstant follows the established naming convention and is placed logically alongside other MIP-related parameters.cpp/src/math_optimization/solver_settings.cu (1)
89-90: LGTM!The new
CUOPT_MIP_CUT_PASSESparameter is correctly wired tomip_settings.max_cut_passeswith sensible bounds. The-1lower bound follows the pattern of other parameters (e.g.,num_cpu_threads) for "automatic" behavior.cpp/src/dual_simplex/simplex_solver_settings.hpp (1)
72-72: LGTM!The
max_cut_passesfield is properly initialized to 10 in the constructor and correctly declared with a descriptive comment. This aligns with the same default inmip_solver_settings_t.Also applies to: 138-138
cpp/include/cuopt/linear_programming/mip/solver_settings.hpp (1)
84-84: LGTM!The public
max_cut_passesfield with default value 10 is consistent withsimplex_solver_settings_tand properly documented.cpp/tests/dual_simplex/unit_tests/solve.cpp (1)
329-412: Good test coverage for the cut-based LP solving workflow.The test properly validates numerical correctness of the optimization results at each stage (initial solve, after first cut, after second round of cuts), which aligns with best practices. The use of
solve_linear_program_with_advanced_basisfollowed by iterativesolve_linear_program_with_cutsexercises the warm-start dual simplex path effectively.cpp/src/mip/solver.cu (2)
171-171: LGTM: Proper wiring ofmax_cut_passesto B&B settings.The new
max_cut_passesparameter is correctly propagated from MIP settings to branch-and-bound settings.
130-130: Debug code: LP concurrent path is disabled.Similar to the other disabled conditions, the
0 &&prefix makes this branch unreachable, disabling the LP fallback for problems reduced to pure LP. Remove the0 &&guard.- if (0 && context.problem_ptr->n_integer_vars == 0) { + if (context.problem_ptr->n_integer_vars == 0) {⛔ Skipped due to learnings
Learnt from: aliceb-nv Repo: NVIDIA/cuopt PR: 527 File: cpp/src/mip/diversity/lns/rins.cu:167-175 Timestamp: 2025-10-22T14:25:22.899Z Learning: In MIP (Mixed Integer Programming) problems in the cuOPT codebase, `n_integer_vars == 0` is impossible by definition—MIP problems must have at least one integer variable. If there are no integer variables, it would be a pure Linear Programming (LP) problem, not a MIP problem.Learnt from: CR Repo: NVIDIA/cuopt PR: 0 File: .github/.coderabbit_review_guide.md:0-0 Timestamp: 2025-11-25T10:20:49.810Z Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Ensure variables and constraints are accessed from the correct problem context (original vs presolve vs folded vs postsolve); verify index mapping consistency across problem transformationsLearnt from: CR Repo: NVIDIA/cuopt PR: 0 File: .github/.coderabbit_review_guide.md:0-0 Timestamp: 2025-11-25T10:20:49.810Z Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Validate correct initialization of variable bounds, constraint coefficients, and algorithm state before solving; ensure reset when transitioning between algorithm phases (presolve, simplex, diving, crossover)Learnt from: CR Repo: NVIDIA/cuopt PR: 0 File: .github/.coderabbit_review_guide.md:0-0 Timestamp: 2025-11-25T10:20:49.810Z Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Validate algorithm correctness in optimization logic: simplex pivots, branch-and-bound decisions, routing heuristics, and constraint/objective handling must produce correct resultsLearnt from: CR Repo: NVIDIA/cuopt PR: 0 File: .github/.coderabbit_review_guide.md:0-0 Timestamp: 2025-11-25T10:20:49.811Z Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Identify assertions with overly strict numerical tolerances that fail on legitimate degenerate/edge cases (near-zero pivots, singular matrices, empty problems)cpp/src/dual_simplex/solve.cpp (1)
297-309: Newsolve_linear_program_with_cutsfunction and explicit instantiation look correct.The function properly augments the LP with cuts, updates the basis representation, and re-solves using dual simplex with warm start. The explicit template instantiation for
<int, double>ensures the function is available at link time.Also applies to: 867-878
cpp/src/dual_simplex/basis_updates.cpp (1)
1113-1315: LGTM: Well-structured MPF basis extension for cut handling.The
append_cutsimplementation correctly:
- Solves U^T W^T = C_B^T row-by-row to compute W
- Derives V via T_k transformations when updates exist
- Extends L as a block lower-triangular matrix with V in the (2,1) block
- Extends U with identity diagonal blocks
- Updates permutations and workspace sizes appropriately
The algorithm aligns with standard MPF update theory for constraint augmentation.
cpp/src/dual_simplex/mip_node.hpp (1)
36-49: LGTM: Proper initialization of newinteger_infeasiblefield.The field is correctly initialized to -1 in the default constructor and to the provided
integer_infparameter in the branching constructor. The updatedbranchmethod signature correctly threads this value through to child node creation.Also applies to: 51-76
cpp/src/dual_simplex/branch_and_bound.cpp (3)
1139-1168: LGTM: Early return when root solution is integer feasible.The logic correctly handles the case where
num_fractional == 0at any cut pass, setting the incumbent, returning OPTIMAL status, and invoking the solution callback. This is a valid optimization path.
1071-1079: LGTM: Root LP solve with advanced basis.The transition to
solve_linear_program_with_advanced_basiscorrectly initializes the basis update structure, basic/nonbasic lists, and threads them through to the solver. Settingscale_columns = falseis appropriate for maintaining basis consistency across cut passes.
668-669: LGTM: Updated branch call signature.The
branchcall now correctly passesleaf_num_fractionalas theinteger_infeasibleparameter, which is consistent with the updatedmip_node_tconstructor andsearch_tree_t::branchsignature.
| if (L_nz != predicted_nz) { | ||
| printf("L_nz %d predicted_nz %d\n", L_nz, predicted_nz); | ||
| exit(1); | ||
| } |
There was a problem hiding this comment.
Critical: exit(1) terminates the application on internal consistency check failure.
This exit(1) is not inside a debug block and will terminate the application if L_nz != predicted_nz. This should be handled gracefully.
if (L_nz != predicted_nz) {
- printf("L_nz %d predicted_nz %d\n", L_nz, predicted_nz);
- exit(1);
+ // Internal consistency check failed - this indicates a bug in append_cuts logic
+ return -1; // Signal error to caller
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cpp/src/dual_simplex/basis_updates.cpp around lines 1275-1278, the code
unconditionally prints an inconsistency and calls exit(1) when L_nz !=
predicted_nz; replace this unconditional termination with graceful error
handling: remove exit(1), log the detailed mismatch (including L_nz and
predicted_nz), set and return an error status (or throw a domain-specific
exception) so the caller can clean up and recover, and restrict any
program-aborting behavior to debug builds (e.g., assert or conditional abort
under #ifndef NDEBUG) while ensuring resources are freed and error codes
propagated in release builds.
| if (std::abs(BTu_bar[k] - 1.0) > 1e-6) { | ||
| settings_.log.printf("BTu_bar[%d] = %e i %d\n", k, BTu_bar[k], i); | ||
| exit(1); | ||
| } | ||
| } else { | ||
| if (std::abs(BTu_bar[k]) > 1e-6) { | ||
| settings_.log.printf("BTu_bar[%d] = %e i %d\n", k, BTu_bar[k], i); | ||
| exit(1); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Multiple exit(1) calls in validation code.
These exit(1) calls are not inside debug blocks and will terminate the application on validation failures. These appear to be assertion-like checks that should either be wrapped in debug macros or converted to graceful error handling.
for (i_t k = 0; k < original_lp_.num_rows; k++) {
if (k == i) {
if (std::abs(BTu_bar[k] - 1.0) > 1e-6) {
- settings_.log.printf("BTu_bar[%d] = %e i %d\n", k, BTu_bar[k], i);
- exit(1);
+ settings_.log.debug("BTu_bar validation failed: BTu_bar[%d] = %e, expected 1.0\n", k, BTu_bar[k]);
+ // Skip this cut due to numerical issues
+ continue;
}
} else {
if (std::abs(BTu_bar[k]) > 1e-6) {
- settings_.log.printf("BTu_bar[%d] = %e i %d\n", k, BTu_bar[k], i);
- exit(1);
+ settings_.log.debug("BTu_bar validation failed: BTu_bar[%d] = %e, expected 0.0\n", k, BTu_bar[k]);
}
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cpp/src/dual_simplex/branch_and_bound.cpp around lines 1263 to 1272, the code
calls exit(1) on validation failures which unconditionally terminates the
process; replace those exit(1) calls with graceful error handling by logging the
detailed failure (include BTu_bar[k], k and i) and then throwing a
std::runtime_error with a clear message so callers can handle the error, or
alternatively wrap the checks in a debug-only assert macro if these are intended
purely as developer-time asserts; do not leave unconditional exit(1) in
non-debug builds.
cpp/src/dual_simplex/solve.cpp
Outdated
| { | ||
| csc_matrix_t<i_t, f_t> Btest(lp.num_rows, lp.num_rows, 1); | ||
| basis_update.multiply_lu(Btest); | ||
| csc_matrix_t<i_t, f_t> B(lp.num_rows, lp.num_rows, 1); | ||
| form_b(lp.A, basic_list, B); | ||
| csc_matrix_t<i_t, f_t> Diff(lp.num_rows, lp.num_rows, 1); | ||
| add(Btest, B, 1.0, -1.0, Diff); | ||
| const f_t err = Diff.norm1(); | ||
| settings.log.printf("Before || B - L*U || %e\n", err); | ||
| if (err > 1e-6) { | ||
| exit(1); | ||
| } | ||
| } |
There was a problem hiding this comment.
Replace exit(1) with error return in debug validation block.
This debug validation of the basis factorization calls exit(1) on failure, which terminates the process from library code. Replace with an error return.
if (err > 1e-6) {
- exit(1);
+ settings.log.printf("Basis factorization error: || B - L*U || = %e exceeds tolerance\n", err);
+ return lp_status_t::NUMERICAL_ISSUES;
}🤖 Prompt for AI Agents
In cpp/src/dual_simplex/solve.cpp around lines 315–327, the debug validation
block currently calls exit(1) when ||B - L*U|| is too large; replace that
process-terminating call with an error return that propagates failure to the
caller (e.g., return a non-zero int or false or throw the project’s standard
exception type depending on the surrounding function signature). Log the error
message as before, remove exit(1), and ensure the function returns the chosen
error value so callers can handle the failure; update caller handling if
necessary to respect the new error path.
|
🔔 Hi @anandhkb @chris-maes, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
2 similar comments
|
🔔 Hi @anandhkb @chris-maes, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
|
🔔 Hi @anandhkb @chris-maes, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
cpp/src/mip/solver.cu (2)
112-113: Debug code: presolve is disabled.The presolve step is commented out and
presolve_successis hardcoded totrue. This bypasses important infeasibility detection and optimization. This should be restored before merging.- //bool presolve_success = dm.run_presolve(timer_.remaining_time()); - bool presolve_success = true; + bool presolve_success = dm.run_presolve(timer_.remaining_time());
121-121: Debug code: condition is always false.The
0 &&prefix makes this condition always evaluate tofalse, disabling the "problem fully reduced" check.- if (0 && context.problem_ptr->empty) { + if (context.problem_ptr->empty) {cpp/src/dual_simplex/branch_and_bound.cpp (2)
1245-1248: Replaceexit(1)with error return.Library code should not terminate the process when bound strengthening fails.
if (!feasible) { settings_.log.printf("Bound strengthening failed\n"); - exit(1); + return mip_status_t::INFEASIBLE; }
1279-1282: Replaceexit(1)with error return.Library code should not terminate the process on non-optimal cut status.
if (cut_status != dual::status_t::OPTIMAL) { settings_.log.printf("Cut status %d\n", cut_status); - exit(1); + // Break out of cut loop and proceed with B&B using current basis + break; }
🧹 Nitpick comments (14)
cpp/src/mip/diversity/lns/rins.cu (1)
248-248: Clarify the rationale for disabling cutting planes in RINS subproblems.The setting appears intentional and should function correctly. However, given that this PR introduces cutting plane infrastructure, it would be valuable to document why cutting planes are explicitly disabled for RINS subproblems. This could be due to performance considerations (keeping RINS fast), algorithmic simplicity, or avoiding interactions during initial development.
Consider adding an inline comment to document this design decision.
📝 Suggested documentation
+ // Disable cutting planes in RINS subproblems to keep them fast and simple branch_and_bound_settings.max_cut_passes = 0;cpp/src/mip/diversity/diversity_manager.cu (1)
412-412: Clarify intent: document or remove commented-out code.The
rins.enable()call has been commented out without explanation at line 412. If this is a temporary change for cutting planes integration or other development work, add a TODO comment with context. If RINS is permanently disabled, remove the line entirely rather than leaving it commented.cpp/src/dual_simplex/branch_and_bound.cpp (2)
1237-1241: Remove verbose diagnostic logging before merge.The "Before A check", "Before A transpose", "After A transpose" log messages appear to be development diagnostics. Consider removing or gating behind a debug flag.
🔎 Suggested fix
- settings_.log.printf("Before A check\n"); original_lp_.A.check_matrix(); - settings_.log.printf("Before A transpose\n"); original_lp_.A.transpose(Arow); - settings_.log.printf("After A transpose\n");
1214-1225: Mutex scope could be narrowed.The mutex lock spans
add_cutsand the subsequent status check. Ifadd_cutsis a lengthy operation, this could create contention withset_new_solution. Consider whether narrower locking is possible, or document why the broad scope is necessary.cpp/src/dual_simplex/cuts.hpp (3)
33-35: Use logging infrastructure instead of printf.The
printfcall bypasses the solver's logging infrastructure. Usesettings.log.printfor similar for consistency with the rest of the codebase.However, since
minimum_violationdoesn't take settings as a parameter, consider either:
- Adding a settings parameter, or
- Making this a debug-only diagnostic (gated by preprocessor)
if (Cx[k] <= cut_rhs[k]) { - printf("C*x <= d for cut %d. C*x %e rhs %e\n", k, Cx[k], cut_rhs[k]); +#ifdef DEBUG_CUTS + fprintf(stderr, "C*x <= d for cut %d. C*x %e rhs %e\n", k, Cx[k], cut_rhs[k]); +#endif }
126-138: Constructor performs computation (b_solve).The constructor calls
basis_update.b_solve(lp.rhs, b_bar_)which performs a potentially expensive basis solve. Consider whether this should be a separate initialization step for better control over when the computation occurs, or document that the constructor has side effects.
219-220: Add namespace closing comment.For consistency with other headers in the codebase, add a closing comment for the namespace.
-} +} // namespace cuopt::linear_programming::dual_simplexcpp/src/dual_simplex/cuts.cpp (7)
15-35: Consider returning an error code fromadd_cut.The function currently performs an early return at Line 28 when encountering invalid variables, but doesn't signal this failure to the caller. Consider returning an error code (e.g.,
i_tstatus) so callers can detect and handle validation failures.
85-150: Consider making cut selection parameters configurable.Lines 88-89 and 111-113 use hardcoded values for weights (
weight_distance,weight_orthogonality), thresholds (max_cuts=2000,min_orthogonality=0.5,min_cut_distance=1e-4). These are typically problem-dependent parameters that benefit from tuning. Consider exposing them through the settings structure.
304-365: Refactor duplicate cut violation checking logic.Lines 314-326 and 344-356 contain nearly identical code for checking cut violation and computing cut distance. This duplication can be extracted into a helper function to improve maintainability.
🔎 Example refactoring:
// Add a helper function template <typename i_t, typename f_t> bool check_cut_violation(const sparse_vector_t<i_t, f_t>& cut, f_t cut_rhs, const std::vector<f_t>& xstar, const simplex_solver_settings_t<i_t, f_t>& settings, i_t cut_id, f_t& distance) { f_t dot = 0.0; f_t cut_norm = 0.0; for (i_t k = 0; k < cut.i.size(); k++) { const i_t jj = cut.i[k]; const f_t aj = cut.x[k]; dot += aj * xstar[jj]; cut_norm += aj * aj; } if (dot >= cut_rhs) { settings.log.printf("Cut %d is not violated. Skipping\n", cut_id); return false; } distance = (cut_rhs - dot) / std::sqrt(cut_norm); return true; }Then replace both code blocks with calls to this helper.
Based on coding guidelines: Refactor code duplication in solver components (3+ occurrences) into shared utilities.
499-509: Consider making shallow cut tolerances configurable.Lines 499 and 506 use hardcoded tolerances (
shallow_tol = 1e-2,0.01,0.99) for filtering shallow or near-integer cuts. These thresholds are problem-dependent and could benefit from being exposed through the settings structure.
807-856: Verify algorithmic complexity of C_B construction.Lines 807-856 construct the
C_Bmatrix by iterating through all cuts and checking basic variable membership. The nested loops (cuts × cut_coefficients × basic_lookup) have O(p × nnz × 1) complexity wherepis the number of cuts. For large-scale problems with thousands of cuts, consider whether thein_basislookup (O(1) via hash map vs O(n) via vector) is optimal. The current implementation uses a vector which is acceptable, but document this for future optimization if needed.Based on coding guidelines: Assess algorithmic complexity for large-scale problems.
894-1012: Consider the performance impact of full basis refactorization.Line 1010 performs a full
basis_update.refactor_basis()after removing cuts, which is computationally expensive. For frequent cut removal operations in branch-and-bound, consider whether incremental basis updates would be more efficient. However, full refactorization may be necessary for numerical stability after structural changes.Based on coding guidelines: Assess algorithmic complexity for large-scale problems; operational concerns such as observability and performance monitoring.
912-918: Consider making cut removal tolerances configurable.Lines 912 and 918 use hardcoded tolerances (
dual_tol = 1e-10,slack_tol = 1e-3) for identifying removable cuts. These thresholds are problem-dependent and affect the aggressiveness of cut removal. Consider exposing them through the settings structure.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cpp/src/dual_simplex/CMakeLists.txt(2 hunks)cpp/src/dual_simplex/branch_and_bound.cpp(16 hunks)cpp/src/dual_simplex/branch_and_bound.hpp(1 hunks)cpp/src/dual_simplex/cuts.cpp(1 hunks)cpp/src/dual_simplex/cuts.hpp(1 hunks)cpp/src/dual_simplex/solve.cpp(1 hunks)cpp/src/dual_simplex/sparse_matrix.cpp(2 hunks)cpp/src/mip/diversity/diversity_manager.cu(1 hunks)cpp/src/mip/diversity/lns/rins.cu(1 hunks)cpp/src/mip/solver.cu(4 hunks)cpp/tests/dual_simplex/unit_tests/solve.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- cpp/tests/dual_simplex/unit_tests/solve.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/src/dual_simplex/sparse_matrix.cpp
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{cu,cuh,cpp,hpp,h}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.{cu,cuh,cpp,hpp,h}: Track GPU device memory allocations and deallocations to prevent memory leaks; ensure cudaMalloc/cudaFree balance and cleanup of streams/events
Validate algorithm correctness in optimization logic: simplex pivots, branch-and-bound decisions, routing heuristics, and constraint/objective handling must produce correct results
Check numerical stability: prevent overflow/underflow, precision loss, division by zero/near-zero, and use epsilon comparisons for floating-point equality checks
Validate correct initialization of variable bounds, constraint coefficients, and algorithm state before solving; ensure reset when transitioning between algorithm phases (presolve, simplex, diving, crossover)
Ensure variables and constraints are accessed from the correct problem context (original vs presolve vs folded vs postsolve); verify index mapping consistency across problem transformations
For concurrent CUDA operations (barriers, async operations), explicitly create and manage dedicated streams instead of reusing the default stream; document stream lifecycle
Eliminate unnecessary host-device synchronization (cudaDeviceSynchronize) in hot paths that blocks GPU pipeline; use streams and events for async execution
Assess algorithmic complexity for large-scale problems (millions of variables/constraints); ensure O(n log n) or better complexity, not O(n²) or worse
Verify correct problem size checks before expensive GPU/CPU operations; prevent resource exhaustion on oversized problems
Identify assertions with overly strict numerical tolerances that fail on legitimate degenerate/edge cases (near-zero pivots, singular matrices, empty problems)
Ensure race conditions are absent in multi-GPU code and multi-threaded server implementations; verify proper synchronization of shared state
Refactor code duplication in solver components (3+ occurrences) into shared utilities; for GPU kernels, use templated device functions to avoid duplication
Check that hard-coded GPU de...
Files:
cpp/src/dual_simplex/branch_and_bound.hppcpp/src/mip/diversity/diversity_manager.cucpp/src/mip/solver.cucpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/mip/diversity/lns/rins.cucpp/src/dual_simplex/cuts.hppcpp/src/dual_simplex/cuts.cpp
**/*.{h,hpp,py}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
Verify C API does not break ABI stability (no struct layout changes, field reordering); maintain backward compatibility in Python and server APIs with deprecation warnings
Files:
cpp/src/dual_simplex/branch_and_bound.hppcpp/src/dual_simplex/cuts.hpp
**/*.{cpp,hpp,h}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.{cpp,hpp,h}: Check for unclosed file handles when reading MPS/QPS problem files; ensure RAII patterns or proper cleanup in exception paths
Validate input sanitization to prevent buffer overflows and resource exhaustion attacks; avoid unsafe deserialization of problem files
Prevent thread-unsafe use of global and static variables; use proper mutex/synchronization in server code accessing shared solver state
Files:
cpp/src/dual_simplex/branch_and_bound.hppcpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/cuts.hppcpp/src/dual_simplex/cuts.cpp
**/*.{cu,cpp,hpp,h}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
Avoid inappropriate use of exceptions in performance-critical GPU operation paths; prefer error codes or CUDA error checking for latency-sensitive code
Files:
cpp/src/dual_simplex/branch_and_bound.hppcpp/src/mip/diversity/diversity_manager.cucpp/src/mip/solver.cucpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/mip/diversity/lns/rins.cucpp/src/dual_simplex/cuts.hppcpp/src/dual_simplex/cuts.cpp
**/*.{cu,cuh}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.{cu,cuh}: Every CUDA kernel launch and memory operation must have error checking with CUDA_CHECK or equivalent verification
Avoid reinventing functionality already available in Thrust, CCCL, or RMM libraries; prefer standard library utilities over custom implementations
Files:
cpp/src/mip/diversity/diversity_manager.cucpp/src/mip/solver.cucpp/src/mip/diversity/lns/rins.cu
**/*.cu
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.cu: Verify race conditions and correctness of GPU kernel shared memory, atomics, and warp-level operations
Detect inefficient GPU kernel launches with low occupancy or poor memory access patterns; optimize for coalesced memory access and minimize warp divergence in hot paths
Files:
cpp/src/mip/diversity/diversity_manager.cucpp/src/mip/solver.cucpp/src/mip/diversity/lns/rins.cu
🧠 Learnings (23)
📓 Common learnings
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Reduce tight coupling between solver components (presolve, simplex, basis, barrier); increase modularity and reusability of optimization algorithms
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cpp,hpp,h} : Prevent thread-unsafe use of global and static variables; use proper mutex/synchronization in server code accessing shared solver state
Applied to files:
cpp/src/dual_simplex/branch_and_bound.hpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Validate algorithm correctness in optimization logic: simplex pivots, branch-and-bound decisions, routing heuristics, and constraint/objective handling must produce correct results
Applied to files:
cpp/src/dual_simplex/branch_and_bound.hppcpp/src/mip/solver.cucpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/CMakeLists.txtcpp/src/dual_simplex/branch_and_bound.cppcpp/src/mip/diversity/lns/rins.cucpp/src/dual_simplex/cuts.hppcpp/src/dual_simplex/cuts.cpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Validate correct initialization of variable bounds, constraint coefficients, and algorithm state before solving; ensure reset when transitioning between algorithm phases (presolve, simplex, diving, crossover)
Applied to files:
cpp/src/dual_simplex/branch_and_bound.hppcpp/src/mip/diversity/diversity_manager.cucpp/src/mip/solver.cucpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/CMakeLists.txtcpp/src/dual_simplex/branch_and_bound.cppcpp/src/mip/diversity/lns/rins.cucpp/src/dual_simplex/cuts.hppcpp/src/dual_simplex/cuts.cpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*test*.{cpp,cu,py} : Add tests for algorithm phase transitions: verify correct initialization of bounds and state when transitioning from presolve to simplex to diving to crossover
Applied to files:
cpp/src/dual_simplex/branch_and_bound.hppcpp/src/mip/solver.cucpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/CMakeLists.txtcpp/src/dual_simplex/branch_and_bound.cppcpp/src/mip/diversity/lns/rins.cucpp/src/dual_simplex/cuts.hppcpp/src/dual_simplex/cuts.cpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Refactor code duplication in solver components (3+ occurrences) into shared utilities; for GPU kernels, use templated device functions to avoid duplication
Applied to files:
cpp/src/mip/diversity/diversity_manager.cucpp/src/mip/solver.cucpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/CMakeLists.txtcpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/cuts.hppcpp/src/dual_simplex/cuts.cpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Ensure variables and constraints are accessed from the correct problem context (original vs presolve vs folded vs postsolve); verify index mapping consistency across problem transformations
Applied to files:
cpp/src/mip/solver.cucpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/CMakeLists.txtcpp/src/dual_simplex/branch_and_bound.cppcpp/src/mip/diversity/lns/rins.cu
📚 Learning: 2025-12-04T20:09:09.264Z
Learnt from: chris-maes
Repo: NVIDIA/cuopt PR: 602
File: cpp/src/linear_programming/solve.cu:732-742
Timestamp: 2025-12-04T20:09:09.264Z
Learning: In cpp/src/linear_programming/solve.cu, the barrier solver does not currently return INFEASIBLE or UNBOUNDED status. It only returns OPTIMAL, TIME_LIMIT, NUMERICAL_ISSUES, or CONCURRENT_LIMIT.
Applied to files:
cpp/src/mip/solver.cucpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/mip/diversity/lns/rins.cucpp/src/dual_simplex/cuts.hpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Reduce tight coupling between solver components (presolve, simplex, basis, barrier); increase modularity and reusability of optimization algorithms
Applied to files:
cpp/src/mip/solver.cucpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/cuts.hppcpp/src/dual_simplex/cuts.cpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Assess algorithmic complexity for large-scale problems (millions of variables/constraints); ensure O(n log n) or better complexity, not O(n²) or worse
Applied to files:
cpp/src/mip/solver.cucpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Check that hard-coded GPU device IDs and resource limits are made configurable; abstract multi-backend support for different CUDA versions
Applied to files:
cpp/src/mip/solver.cu
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Verify correct problem size checks before expensive GPU/CPU operations; prevent resource exhaustion on oversized problems
Applied to files:
cpp/src/mip/solver.cucpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Identify assertions with overly strict numerical tolerances that fail on legitimate degenerate/edge cases (near-zero pivots, singular matrices, empty problems)
Applied to files:
cpp/src/mip/solver.cucpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cpp,hpp,h} : Avoid inappropriate use of exceptions in performance-critical GPU operation paths; prefer error codes or CUDA error checking for latency-sensitive code
Applied to files:
cpp/src/mip/solver.cucpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-10-22T14:25:22.899Z
Learnt from: aliceb-nv
Repo: NVIDIA/cuopt PR: 527
File: cpp/src/mip/diversity/lns/rins.cu:167-175
Timestamp: 2025-10-22T14:25:22.899Z
Learning: In MIP (Mixed Integer Programming) problems in the cuOPT codebase, `n_integer_vars == 0` is impossible by definition—MIP problems must have at least one integer variable. If there are no integer variables, it would be a pure Linear Programming (LP) problem, not a MIP problem.
Applied to files:
cpp/src/mip/solver.cucpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/cuts.hpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*test*.{cpp,cu,py} : Test with free variables, singleton problems, and extreme problem dimensions near resource limits to validate edge case handling
Applied to files:
cpp/src/mip/solver.cucpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/CMakeLists.txtcpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-12-04T04:11:12.640Z
Learnt from: chris-maes
Repo: NVIDIA/cuopt PR: 500
File: cpp/src/dual_simplex/scaling.cpp:68-76
Timestamp: 2025-12-04T04:11:12.640Z
Learning: In the cuOPT dual simplex solver, CSR/CSC matrices (including the quadratic objective matrix Q) are required to have valid dimensions and indices by construction. Runtime bounds checking in performance-critical paths like matrix scaling is avoided to prevent slowdowns. Validation is performed via debug-only check_matrix() calls wrapped in #ifdef CHECK_MATRIX.
Applied to files:
cpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/cuts.cpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*test*.{cpp,cu,py} : Write tests validating numerical correctness of optimization results (not just 'runs without error'); test degenerate cases (infeasible, unbounded, empty, singleton problems)
Applied to files:
cpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-12-03T23:29:26.391Z
Learnt from: chris-maes
Repo: NVIDIA/cuopt PR: 500
File: cpp/src/dual_simplex/sparse_matrix.cpp:519-524
Timestamp: 2025-12-03T23:29:26.391Z
Learning: In cpp/src/dual_simplex/sparse_matrix.cpp, the check_matrix() function is debug/diagnostic code (wrapped in #ifdef CHECK_MATRIX) that intentionally prints errors without necessarily returning early. The return codes from this debug code are not actively checked; the purpose is to print all validation errors in one pass for better diagnostics.
Applied to files:
cpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*test*.{cpp,cu,py} : Add tests for problem transformations: verify correctness of original→transformed→postsolve mappings and index consistency across problem representations
Applied to files:
cpp/src/dual_simplex/CMakeLists.txt
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh} : Avoid reinventing functionality already available in Thrust, CCCL, or RMM libraries; prefer standard library utilities over custom implementations
Applied to files:
cpp/src/dual_simplex/CMakeLists.txt
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cpp,hpp,h} : Check for unclosed file handles when reading MPS/QPS problem files; ensure RAII patterns or proper cleanup in exception paths
Applied to files:
cpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Check numerical stability: prevent overflow/underflow, precision loss, division by zero/near-zero, and use epsilon comparisons for floating-point equality checks
Applied to files:
cpp/src/dual_simplex/branch_and_bound.cpp
🧬 Code graph analysis (3)
cpp/src/dual_simplex/branch_and_bound.cpp (3)
cpp/src/dual_simplex/solve.hpp (4)
solve_linear_program_with_advanced_basis(53-62)compute_user_objective(37-37)compute_user_objective(40-40)compute_objective(34-34)cpp/src/dual_simplex/presolve.hpp (2)
uncrush_primal_solution(160-163)bound_strengthening(185-190)cpp/src/dual_simplex/cuts.hpp (2)
minimum_violation(22-39)minimum_violation(22-24)
cpp/src/dual_simplex/cuts.hpp (2)
cpp/src/dual_simplex/sparse_matrix.hpp (2)
C(140-140)Arow(51-52)cpp/src/dual_simplex/sparse_matrix.cpp (1)
matrix_vector_multiply(957-962)
cpp/src/dual_simplex/cuts.cpp (3)
cpp/src/dual_simplex/cuts.hpp (6)
n(57-57)cut_storage_(68-68)row(71-71)row(72-72)i(73-73)a(176-182)cpp/src/dual_simplex/sparse_matrix.cpp (10)
dot(846-855)dot(846-846)sparse_dot(858-880)sparse_dot(858-861)sparse_dot(883-907)sparse_dot(883-886)sparse_dot(952-955)add(708-740)add(708-712)add(946-950)cpp/src/dual_simplex/sparse_matrix.hpp (1)
Arow(51-52)
🔇 Additional comments (15)
cpp/src/dual_simplex/CMakeLists.txt (2)
13-13: LGTM: New source file for cut generation.The addition of
cuts.cppto the build is appropriate for the new cutting plane support.
37-37: Debug compile flag enabled - verify this is intentional for release.The
-g1compile option is now active for all dual simplex sources. While useful during development, this adds debug symbols to the binary which may not be desired in production builds. Consider gating this behind a CMake option or removing before merging if not needed.cpp/src/dual_simplex/branch_and_bound.hpp (1)
152-154: LGTM: Mutex for thread-safe LP modifications.The new
mutex_original_lp_member appropriately protects the original LP data structure during concurrent cut-generation operations in the branch-and-bound workflow.cpp/src/dual_simplex/solve.cpp (1)
11-11: LGTM: Include for basis solve infrastructure.The new include supports the advanced basis-based solving workflow used by the cut-generation integration.
cpp/src/mip/solver.cu (3)
171-171: LGTM: Propagating new cut passes setting.Correctly propagates the
max_cut_passesconfiguration from solver settings to branch-and-bound settings.
229-231: Commented-out status retrieval.The immediate branch-and-bound status retrieval is commented out. The status is still retrieved later at line 236 after primal heuristics complete, so this appears intentional. Verify this is the desired behavior for the concurrent execution model.
130-130: Debug code: LP-concurrent path disabled.The
0 &&prefix disables the LP fallback path when the problem is reduced to pure LP.- if (0 && context.problem_ptr->n_integer_vars == 0) { + if (context.problem_ptr->n_integer_vars == 0) {⛔ Skipped due to learnings
Learnt from: aliceb-nv Repo: NVIDIA/cuopt PR: 527 File: cpp/src/mip/diversity/lns/rins.cu:167-175 Timestamp: 2025-10-22T14:25:22.899Z Learning: In MIP (Mixed Integer Programming) problems in the cuOPT codebase, `n_integer_vars == 0` is impossible by definition—MIP problems must have at least one integer variable. If there are no integer variables, it would be a pure Linear Programming (LP) problem, not a MIP problem.Learnt from: CR Repo: NVIDIA/cuopt PR: 0 File: .github/.coderabbit_review_guide.md:0-0 Timestamp: 2025-11-25T10:20:49.822Z Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Ensure variables and constraints are accessed from the correct problem context (original vs presolve vs folded vs postsolve); verify index mapping consistency across problem transformationsLearnt from: CR Repo: NVIDIA/cuopt PR: 0 File: .github/.coderabbit_review_guide.md:0-0 Timestamp: 2025-11-25T10:20:49.822Z Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Validate algorithm correctness in optimization logic: simplex pivots, branch-and-bound decisions, routing heuristics, and constraint/objective handling must produce correct resultsLearnt from: CR Repo: NVIDIA/cuopt PR: 0 File: .github/.coderabbit_review_guide.md:0-0 Timestamp: 2025-11-25T10:20:49.822Z Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Validate correct initialization of variable bounds, constraint coefficients, and algorithm state before solving; ensure reset when transitioning between algorithm phases (presolve, simplex, diving, crossover)cpp/src/dual_simplex/branch_and_bound.cpp (5)
9-10: LGTM: New includes for cut generation infrastructure.The includes for
basis_solves.hppandcuts.hppare required for the new cutting plane workflow.
260-269: LGTM: Thread-safe access to original LP.Proper mutex locking around
original_lp_access when processing external solutions. The lock scope is appropriately narrow.
1080-1088: LGTM: Root LP solve with advanced basis.The switch to
solve_linear_program_with_advanced_basisenables basis tracking required for cut generation. The additional parameters (basis_update,basic_list,nonbasic_list) are correctly initialized.
1148-1149: Cut pool initialized with fixed column count.The
cut_poolis initialized withoriginal_lp_.num_colsat this point. After cuts are added,original_lp_.num_colsincreases (slack variables are added). Verify that the cut pool'soriginal_vars_remains correct throughout the cut generation loop, or if it needs to track the original column count separately.
678-678: LGTM: Branch call updated with integer infeasibility count.The branch call now correctly passes
leaf_num_fractionalfor tracking integer infeasibility at each node.cpp/src/dual_simplex/cuts.hpp (3)
44-52: Verify lifetime ofsettings_reference.The
cut_pool_tstoressettings_as a const reference. Ensure thesimplex_solver_settings_tobject outlives thecut_pool_tinstance to avoid dangling reference issues.
41-88: LGTM: Cut pool class design.The
cut_pool_tclass provides a clean interface for managing cuts with scoring, aging, and selection. The separation of cut storage, scoring metrics, and selection logic is well-structured.
194-218: LGTM: add_cuts and remove_cuts function signatures.The function signatures appropriately take all necessary state for modifying the LP with cuts and updating the basis, solution vectors, and variable statuses.
| if (add_cuts_status != 0) { | ||
| settings_.log.printf("Failed to add cuts\n"); | ||
| exit(1); | ||
| } |
There was a problem hiding this comment.
Replace exit(1) with error return.
Library code should not terminate the process. Return an appropriate error status instead.
if (add_cuts_status != 0) {
settings_.log.printf("Failed to add cuts\n");
- exit(1);
+ return mip_status_t::NUMERICAL;
}🤖 Prompt for AI Agents
In cpp/src/dual_simplex/branch_and_bound.cpp around lines 1226 to 1229, the code
currently calls exit(1) when add_cuts_status != 0; change this to return an
appropriate error status instead of terminating the process. Modify the
surrounding function to propagate the failure: map add_cuts_status to the
function's error return value (e.g., return add_cuts_status or a defined error
code), ensure any necessary cleanup is done before returning, and update
callers/signature if needed so library code reports the error up the call stack
rather than calling exit.
| template <typename i_t, typename f_t> | ||
| f_t cut_pool_t<i_t, f_t>::cut_distance(i_t row, const std::vector<f_t>& x, f_t& cut_violation, f_t &cut_norm) | ||
| { | ||
| const i_t row_start = cut_storage_.row_start[row]; | ||
| const i_t row_end = cut_storage_.row_start[row + 1]; | ||
| f_t cut_x = 0.0; | ||
| f_t dot = 0.0; | ||
| for (i_t p = row_start; p < row_end; p++) { | ||
| const i_t j = cut_storage_.j[p]; | ||
| const f_t cut_coeff = cut_storage_.x[p]; | ||
| cut_x += cut_coeff * x[j]; | ||
| dot += cut_coeff * cut_coeff; | ||
| } | ||
| cut_violation = rhs_storage_[row] - cut_x; | ||
| cut_norm = std::sqrt(dot); | ||
| const f_t distance = cut_violation / cut_norm; | ||
| return distance; | ||
| } |
There was a problem hiding this comment.
Division by zero risk when computing cut distance.
Line 53 divides cut_violation by cut_norm without checking if cut_norm is zero or near-zero. If all cut coefficients are zero (or extremely small), this will cause division by zero or numerical instability. Add an epsilon check before division.
🔎 Apply this diff to add a safety check:
cut_violation = rhs_storage_[row] - cut_x;
cut_norm = std::sqrt(dot);
+ if (cut_norm < 1e-12) {
+ return 0.0; // or handle degenerate cut appropriately
+ }
const f_t distance = cut_violation / cut_norm;
return distance;Based on coding guidelines: Check numerical stability to prevent division by zero/near-zero.
🤖 Prompt for AI Agents
In cpp/src/dual_simplex/cuts.cpp around lines 38 to 55, the code divides
cut_violation by cut_norm without guarding against cut_norm being zero or nearly
zero; add a small epsilon (e.g. std::numeric_limits<f_t>::epsilon() * max(1,
std::abs(rhs_storage_[row]), std::abs(cut_x)) or a fixed tiny constant) and
check if cut_norm < eps before the division, and if so set cut_norm = eps (or
handle by setting distance to a safe sentinel like 0 or
std::numeric_limits<f_t>::max()); ensure the check uses the templated f_t type
and keep semantics consistent (assign cut_norm the safe value and compute
distance = cut_violation / cut_norm).
| template <typename i_t, typename f_t> | ||
| f_t cut_pool_t<i_t, f_t>::cut_orthogonality(i_t i, i_t j) | ||
| { | ||
| const i_t i_start = cut_storage_.row_start[i]; | ||
| const i_t i_end = cut_storage_.row_start[i + 1]; | ||
| const i_t i_nz = i_end - i_start; | ||
| const i_t j_start = cut_storage_.row_start[j]; | ||
| const i_t j_end = cut_storage_.row_start[j + 1]; | ||
| const i_t j_nz = j_end - j_start; | ||
|
|
||
| f_t dot = sparse_dot(cut_storage_.j.data() + i_start, cut_storage_.x.data() + i_start, i_nz, | ||
| cut_storage_.j.data() + j_start, cut_storage_.x.data() + j_start, j_nz); | ||
|
|
||
| f_t norm_i = cut_norms_[i]; | ||
| f_t norm_j = cut_norms_[j]; | ||
| return 1.0 - std::abs(dot) / (norm_i * norm_j); | ||
| } |
There was a problem hiding this comment.
Division by zero risk in orthogonality calculation.
Line 82 divides by (norm_i * norm_j) without checking if either norm is zero or near-zero. Add an epsilon check to prevent division by zero for degenerate cuts.
🔎 Apply this diff to add a safety check:
f_t norm_i = cut_norms_[i];
f_t norm_j = cut_norms_[j];
+ const f_t denom = norm_i * norm_j;
+ if (denom < 1e-12) {
+ return 1.0; // treat degenerate cuts as orthogonal
+ }
- return 1.0 - std::abs(dot) / (norm_i * norm_j);
+ return 1.0 - std::abs(dot) / denom;Based on coding guidelines: Check numerical stability to prevent division by zero/near-zero.
| if (lp.lower[j] < 0) { | ||
| settings_.log.printf("Variable %d has negative lower bound %e\n", j, lp.lower[j]); | ||
| exit(1); | ||
| } |
There was a problem hiding this comment.
Replace exit(1) with proper error handling.
Lines 543-546 call exit(1) when encountering a negative lower bound, which terminates the entire process. This is not appropriate for a library function in a production code path. Consider returning an error code or throwing an exception instead, allowing the caller to handle the error gracefully.
🔎 Apply this diff to use error code instead:
+ // Return error code via function signature change:
+ // i_t initialize(const lp_problem_t<i_t, f_t>& lp, const std::vector<f_t>& xstar)
+
for (i_t j = 0; j < lp.num_cols; j++) {
if (lp.lower[j] < 0) {
settings_.log.printf("Variable %d has negative lower bound %e\n", j, lp.lower[j]);
- exit(1);
+ return -1; // Return error code
}Based on coding guidelines: Verify error propagation from internal functions to user-facing APIs; avoid inappropriate use of exceptions in performance-critical paths, but exit() is even worse.
Committable suggestion skipped: line range outside the PR's diff.
| std::vector<f_t> b_bar_; | ||
| std::vector<f_t> nonbasic_mark_; | ||
| std::vector<f_t> x_workspace_; | ||
| std::vector<i_t> x_mark_; |
There was a problem hiding this comment.
Type mismatch: nonbasic_mark_ should be std::vector<i_t>.
nonbasic_mark_ is declared as std::vector<f_t> (floating-point) but is used as a marker array with values 0 or 1. It should be std::vector<i_t> for consistency with x_mark_.
- std::vector<f_t> nonbasic_mark_;
+ std::vector<i_t> nonbasic_mark_;📝 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.
| std::vector<f_t> b_bar_; | |
| std::vector<f_t> nonbasic_mark_; | |
| std::vector<f_t> x_workspace_; | |
| std::vector<i_t> x_mark_; | |
| std::vector<f_t> b_bar_; | |
| std::vector<i_t> nonbasic_mark_; | |
| std::vector<f_t> x_workspace_; | |
| std::vector<i_t> x_mark_; |
🤖 Prompt for AI Agents
In cpp/src/dual_simplex/cuts.hpp around lines 154 to 157, nonbasic_mark_ is
declared as std::vector<f_t> but is used as an integer marker (0/1); change its
type to std::vector<i_t> to match x_mark_ and ensure consistent integer
semantics; update any initialization or uses that assume floating-point (if any)
to work with i_t and rebuild to confirm no further type errors.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
cpp/src/dual_simplex/cuts.cpp (4)
34-51: Division by zero risk when computing cut distance.Line 49 divides
cut_violationbycut_normwithout checking ifcut_normis zero or near-zero. If all cut coefficients are zero (or extremely small), this will cause division by zero or numerical instability.Based on coding guidelines: Check numerical stability to prevent division by zero/near-zero.
🔎 Apply this diff to add a safety check:
cut_violation = rhs_storage_[row] - cut_x; cut_norm = std::sqrt(dot); + if (cut_norm < std::numeric_limits<f_t>::epsilon() * std::max(f_t(1.0), std::abs(cut_violation))) { + cut_violation = 0.0; + cut_norm = 1.0; + } const f_t distance = cut_violation / cut_norm; return distance;
63-79: Division by zero risk in orthogonality calculation.Line 78 divides by
(norm_i * norm_j)without checking if either norm is zero or near-zero.Based on coding guidelines: Check numerical stability to prevent division by zero/near-zero.
🔎 Apply this diff to add a safety check:
f_t norm_i = cut_norms_[i]; f_t norm_j = cut_norms_[j]; + const f_t denom = norm_i * norm_j; + const f_t eps = std::numeric_limits<f_t>::epsilon(); + if (denom < eps) { + return 1.0; // treat degenerate cuts as orthogonal + } - return 1.0 - std::abs(dot) / (norm_i * norm_j); + return 1.0 - std::abs(dot) / denom;
205-266: Fix control flow logic for invalid MIR cuts.Lines 247-263: When
mir_status != 0(failure), execution falls through to lines 262-263 and adds an invalid cut to the pool. The function should skip adding the cut whenmir_status != 0.Based on coding guidelines: Validate algorithm correctness in optimization logic.
🔎 Apply this diff to fix the control flow:
i_t mir_status = mir.generate_cut(inequality, inequality_rhs, lp.upper, lp.lower, var_types, cut, cut_rhs); if (mir_status == 0) { f_t dot = 0.0; f_t cut_norm = 0.0; for (i_t k = 0; k < cut.i.size(); k++) { const i_t jj = cut.i[k]; const f_t aj = cut.x[k]; dot += aj * xstar[jj]; cut_norm += aj * aj; } if (dot >= cut_rhs) { continue; } + settings.log.printf("Adding MIR cut %d\n", i); + cut_pool_.add_cut(cut, cut_rhs); } - - settings.log.printf("Adding MIR cut %d\n", i); - cut_pool_.add_cut(cut, cut_rhs); }
543-548: Replaceexit(1)with proper error handling.Lines 545-547 call
exit(1)when encountering a negative lower bound, which terminates the entire process. This is not appropriate for library code in a production path.Based on coding guidelines: Verify error propagation from internal functions to user-facing APIs.
🔎 Recommended approach:
Change the function signature to return an error code:
i_t initialize(const lp_problem_t<i_t, f_t>& lp, const std::vector<i_t>& new_slacks, const std::vector<f_t>& xstar)Then replace the exit call:
for (i_t j = 0; j < lp.num_cols; j++) { if (lp.lower[j] < 0) { settings_.log.printf("Variable %d has negative lower bound %e\n", j, lp.lower[j]); - exit(1); + return -1; // Return error code }Update callers to check the return value and handle errors appropriately.
cpp/src/dual_simplex/branch_and_bound.cpp (3)
1230-1233: Replaceexit(1)with error return.Line 1232 calls
exit(1)whenadd_cuts_status != 0. Library code should not terminate the process.Based on coding guidelines: Verify error propagation from internal functions to user-facing APIs.
🔎 Apply this diff:
if (add_cuts_status != 0) { settings_.log.printf("Failed to add cuts\n"); - exit(1); + status_ = mip_exploration_status_t::NUMERICAL; + return set_final_solution(solution, root_objective_); }
1282-1285: Replaceexit(1)with error return.Lines 1283-1284 call
exit(1)when cut solve returns non-optimal status. This should be handled gracefully.Based on coding guidelines: Verify error propagation from internal functions to user-facing APIs.
🔎 Apply this diff:
if (cut_status != dual::status_t::OPTIMAL) { settings_.log.printf("Cut status %d\n", cut_status); - exit(1); + status_ = mip_exploration_status_t::NUMERICAL; + return set_final_solution(solution, root_objective_); }
1154-1324: Cutting plane loop lacks convergence detection and memory bounds enforcement.The loop (lines 1154-1325) runs
for (i_t cut_pass = 0; cut_pass < settings_.max_cut_passes; cut_pass++)with three problems:
No early termination on stagnation: The loop always runs
max_cut_passes(default 10) iterations even if cuts stop improving the objective or no improving cuts are generated. Standard cutting plane algorithms should exit early when no new cuts improve the bound or when cut violations fall below a threshold.Unbounded cut pool memory:
cut_poolaccumulates cuts across iterations with no enforcement of memory bounds. Whileage_cuts()anddrop_cuts()methods exist incut_pool_t, they are never called during the loop. Cut pools can grow without bound, risking exhaustion of memory on large problems with many variables/constraints.Hard exit on LP failures: Lines 1230-1232 and 1249-1250 call
exit(1)on LP infeasibility or cut-addition failures. This bypasses proper error propagation to user-facing APIs. Instead, log the failure and gracefully return a status code indicating the issue.Add convergence detection (e.g., check objective improvement between iterations or cut violation magnitude), implement cut pool size limits with periodic pruning, and replace
exit(1)calls with proper error returns.cpp/src/dual_simplex/cuts.hpp (1)
156-160: Type mismatch:nonbasic_mark_should bestd::vector<i_t>.Line 158:
nonbasic_mark_is declared asstd::vector<f_t>but is used as a marker array with values 0 or 1 (see line 139 in the constructor). It should bestd::vector<i_t>for consistency withx_mark_.🔎 Apply this diff:
- std::vector<f_t> nonbasic_mark_; + std::vector<i_t> nonbasic_mark_;
🧹 Nitpick comments (3)
cpp/src/dual_simplex/cuts.cpp (1)
314-322: Add safety check for near-zero cut norm.Line 321 divides by
std::sqrt(cut_norm)without checking ifcut_normis near-zero. While cuts should normally have non-zero coefficients, defensive programming suggests adding a check.Based on coding guidelines: Check numerical stability to prevent division by zero/near-zero.
🔎 Suggested enhancement:
f_t dot = cut_A.dot(xstar); f_t cut_norm = cut_A.norm2_squared(); + if (cut_norm < std::numeric_limits<f_t>::epsilon()) { + settings.log.printf("Cut %d has near-zero norm. Skipping\n", i); + continue; + } if (dot >= cut_A_rhs) { settings.log.printf("Cut %d is not violated. Skipping\n", i); continue; } cut_A_distance = (cut_A_rhs - dot) / std::sqrt(cut_norm);Apply similar check at line 340-346 for cut_B.
cpp/src/dual_simplex/sparse_matrix.cpp (1)
360-401: LGTM - append_rows implementation with minor suggestion.The
append_rowsimplementation correctly validates dimensions, resizes storage, and appends row data. The implementation is clean and handles the CSR structure properly.Optional enhancement for better diagnostics:
Consider returning the validation error earlier with more context:
if (C.n > n) { - printf("append_rows error: C.n %d n %d\n", C.n, n); + printf("append_rows error: source matrix has %d columns but target has only %d columns\n", C.n, n); return -1; }cpp/src/dual_simplex/cuts.hpp (1)
21-39: Consider adding epsilon tolerance to minimum_violation check.Line 33 checks
Cx[k] <= cut_rhs[k]with exact comparison. For numerical robustness, consider using an epsilon tolerance since floating-point arithmetic may have small rounding errors.Based on coding guidelines: Use epsilon comparisons for floating-point equality checks.
Suggested enhancement:
+ const f_t eps = std::numeric_limits<f_t>::epsilon() * 10; for (i_t k = 0; k < Cx.size(); k++) { - if (Cx[k] <= cut_rhs[k]) { + if (Cx[k] <= cut_rhs[k] + eps) { printf("C*x <= d for cut %d. C*x %e rhs %e\n", k, Cx[k], cut_rhs[k]); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cpp/src/dual_simplex/branch_and_bound.cpp(16 hunks)cpp/src/dual_simplex/cuts.cpp(1 hunks)cpp/src/dual_simplex/cuts.hpp(1 hunks)cpp/src/dual_simplex/sparse_matrix.cpp(3 hunks)cpp/src/dual_simplex/sparse_vector.cpp(2 hunks)cpp/src/dual_simplex/sparse_vector.hpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cu,cuh,cpp,hpp,h}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.{cu,cuh,cpp,hpp,h}: Track GPU device memory allocations and deallocations to prevent memory leaks; ensure cudaMalloc/cudaFree balance and cleanup of streams/events
Validate algorithm correctness in optimization logic: simplex pivots, branch-and-bound decisions, routing heuristics, and constraint/objective handling must produce correct results
Check numerical stability: prevent overflow/underflow, precision loss, division by zero/near-zero, and use epsilon comparisons for floating-point equality checks
Validate correct initialization of variable bounds, constraint coefficients, and algorithm state before solving; ensure reset when transitioning between algorithm phases (presolve, simplex, diving, crossover)
Ensure variables and constraints are accessed from the correct problem context (original vs presolve vs folded vs postsolve); verify index mapping consistency across problem transformations
For concurrent CUDA operations (barriers, async operations), explicitly create and manage dedicated streams instead of reusing the default stream; document stream lifecycle
Eliminate unnecessary host-device synchronization (cudaDeviceSynchronize) in hot paths that blocks GPU pipeline; use streams and events for async execution
Assess algorithmic complexity for large-scale problems (millions of variables/constraints); ensure O(n log n) or better complexity, not O(n²) or worse
Verify correct problem size checks before expensive GPU/CPU operations; prevent resource exhaustion on oversized problems
Identify assertions with overly strict numerical tolerances that fail on legitimate degenerate/edge cases (near-zero pivots, singular matrices, empty problems)
Ensure race conditions are absent in multi-GPU code and multi-threaded server implementations; verify proper synchronization of shared state
Refactor code duplication in solver components (3+ occurrences) into shared utilities; for GPU kernels, use templated device functions to avoid duplication
Check that hard-coded GPU de...
Files:
cpp/src/dual_simplex/sparse_vector.cppcpp/src/dual_simplex/cuts.cppcpp/src/dual_simplex/sparse_matrix.cppcpp/src/dual_simplex/sparse_vector.hppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/cuts.hpp
**/*.{cpp,hpp,h}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.{cpp,hpp,h}: Check for unclosed file handles when reading MPS/QPS problem files; ensure RAII patterns or proper cleanup in exception paths
Validate input sanitization to prevent buffer overflows and resource exhaustion attacks; avoid unsafe deserialization of problem files
Prevent thread-unsafe use of global and static variables; use proper mutex/synchronization in server code accessing shared solver state
Files:
cpp/src/dual_simplex/sparse_vector.cppcpp/src/dual_simplex/cuts.cppcpp/src/dual_simplex/sparse_matrix.cppcpp/src/dual_simplex/sparse_vector.hppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/cuts.hpp
**/*.{cu,cpp,hpp,h}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
Avoid inappropriate use of exceptions in performance-critical GPU operation paths; prefer error codes or CUDA error checking for latency-sensitive code
Files:
cpp/src/dual_simplex/sparse_vector.cppcpp/src/dual_simplex/cuts.cppcpp/src/dual_simplex/sparse_matrix.cppcpp/src/dual_simplex/sparse_vector.hppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/cuts.hpp
**/*.{h,hpp,py}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
Verify C API does not break ABI stability (no struct layout changes, field reordering); maintain backward compatibility in Python and server APIs with deprecation warnings
Files:
cpp/src/dual_simplex/sparse_vector.hppcpp/src/dual_simplex/cuts.hpp
🧠 Learnings (19)
📓 Common learnings
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Reduce tight coupling between solver components (presolve, simplex, basis, barrier); increase modularity and reusability of optimization algorithms
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*test*.{cpp,cu,py} : Add tests for algorithm phase transitions: verify correct initialization of bounds and state when transitioning from presolve to simplex to diving to crossover
📚 Learning: 2025-12-04T04:11:12.640Z
Learnt from: chris-maes
Repo: NVIDIA/cuopt PR: 500
File: cpp/src/dual_simplex/scaling.cpp:68-76
Timestamp: 2025-12-04T04:11:12.640Z
Learning: In the cuOPT dual simplex solver, CSR/CSC matrices (including the quadratic objective matrix Q) are required to have valid dimensions and indices by construction. Runtime bounds checking in performance-critical paths like matrix scaling is avoided to prevent slowdowns. Validation is performed via debug-only check_matrix() calls wrapped in #ifdef CHECK_MATRIX.
Applied to files:
cpp/src/dual_simplex/sparse_vector.cppcpp/src/dual_simplex/cuts.cppcpp/src/dual_simplex/sparse_matrix.cppcpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Validate algorithm correctness in optimization logic: simplex pivots, branch-and-bound decisions, routing heuristics, and constraint/objective handling must produce correct results
Applied to files:
cpp/src/dual_simplex/cuts.cppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/cuts.hpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*test*.{cpp,cu,py} : Add tests for algorithm phase transitions: verify correct initialization of bounds and state when transitioning from presolve to simplex to diving to crossover
Applied to files:
cpp/src/dual_simplex/cuts.cppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/cuts.hpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Validate correct initialization of variable bounds, constraint coefficients, and algorithm state before solving; ensure reset when transitioning between algorithm phases (presolve, simplex, diving, crossover)
Applied to files:
cpp/src/dual_simplex/cuts.cppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/cuts.hpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Refactor code duplication in solver components (3+ occurrences) into shared utilities; for GPU kernels, use templated device functions to avoid duplication
Applied to files:
cpp/src/dual_simplex/cuts.cppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/cuts.hpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Check numerical stability: prevent overflow/underflow, precision loss, division by zero/near-zero, and use epsilon comparisons for floating-point equality checks
Applied to files:
cpp/src/dual_simplex/cuts.cppcpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Identify assertions with overly strict numerical tolerances that fail on legitimate degenerate/edge cases (near-zero pivots, singular matrices, empty problems)
Applied to files:
cpp/src/dual_simplex/cuts.cppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/cuts.hpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Verify correct problem size checks before expensive GPU/CPU operations; prevent resource exhaustion on oversized problems
Applied to files:
cpp/src/dual_simplex/cuts.cppcpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Ensure variables and constraints are accessed from the correct problem context (original vs presolve vs folded vs postsolve); verify index mapping consistency across problem transformations
Applied to files:
cpp/src/dual_simplex/cuts.cppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/cuts.hpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cpp,hpp,h} : Check for unclosed file handles when reading MPS/QPS problem files; ensure RAII patterns or proper cleanup in exception paths
Applied to files:
cpp/src/dual_simplex/cuts.cppcpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*test*.{cpp,cu,py} : Test with free variables, singleton problems, and extreme problem dimensions near resource limits to validate edge case handling
Applied to files:
cpp/src/dual_simplex/cuts.cppcpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-12-04T20:09:09.264Z
Learnt from: chris-maes
Repo: NVIDIA/cuopt PR: 602
File: cpp/src/linear_programming/solve.cu:732-742
Timestamp: 2025-12-04T20:09:09.264Z
Learning: In cpp/src/linear_programming/solve.cu, the barrier solver does not currently return INFEASIBLE or UNBOUNDED status. It only returns OPTIMAL, TIME_LIMIT, NUMERICAL_ISSUES, or CONCURRENT_LIMIT.
Applied to files:
cpp/src/dual_simplex/cuts.cppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/cuts.hpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*test*.{cpp,cu,py} : Write tests validating numerical correctness of optimization results (not just 'runs without error'); test degenerate cases (infeasible, unbounded, empty, singleton problems)
Applied to files:
cpp/src/dual_simplex/cuts.cppcpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-10-22T14:25:22.899Z
Learnt from: aliceb-nv
Repo: NVIDIA/cuopt PR: 527
File: cpp/src/mip/diversity/lns/rins.cu:167-175
Timestamp: 2025-10-22T14:25:22.899Z
Learning: In MIP (Mixed Integer Programming) problems in the cuOPT codebase, `n_integer_vars == 0` is impossible by definition—MIP problems must have at least one integer variable. If there are no integer variables, it would be a pure Linear Programming (LP) problem, not a MIP problem.
Applied to files:
cpp/src/dual_simplex/cuts.cppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/cuts.hpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Reduce tight coupling between solver components (presolve, simplex, basis, barrier); increase modularity and reusability of optimization algorithms
Applied to files:
cpp/src/dual_simplex/cuts.cppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/cuts.hpp
📚 Learning: 2025-12-03T23:29:26.391Z
Learnt from: chris-maes
Repo: NVIDIA/cuopt PR: 500
File: cpp/src/dual_simplex/sparse_matrix.cpp:519-524
Timestamp: 2025-12-03T23:29:26.391Z
Learning: In cpp/src/dual_simplex/sparse_matrix.cpp, the check_matrix() function is debug/diagnostic code (wrapped in #ifdef CHECK_MATRIX) that intentionally prints errors without necessarily returning early. The return codes from this debug code are not actively checked; the purpose is to print all validation errors in one pass for better diagnostics.
Applied to files:
cpp/src/dual_simplex/sparse_matrix.cppcpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Assess algorithmic complexity for large-scale problems (millions of variables/constraints); ensure O(n log n) or better complexity, not O(n²) or worse
Applied to files:
cpp/src/dual_simplex/branch_and_bound.cpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cpp,hpp,h} : Avoid inappropriate use of exceptions in performance-critical GPU operation paths; prefer error codes or CUDA error checking for latency-sensitive code
Applied to files:
cpp/src/dual_simplex/branch_and_bound.cpp
🧬 Code graph analysis (5)
cpp/src/dual_simplex/sparse_vector.cpp (4)
cpp/src/dual_simplex/sparse_vector.hpp (6)
sparse_vector_t(21-21)sparse_vector_t(23-23)sparse_vector_t(25-25)sparse_vector_t(27-27)sparse_vector_t(29-29)x(44-44)cpp/src/dual_simplex/sparse_matrix.cpp (2)
dot(851-860)dot(851-851)cpp/src/dual_simplex/vector_math.hpp (1)
dot(42-42)cpp/src/dual_simplex/vector_math.cpp (3)
dot(48-57)dot(48-48)dot(181-181)
cpp/src/dual_simplex/cuts.cpp (2)
cpp/src/dual_simplex/cuts.hpp (13)
cut(57-57)cut_storage_(68-68)row(71-71)row(72-72)i(73-73)x_relax(59-59)a(181-187)lp(96-104)lp(107-115)lp(117-122)lp(144-154)lp(177-179)lp(189-192)cpp/src/dual_simplex/sparse_vector.hpp (3)
p(40-40)p(42-42)x(44-44)
cpp/src/dual_simplex/sparse_matrix.cpp (1)
cpp/src/dual_simplex/sparse_matrix.hpp (5)
C(140-140)new_nz(48-48)nz(78-78)c(143-143)j(63-63)
cpp/src/dual_simplex/sparse_vector.hpp (1)
cpp/src/dual_simplex/sparse_matrix.hpp (3)
row(87-87)x(70-70)x(74-74)
cpp/src/dual_simplex/branch_and_bound.cpp (4)
cpp/src/dual_simplex/solve.hpp (4)
solve_linear_program_with_advanced_basis(53-62)compute_user_objective(37-37)compute_user_objective(40-40)compute_objective(34-34)cpp/src/dual_simplex/presolve.hpp (2)
uncrush_primal_solution(160-163)bound_strengthening(185-190)cpp/src/dual_simplex/cuts.hpp (3)
minimum_violation(22-39)minimum_violation(22-24)add_cuts(207-217)cpp/src/dual_simplex/phase2.hpp (1)
dual_phase2_with_advanced_basis(61-73)
🔇 Additional comments (9)
cpp/src/dual_simplex/sparse_vector.hpp (2)
28-29: LGTM - CSR constructor declaration.The new constructor from CSR matrix row complements the existing CSC-based constructor and aligns with the expanded CSR support in the codebase.
43-44: LGTM - dot product method declaration.The dot product method declaration is clean and follows the existing API patterns in the class.
cpp/src/dual_simplex/sparse_vector.cpp (2)
31-44: LGTM - CSR constructor implementation.The CSR row constructor implementation correctly mirrors the CSC column constructor pattern. It properly initializes the sparse vector dimension, reserves storage, and populates from the CSR row data.
Based on learnings: Runtime bounds checking is avoided in performance-critical paths; validation is performed via debug-only checks.
124-133: LGTM - dot product implementation.The dot product implementation is straightforward and efficient, accumulating the product of sparse coefficients with the corresponding dense vector elements.
cpp/src/dual_simplex/sparse_matrix.cpp (3)
403-427: LGTM - append_row implementation.The
append_rowimplementation correctly handles appending a single sparse vector as a CSR row, properly updating dimensions and storage.
570-573: Good addition - CSC column pointer bounds check.The added bounds check for
col_startsize prevents potential out-of-bounds access during matrix validation. This is a good defensive programming practice.
632-632: Good addition - CSR column index bounds check.The added bounds check for column indices ensures CSR matrix integrity during validation.
Based on learnings: check_matrix() is debug/diagnostic code that prints all validation errors for better diagnostics.
cpp/src/dual_simplex/branch_and_bound.cpp (2)
9-10: LGTM - New includes for cut framework.The new includes for
basis_solves.hppandcuts.hppare necessary for the extended cutting plane functionality.
1080-1088: LGTM - Enhanced root LP solve with advanced basis.The change from
solve_linear_program_advancedtosolve_linear_program_with_advanced_basiswith explicit basis tracking (basis_update,basic_list,nonbasic_list) enables the warm-start dual simplex flow described in the PR objectives.
| if (!feasible) { | ||
| settings_.log.printf("Bound strengthening failed\n"); | ||
| exit(1); | ||
| } |
There was a problem hiding this comment.
Replace exit(1) with error return.
Lines 1249-1250 call exit(1) when bound strengthening fails. This should propagate an error instead.
Based on coding guidelines: Verify error propagation from internal functions to user-facing APIs.
🔎 Apply this diff:
if (!feasible) {
settings_.log.printf("Bound strengthening failed\n");
- exit(1);
+ status_ = mip_exploration_status_t::NUMERICAL;
+ return set_final_solution(solution, root_objective_);
}📝 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 (!feasible) { | |
| settings_.log.printf("Bound strengthening failed\n"); | |
| exit(1); | |
| } | |
| if (!feasible) { | |
| settings_.log.printf("Bound strengthening failed\n"); | |
| status_ = mip_exploration_status_t::NUMERICAL; | |
| return set_final_solution(solution, root_objective_); | |
| } |
🤖 Prompt for AI Agents
In cpp/src/dual_simplex/branch_and_bound.cpp around lines 1248-1251, the code
calls exit(1) on a bound-strengthening failure which must be replaced with error
propagation; remove exit(1), keep the settings_.log.printf line, and return an
error indicator consistent with the function's signature (e.g., return false for
a bool function or return a non-ok Status/errno value); update all callers to
check the returned error and propagate it upward (return early on error) so the
failure reaches the user-facing API instead of terminating the process.
| i_t append_status = new_A_row.append_rows(cuts); | ||
| if (append_status != 0) { | ||
| settings.log.printf("append_rows error: %d\n", append_status); | ||
| exit(1); | ||
| } |
There was a problem hiding this comment.
Unconditional exit(1) on append error.
Lines 850-853 call exit(1) when append_rows fails. This is not wrapped in a debug macro and will terminate the process in production builds.
Based on coding guidelines: Verify error propagation from internal functions to user-facing APIs.
🔎 Apply this diff to propagate the error:
i_t append_status = new_A_row.append_rows(cuts);
if (append_status != 0) {
settings.log.printf("append_rows error: %d\n", append_status);
- exit(1);
+ return -1;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cpp/src/dual_simplex/cuts.cpp around lines 850 to 854, the code
unconditionally calls exit(1) when new_A_row.append_rows(cuts) returns a
non-zero status; instead propagate the error to the caller. Replace the exit(1)
with logic that returns or forwards append_status (or sets an appropriate error
code/flag and jumps to the existing cleanup/return path) so the caller can
handle the failure; do not terminate the process here and ensure any allocated
resources are cleaned up before returning the error.
|
🔔 Hi @anandhkb @chris-maes, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
2 participants
This PR allows you to add constraints in the form C*x <= d to an LP that has already been solved to optimality (and has basis information).
The constraints must be violated at the current solution x^star. That is, C*x^star > d.
The constraint matrix, rhs, basis, and basis update, are all updated to include the additional constraints.
Dual simplex is started in phase 2 from a dual feasible solution.
This is a prerequisite for adding cuts into MIP.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.