Fix bug where batch PDLP for strong branching was running on problem without cuts#951
Conversation
Kh4ster
added
bug
📝 WalkthroughWalkthroughReworks strong_branching and simplex_problem_to_mps_data_model signatures to use 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)
📝 Coding Plan
Comment Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
![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
|
/ok to test 3aa1ea5 |
|
/ok to test 4d51222 |
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/pseudo_costs.cpp (1)
383-423:⚠️ Potential issue | 🟠 MajorHandle infeasible Batch PDLP branches as
+inf, notNaN.The simplex fallback in this file treats an infeasible child as
+infand clamps the delta withstd::max(obj - root_obj, 0.0). This path only acceptsOptimal, so an infeasible down/up branch gets recorded asNaNand then ignored byupdate_pseudo_costs_from_strong_branching(). That drops exactly the strongest branching signal and can skew variable selection after cuts are added. Please map the PDLP infeasible termination to+infhere and keep the non-negative delta clamp in the Batch PDLP path as well.I can help sketch a small
termination_status -> strong_branch_deltahelper once you confirm which PDLP infeasible enum(s) Batch PDLP returns here.As per coding guidelines, "Validate algorithm correctness in optimization logic: simplex pivots, branch-and-bound decisions, routing heuristics, and constraint/objective handling must produce correct results".
🤖 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 383 - 423, The batch PDLP branch results currently map non-Optimal termination to NaN (via obj_down/obj_up), which causes strong branching deltas to be ignored; change the logic in the loop that computes obj_down/obj_up (the uses of solutions.get_termination_status(...) and solutions.get_dual_objective_value(...)) so that infeasible PDLP termination(s) returned for a child are mapped to +infinity instead of NaN, then compute the stored deltas pc.strong_branch_down[k] and pc.strong_branch_up[k] as non-negative clamped values (e.g. max(obj - root_obj, 0.0)) like the simplex fallback does; ensure you handle the two indices k and k + fractional.size() consistently and keep update_pseudo_costs_from_strong_branching()’s expectations intact.
🧹 Nitpick comments (1)
cpp/src/branch_and_bound/pseudo_costs.cpp (1)
243-277: Keep the reduced-space vectors aligned with the same slack mask.
A_no_slacks.remove_columns(cols_to_remove)supports arbitrary slack positions, but the objective, bounds, andoriginal_root_soln_xare still taken as the firstnentries and then indexed with original column ids. That only stays correct if everynew_slacksentry is a suffix column. Please either compact those vectors withcols_to_removetoo, or assert that invariant here before using prefix slices.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".
Also applies to: 364-367
🤖 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 - 277, The reduced-space data (original_root_soln_x, objective/bounds used for mps_model via mps_model.set_objective_coefficients and set_variable_lower_bounds/set_variable_upper_bounds) is being taken as the first n entries but A_no_slacks.remove_columns(cols_to_remove) can remove arbitrary columns (new_slacks), so either compact lp.objective, lp.lower, lp.upper and root_soln into new vectors using the same cols_to_remove mask before slicing (and fill original_root_soln_x from that compacted root vector), or add a clear assertion that all indices in new_slacks form a suffix so the current prefix-slice is valid; update the code paths around original_root_soln_x assignment, A_no_slacks.remove_columns(cols_to_remove), and the calls to mps_model.set_* to use the compacted vectors or fail fast if the suffix invariant is not met.
🤖 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/pseudo_costs.cpp`:
- Around line 383-423: The batch PDLP branch results currently map non-Optimal
termination to NaN (via obj_down/obj_up), which causes strong branching deltas
to be ignored; change the logic in the loop that computes obj_down/obj_up (the
uses of solutions.get_termination_status(...) and
solutions.get_dual_objective_value(...)) so that infeasible PDLP termination(s)
returned for a child are mapped to +infinity instead of NaN, then compute the
stored deltas pc.strong_branch_down[k] and pc.strong_branch_up[k] as
non-negative clamped values (e.g. max(obj - root_obj, 0.0)) like the simplex
fallback does; ensure you handle the two indices k and k + fractional.size()
consistently and keep update_pseudo_costs_from_strong_branching()’s expectations
intact.
---
Nitpick comments:
In `@cpp/src/branch_and_bound/pseudo_costs.cpp`:
- Around line 243-277: The reduced-space data (original_root_soln_x,
objective/bounds used for mps_model via mps_model.set_objective_coefficients and
set_variable_lower_bounds/set_variable_upper_bounds) is being taken as the first
n entries but A_no_slacks.remove_columns(cols_to_remove) can remove arbitrary
columns (new_slacks), so either compact lp.objective, lp.lower, lp.upper and
root_soln into new vectors using the same cols_to_remove mask before slicing
(and fill original_root_soln_x from that compacted root vector), or add a clear
assertion that all indices in new_slacks form a suffix so the current
prefix-slice is valid; update the code paths around original_root_soln_x
assignment, A_no_slacks.remove_columns(cols_to_remove), and the calls to
mps_model.set_* to use the compacted vectors or fail fast if the suffix
invariant is not met.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 622d9ba7-fa15-49ed-a2e5-aa51e05b3221
📒 Files selected for processing (1)
cpp/src/branch_and_bound/pseudo_costs.cpp
3 participants
Thanks to Nicolas for discovering the issue.
We now translate the problem with cuts directly to a problem batch PDLP can solve.