Fix bug where batch PDLP for strong branching was running on problem without cuts#951
Conversation
Kh4ster
added
bug
📝 WalkthroughWalkthroughThe pull request refactors the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/branch_and_bound/branch_and_bound.cpp (1)
2409-2420:⚠️ Potential issue | 🟠 MajorBatch PDLP still receives the cut-free root state.
remove_cuts(...)already ran on Lines 2339-2355, andfractionalis recomputed after that on Lines 2360-2361. So this call still feeds the strippedoriginal_lp_/root_relax_soln_/root_vstatus_state intostrong_branching, which means the newlp_problem_t-based path never sees the cut-strengthened root relaxation. If the goal of this PR is to score strong branches against the root LP with cuts, this needs to move beforeremove_cuts(...)or use a snapshot captured before that projection. As per coding guidelines, "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".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 2409 - 2420, strong_branching is being called with the cut-stripped state (original_lp_, root_relax_soln_, root_vstatus_, fractional) after remove_cuts(...) runs, so the branch scoring never sees the cut-strengthened root LP; move the strong_branching<i_t,f_t>(...) invocation to run before remove_cuts(...) or else capture a snapshot of the pre-remove_cuts state (including original_lp_, root_relax_soln_, root_vstatus_, and the recomputed fractional) and pass that snapshot into strong_branching so the function evaluates branches against the cut-strengthened root relaxation.
🧹 Nitpick comments (1)
cpp/src/branch_and_bound/pseudo_costs.cpp (1)
243-279: Project the non-slack vectors with the same mask as the matrix.
A_no_slacks.remove_columns(cols_to_remove)is order-agnostic, butoriginal_root_soln_x,objective,lower, andupperare still built by taking the firstnentries. That keeps this helper coupled to the current “slacks are suffix columns” layout. Rebuilding those arrays withcols_to_removewould make the translator self-contained and safer if column insertion order ever changes.♻️ Suggested direction
- int n = lp.num_cols - new_slacks.size(); - original_root_soln_x.resize(n); + int n = lp.num_cols - new_slacks.size(); + std::vector<f_t> objective_no_slacks; + std::vector<f_t> lower_no_slacks; + std::vector<f_t> upper_no_slacks; + objective_no_slacks.reserve(n); + lower_no_slacks.reserve(n); + upper_no_slacks.reserve(n); + original_root_soln_x.clear(); + original_root_soln_x.reserve(n); @@ - for (i_t j = 0; j < n; j++) { - original_root_soln_x[j] = root_soln[j]; - } + for (i_t j = 0; j < lp.num_cols; ++j) { + if (cols_to_remove[j]) { continue; } + objective_no_slacks.push_back(lp.objective[j]); + lower_no_slacks.push_back(lp.lower[j]); + upper_no_slacks.push_back(lp.upper[j]); + original_root_soln_x.push_back(root_soln[j]); + } @@ - mps_model.set_objective_coefficients(lp.objective.data(), n); + mps_model.set_objective_coefficients(objective_no_slacks.data(), n); @@ - mps_model.set_variable_lower_bounds(lp.lower.data(), n); - mps_model.set_variable_upper_bounds(lp.upper.data(), n); + mps_model.set_variable_lower_bounds(lower_no_slacks.data(), n); + mps_model.set_variable_upper_bounds(upper_no_slacks.data(), n);Based on learnings, "Reduce tight coupling between solver components (presolve, simplex, basis, barrier); increase modularity and reusability of optimization algorithms".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/pseudo_costs.cpp` around lines 243 - 279, The code currently trims columns from A via A_no_slacks.remove_columns(cols_to_remove) but still builds original_root_soln_x and passes lp.objective, lp.lower, lp.upper by taking the first n entries, assuming slacks were trailing; change this to project those vectors using the same cols_to_remove mask so their entries align with the column order of A_no_slacks: build a projected_original_root_soln_x by iterating over the full root_soln and copying entries where cols_to_remove[j]==0 (maintaining original column order), similarly construct projected_objective, projected_lower, and projected_upper from lp.objective, lp.lower, lp.upper using cols_to_remove, then pass those projected arrays to mps_model.set_objective_coefficients and set_variable_lower_bounds/set_variable_upper_bounds and use projected_original_root_soln_x instead of slicing the first n elements.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 2409-2420: strong_branching is being called with the cut-stripped
state (original_lp_, root_relax_soln_, root_vstatus_, fractional) after
remove_cuts(...) runs, so the branch scoring never sees the cut-strengthened
root LP; move the strong_branching<i_t,f_t>(...) invocation to run before
remove_cuts(...) or else capture a snapshot of the pre-remove_cuts state
(including original_lp_, root_relax_soln_, root_vstatus_, and the recomputed
fractional) and pass that snapshot into strong_branching so the function
evaluates branches against the cut-strengthened root relaxation.
---
Nitpick comments:
In `@cpp/src/branch_and_bound/pseudo_costs.cpp`:
- Around line 243-279: The code currently trims columns from A via
A_no_slacks.remove_columns(cols_to_remove) but still builds original_root_soln_x
and passes lp.objective, lp.lower, lp.upper by taking the first n entries,
assuming slacks were trailing; change this to project those vectors using the
same cols_to_remove mask so their entries align with the column order of
A_no_slacks: build a projected_original_root_soln_x by iterating over the full
root_soln and copying entries where cols_to_remove[j]==0 (maintaining original
column order), similarly construct projected_objective, projected_lower, and
projected_upper from lp.objective, lp.lower, lp.upper using cols_to_remove, then
pass those projected arrays to mps_model.set_objective_coefficients and
set_variable_lower_bounds/set_variable_upper_bounds and use
projected_original_root_soln_x instead of slicing the first n elements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1065a0a-3686-4088-bff9-6c0dcd8e7f4f
📒 Files selected for processing (3)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/pseudo_costs.cppcpp/src/branch_and_bound/pseudo_costs.hpp
3 participants
Thanks to Nicolas for discovering the issue.
We now translate the problem with cuts directly to a problem batch PDLP can solve.