Persistent basis_update_mpf_t in the Dual Simplex Phase 2 #383
Conversation
|
/ok to test a3e50f8 |
|
🔔 Hi @anandhkb, 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. |
|
This PR solves 89/92 instances from Below are the errors for each instance with both |
|
🔔 Hi @anandhkb, 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. |
…ing basis_update_mpf_t.
nguidotti
force-pushed
the
dual-simplex-update-basis
branch
from
7422762 to
181aef4
Compare
rgsl888prabhu
requested review from
KyleFromNVIDIA and
rgsl888prabhu
and removed request for
a team
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
/ok to test deffe68 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cpp/src/dual_simplex/basis_updates.cpp (1)
2111-2114: Preserve Q after refactorization
reset()callsclear(), which rewritescol_permutation_/inverse_col_permutation_to the identity. That destroys the Q permutation produced byfactorize_basis, so every subsequentb_*/u_*solve uses the wrong permuted LU and produces incorrect results. Please keep the existing permutations and only clear the incremental update structures. Suggested fix:reorder_basic_list(col_permutation_, basic_list); - reset(); + inverse_permutation(row_permutation_, inverse_row_permutation_); + inverse_permutation(col_permutation_, inverse_col_permutation_); + pivot_indices_.clear(); + pivot_indices_.reserve(L0_.m); + S_.col_start.assign(refactor_frequency_ + 1, 0); + S_.i.clear(); + S_.x.clear(); + S_.n = 0; + mu_values_.clear(); + num_updates_ = 0; + compute_transposes(); + reset_stats();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cpp/src/dual_simplex/basis_updates.cpp(2 hunks)cpp/src/dual_simplex/basis_updates.hpp(6 hunks)cpp/src/dual_simplex/phase2.cpp(5 hunks)cpp/src/dual_simplex/phase2.hpp(2 hunks)cpp/src/dual_simplex/solve.cpp(3 hunks)
🧰 Additional context used
🪛 Clang (14.0.6)
cpp/src/dual_simplex/basis_updates.cpp
[error] 18-18: 'dual_simplex/basis_solves.hpp' file not found
(clang-diagnostic-error)
cpp/src/dual_simplex/basis_updates.hpp
[error] 20-20: 'dual_simplex/initial_basis.hpp' file not found
(clang-diagnostic-error)
cpp/src/dual_simplex/phase2.hpp
[error] 20-20: 'dual_simplex/basis_updates.hpp' file not found
(clang-diagnostic-error)
cpp/src/dual_simplex/solve.cpp
Outdated
| junk.clear(); | ||
| dual_phase2(1, | ||
| 0, | ||
| start_time, | ||
| phase1_problem, | ||
| settings, | ||
| phase1_vstatus, | ||
| phase1_solution, | ||
| iter, | ||
| edge_norms); | ||
| initialize_basis_update = false; | ||
| dual_phase2_with_advanced_basis(1, | ||
| 0, | ||
| initialize_basis_update, | ||
| start_time, | ||
| phase1_problem, | ||
| settings, | ||
| phase1_vstatus, | ||
| ft, | ||
| basic_list, | ||
| nonbasic_list, | ||
| phase1_solution, | ||
| iter, | ||
| edge_norms); | ||
| vstatus = phase1_vstatus; | ||
| edge_norms.clear(); | ||
| status = dual_phase2(2, 0, start_time, lp, settings, vstatus, solution, iter, edge_norms); | ||
| status = dual_phase2_with_advanced_basis(2, | ||
| 0, | ||
| initialize_basis_update, | ||
| start_time, | ||
| lp, | ||
| settings, | ||
| vstatus, | ||
| ft, | ||
| basic_list, | ||
| nonbasic_list, | ||
| solution, | ||
| iter, | ||
| edge_norms); | ||
| } |
There was a problem hiding this comment.
Re-initialize basis before switching problems
When Phase 2 returns NUMERICAL, you set initialize_basis_update = false and immediately reuse ft for phase1_problem and then for the original lp. However, ft (and the associated basic_list/nonbasic_list) still contain the LU factorization of the previous matrix, so the very first solve after this point works with the wrong basis. Please rebuild the basis before each call on a different problem:
- initialize_basis_update = false;
- dual_phase2_with_advanced_basis(1,
- 0,
- initialize_basis_update,
+ initialize_basis_update = true;
+ dual_phase2_with_advanced_basis(1,
+ 0,
+ initialize_basis_update,
start_time,
phase1_problem,
settings,
phase1_vstatus,
ft,
basic_list,
nonbasic_list,
phase1_solution,
iter,
edge_norms);
vstatus = phase1_vstatus;
edge_norms.clear();
- status = dual_phase2_with_advanced_basis(2,
- 0,
- initialize_basis_update,
+ initialize_basis_update = true;
+ status = dual_phase2_with_advanced_basis(2,
+ 0,
+ initialize_basis_update,
start_time,
lp,
settings,
vstatus,
ft,
basic_list,
nonbasic_list,
solution,
iter,
edge_norms);Without this change we operate on stale LU factors, which corrupts the pivot operations and produces incorrect results.
📝 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.
| junk.clear(); | |
| dual_phase2(1, | |
| 0, | |
| start_time, | |
| phase1_problem, | |
| settings, | |
| phase1_vstatus, | |
| phase1_solution, | |
| iter, | |
| edge_norms); | |
| initialize_basis_update = false; | |
| dual_phase2_with_advanced_basis(1, | |
| 0, | |
| initialize_basis_update, | |
| start_time, | |
| phase1_problem, | |
| settings, | |
| phase1_vstatus, | |
| ft, | |
| basic_list, | |
| nonbasic_list, | |
| phase1_solution, | |
| iter, | |
| edge_norms); | |
| vstatus = phase1_vstatus; | |
| edge_norms.clear(); | |
| status = dual_phase2(2, 0, start_time, lp, settings, vstatus, solution, iter, edge_norms); | |
| status = dual_phase2_with_advanced_basis(2, | |
| 0, | |
| initialize_basis_update, | |
| start_time, | |
| lp, | |
| settings, | |
| vstatus, | |
| ft, | |
| basic_list, | |
| nonbasic_list, | |
| solution, | |
| iter, | |
| edge_norms); | |
| } | |
| junk.clear(); | |
| initialize_basis_update = true; | |
| dual_phase2_with_advanced_basis(1, | |
| 0, | |
| initialize_basis_update, | |
| start_time, | |
| phase1_problem, | |
| settings, | |
| phase1_vstatus, | |
| ft, | |
| basic_list, | |
| nonbasic_list, | |
| phase1_solution, | |
| iter, | |
| edge_norms); | |
| vstatus = phase1_vstatus; | |
| edge_norms.clear(); | |
| initialize_basis_update = true; | |
| status = dual_phase2_with_advanced_basis(2, | |
| 0, | |
| initialize_basis_update, | |
| start_time, | |
| lp, | |
| settings, | |
| vstatus, | |
| ft, | |
| basic_list, | |
| nonbasic_list, | |
| solution, | |
| iter, | |
| edge_norms); | |
| } |
🤖 Prompt for AI Agents
In cpp/src/dual_simplex/solve.cpp around lines 229 to 259, the code reuses ft
and the basic/nonbasic lists (LU factorization) across different problems;
before calling dual_phase2_with_advanced_basis on a different problem you must
rebuild the basis so the LU matches the new matrix. Fix this by reinitializing
the basis between problem switches: clear or reconstruct ft and the
basic_list/nonbasic_list (or call the existing basis-initialization helper) and
set initialize_basis_update appropriately so the next
dual_phase2_with_advanced_basis call computes a fresh LU for that problem rather
than using stale factors.
There was a problem hiding this comment.
The basis and ft is valid in this case. We can force a refactorization of the basis in the phase 1 problem if we run into numerical difficults. But it should not be required for the algorithm to work correctly.
There was a problem hiding this comment.
@chris-maes, thank you for the clarification! I understand now that the basis and ft remain valid when switching between the lp and phase1_problem in this scenario, and that reusing the factorization is intentional to avoid unnecessary recomputation. I appreciate you confirming that the algorithm's correctness is maintained and that refactorization can be forced if numerical issues arise. I'll dismiss this concern.
The member variable |
|
/ok to test f779cb7 |
|
/ok to test ad756d1 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/dual_simplex/basis_updates.hpp (1)
254-264: Guard stats updates against divide‑by‑zero.sum_* accumulators are updated using ratios like size/denominator in b_[transpose]_solve paths; when the input has 0 NZ, denominator can be 0 → NaN/inf. Add guards.
Apply this patch in cpp/src/dual_simplex/basis_updates.cpp:
@@ - sum_U_transpose_ += static_cast<f_t>(solution.i.size()) / input_size; + if (input_size > 0) { + sum_U_transpose_ += static_cast<f_t>(solution.i.size()) / input_size; + } @@ - sum_L_transpose_ += static_cast<f_t>(solution.i.size()) / rhs_size; + if (rhs_size > 0) { + sum_L_transpose_ += static_cast<f_t>(solution.i.size()) / rhs_size; + } @@ - sum_L_ += static_cast<f_t>(solution.i.size()) / input_size; + if (input_size > 0) { + sum_L_ += static_cast<f_t>(solution.i.size()) / input_size; + } @@ - sum_U_ += static_cast<f_t>(solution.i.size()) / rhs_size; + if (rhs_size > 0) { + sum_U_ += static_cast<f_t>(solution.i.size()) / rhs_size; + }
♻️ Duplicate comments (2)
cpp/src/dual_simplex/basis_updates.cpp (1)
2077-2092: Debug-check fixes look correct.Using A.m to size B and L0_ in the check blocks resolves the undefined identifiers noted earlier. LGTM.
cpp/src/dual_simplex/solve.cpp (1)
230-259: Reuse across Phase1/Phase2 acknowledged; no change requested.Reusing ft and lists with initialize_basis_update=false here is intentional per prior review discussion.
If helpful, add a brief comment in code noting that reusing the factorization across problems is by design and refactorization will be forced on numerical issues.
🧹 Nitpick comments (2)
cpp/src/dual_simplex/basis_updates.hpp (2)
281-287: Minor: consider recomputing inverse_row_permutation_ in reset().If row_permutation_ were externally modified before reset(), recomputing its inverse here would keep invariants local to the type.
i_t reset() { clear(); + inverse_permutation(row_permutation_, inverse_row_permutation_); compute_transposes(); reset_stats(); return 0; }
289-303: Resize should recompute transposes to keep object usable post‑call.Currently resize() doesn’t call compute_transposes(); subsequent solves relying on L0_transpose_/U0_transpose_ could misbehave if resize() is used stand‑alone. Call compute_transposes() or document that resize() must be followed by refactor_basis().
void resize(i_t n) { L0_.resize(n, n, 1); U0_.resize(n, n, 1); row_permutation_.resize(n); inverse_row_permutation_.resize(n); S_.resize(n, 0, 0); xi_workspace_.resize(2 * n, 0); x_workspace_.resize(n, 0.0); - U0_transpose_.resize(1, 1, 1); - L0_transpose_.resize(1, 1, 1); + U0_transpose_.resize(1, 1, 1); + L0_transpose_.resize(1, 1, 1); clear(); reset_stats(); + compute_transposes(); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cpp/src/dual_simplex/basis_updates.cpp(2 hunks)cpp/src/dual_simplex/basis_updates.hpp(6 hunks)cpp/src/dual_simplex/solve.cpp(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cpp/src/dual_simplex/solve.cpp (1)
cpp/src/dual_simplex/phase2.cpp (9)
settings(2129-2151)settings(2129-2129)basic_list(2186-2186)dual_phase2(2174-2204)dual_phase2(2174-2182)dual_phase2(2998-3007)dual_phase2_with_advanced_basis(2207-2994)dual_phase2_with_advanced_basis(2207-2219)dual_phase2_with_advanced_basis(3009-3022)
cpp/src/dual_simplex/basis_updates.cpp (2)
cpp/src/dual_simplex/basis_updates.hpp (3)
A(384-388)j(148-148)j(407-407)cpp/src/dual_simplex/basis_solves.cpp (12)
factorize_basis(167-594)factorize_basis(167-176)factorize_basis(822-831)basis_repair(597-648)basis_repair(597-603)basis_repair(833-839)form_b(651-680)form_b(651-653)form_b(841-843)reorder_basic_list(29-37)reorder_basic_list(29-29)reorder_basic_list(814-814)
🪛 Clang (14.0.6)
cpp/src/dual_simplex/basis_updates.hpp
[error] 20-20: 'dual_simplex/initial_basis.hpp' file not found
(clang-diagnostic-error)
cpp/src/dual_simplex/basis_updates.cpp
[error] 18-18: 'dual_simplex/basis_solves.hpp' file not found
(clang-diagnostic-error)
🔇 Additional comments (8)
cpp/src/dual_simplex/basis_updates.cpp (2)
2112-2116: Reset after reordering: safe for MPF, confirm intent.Calling reset() here clears update state and recomputes transposes without any Q state in MPF (no col_permutation_), so it won’t clobber permutations. Please confirm this path won’t be used with basis_update_t (FT), where reset() would reinit Q.
18-21: No action required—includes are properly configured.All four headers exist at
cpp/src/dual_simplex/and the CMakeLists.txt correctly configures"${CMAKE_CURRENT_SOURCE_DIR}/src"(resolving tocpp/src/) in the include path. The angle-bracket includes will resolve correctly. The static analysis warning is a false positive.Likely an incorrect or invalid review comment.
cpp/src/dual_simplex/solve.cpp (3)
119-135: Wrapper looks good and aligns with advanced basis flow.Constructing basis_update_mpf_t once and delegating to solve_linear_program_with_advanced_basis is clean.
137-148: New advanced-basis entrypoint is coherent.Signature and parameter threading match the rest of the solver stack.
211-224: Correct: initialize_basis_update=true on first advanced Phase 2.This ensures refactorization/init happens once before updates persist.
cpp/src/dual_simplex/basis_updates.hpp (3)
181-205: Constructor initializes MPF state appropriately.Reserving workspaces, wiring refactor_frequency_, and calling clear()+reset_stats() are appropriate.
383-389: API surface OK—ensure consistency with implementation.refactor_basis signature matches the implementation; confirm the header and cpp stay in lock‑step as this evolves.
20-22: No changes required. The include configuration is correct as-is.The headers
initial_basis.hppandsimplex_solver_settings.hppare internal to thedual_simplexmodule and do not need to be in the public include set. The build system correctly addscpp/srcas a PRIVATE include directory (cpp/CMakeLists.txt line 266), making<dual_simplex/...>angle-bracket includes work correctly for all internal consumers within that directory. All consumers ofbasis_updates.hppare located withincpp/src/dual_simplex/, confirming this is an internal module arrangement. Using angle-bracket includes for internal headers is standard practice.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cpp/src/dual_simplex/solve.hpp (1)
60-72: Consider expanding the API documentation.The function signature is well-designed and consistent with existing patterns. The persistent
basis_update_mpf_tparameter enables the branch-and-bound use case mentioned in the PR objectives.However, the comment could be more informative for API consumers. Consider documenting:
- When to use this function vs.
solve_linear_program_advanced- What "future use" means (e.g., passing basis state to child nodes in branch-and-bound)
- Whether
ft,basic_list, andnonbasic_listshould be empty on first call or can be pre-populatedExample expanded documentation:
-// Solve the LP using dual simplex and keep the `basis_update_mpf_t` -// for future use. +// Solve the LP using dual simplex and preserve the basis factorization state. +// This variant persists the basis_update_mpf_t (ft), basic/nonbasic lists, +// variable status, and edge norms for reuse in subsequent solves (e.g., +// passing basis state from parent to child in branch-and-bound). +// On the first call, ft can be default-constructed and lists can be empty.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cpp/src/dual_simplex/solve.hpp(2 hunks)
🧰 Additional context used
🪛 Clang (14.0.6)
cpp/src/dual_simplex/solve.hpp
[error] 20-20: 'dual_simplex/basis_updates.hpp' file not found
(clang-diagnostic-error)
🔇 Additional comments (1)
cpp/src/dual_simplex/solve.hpp (1)
20-20: LGTM!The new include is necessary to support the
basis_update_mpf_ttype in the new function signature below.Note: The static analysis error about the file not being found is a false positive—the tool is analyzing this header in isolation without the full project context.
|
/ok to test 8a06d3e |
|
/ok to test f3450c7 |
|
/ok to test 63969d2 |
|
/ok to test 0b578ad |
|
/merge |
3 participants
This PR extract the LU factorisation from the
dual_phase2to a separated method. It also added a new methods in the dual simplex to allow thebasis_update_mpf_tto persist between calls.This allows the branch-and-bound to pass the
basis_updatefrom parent to child in the future.Checklist
Summary by CodeRabbit
Bug Fixes
New Features