Crossover dualize fix #504
Conversation
chris-maes
requested review from
Kh4ster and
hlinsen
and removed request for
a team
chris-maes
added
bug
|
Tip For best results, initiate chat on the files or code changes. @rgsl888prabhu, hello! How can I help you with this PR? I can:
Please let me know what you'd like me to do! |
Review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes optimize presolve by eliminating an unnecessary CSR row matrix conversion, enhance the dual simplex solver with dynamic slack variable augmentation and primal residual logging during crossover, and standardize thrust-based initialization patterns across MIP components. Changes
Sequence DiagramsequenceDiagram
participant Solver as Dual Simplex Solver
participant PrimalLP as Primal LP
participant Slack as Slack Augmentation
participant Crossover as Crossover Phase
Solver->>PrimalLP: Check inequality constraints (less_rows > 0)
alt Inequalities exist
Solver->>Slack: Augment problem with slack variables
Slack->>PrimalLP: Expand A, bounds, objective, solution vectors
Slack->>Slack: Initialize slack variables
Slack-->>Solver: Decrement less_rows to 0
Solver->>PrimalLP: Recompute primal residuals
Solver->>Solver: Log primal residuals (post-augmentation)
end
Solver->>Crossover: Enter crossover phase
Crossover->>Crossover: Log primal residuals (before artificial vars)
Crossover->>PrimalLP: Append artificial variables
Crossover->>Crossover: Log primal residuals (after artificial vars)
Crossover-->>Solver: Crossover complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The change set exhibits mixed complexity: four files contain straightforward thrust include additions and standard initialization patterns, while 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cpp/src/mip/problem/presolve_data.cuh (1)
63-66: Consider using thrust::fill instead of thrust::uninitialized_fill.After
resize()on line 62, the memory is already initialized byrmm::device_uvector. Thethrust::uninitialized_fillfunction is designed for uninitialized memory (placement-new style construction), whilethrust::fillis more appropriate for already-initialized memory.For POD types like
f_t(float/double), this likely won't cause runtime issues, but usingthrust::fillwould be more semantically correct.- thrust::uninitialized_fill(handle_ptr->get_thrust_policy(), - fixed_var_assignment.begin(), - fixed_var_assignment.end(), - 0.); + thrust::fill(handle_ptr->get_thrust_policy(), + fixed_var_assignment.begin(), + fixed_var_assignment.end(), + 0.);cpp/src/dual_simplex/solve.cpp (2)
401-441: Slack augmentation is sound; use numeric_limits and tighten invariantsThe construction/population of slacks (s = b − Ax, bounds [0, +∞), z_s = −y) looks correct and fixes the residual issue. Two nits:
- Replace INFINITY with std::numeric_limits<f_t>::infinity() to avoid header/macro pitfalls and be type‑correct for f_t.
- Optional: assert invariants around col_start monotonicity after the loop.
Apply includes outside this hunk:
@@ #include <queue> #include <string> +#include <limits>And within this block:
- problem.upper[j] = INFINITY; + problem.upper[j] = std::numeric_limits<f_t>::infinity();Optionally, right after setting problem.A.col_start[num_cols]:
+ for (i_t c = 0; c < num_cols; ++c) { assert(problem.A.col_start[c] <= problem.A.col_start[c+1]); }
484-489: Helpful pre-augmentation residual log; minor printf nit nearbyThe added pre-artificial residual log is useful. Minor: the nearby “Adding %ld artificial variables” (Line 497) should prefer %zu for size_t to be portable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cpp/src/dual_simplex/presolve.cpp(0 hunks)cpp/src/dual_simplex/solve.cpp(3 hunks)cpp/src/mip/diversity/weights.cuh(1 hunks)cpp/src/mip/problem/presolve_data.cuh(1 hunks)cpp/src/mip/relaxed_lp/lp_state.cuh(1 hunks)
💤 Files with no reviewable changes (1)
- cpp/src/dual_simplex/presolve.cpp
🔇 Additional comments (6)
cpp/src/mip/relaxed_lp/lp_state.cuh (1)
20-20: LGTM! Missing include added.The include for
thrust/fill.his necessary sincethrust::fillis used in the constructor (lines 35-38). This ensures the code compiles correctly.cpp/src/mip/problem/presolve_data.cuh (1)
23-23: Include added for thrust::uninitialized_fill usage.The include is necessary for the function used in
initialize_var_mapping.cpp/src/mip/diversity/weights.cuh (1)
20-20: LGTM! Proper thrust::fill usage for initialization.The include for
thrust/fill.his correctly added, and the usage ofthrust::fillto initializecstr_weightsto 1.0 is appropriate and follows best practices.Also applies to: 32-32
cpp/src/dual_simplex/solve.cpp (3)
395-401: Row-sense assumption check for slack additionYou treat every non-equality row as a “≤” and add +1 slack. If any rows are “≥”, their slack should enter with −1. Please confirm row senses here or gate by metadata; otherwise the constructed primal may be inconsistent.
443-446: Recomputing residual after slack augmentation: LGTMResidual on the augmented system is computed correctly as b − A x with the new slack columns.
536-540: Post-augmentation residual log: LGTMGood to log after appending artificials; this validates the augmentation didn’t worsen feasibility.
2 participants
When we solve the dual, instead of the primal problem, we weren't populating the primal slack variables. As a result crossover had a large initial primal residual and could fail.
This issue appeared when solving the LP relaxation of graph20-80-1rand.
This issue was not present if crossover was disabled.
This PR also removes an unnecessary transpose of the constraint matrix in
convert_less_than_to_equal.Summary by CodeRabbit
Improvements
Performance