Bug fix of dual init point computation in barrier#1577
Conversation
Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
yuwenchen95
added
non-breaking
yuwenchen95
changed the title
yuwenchen95
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughChangesThe barrier solver adds SOC-aware interior initialization and a Sturm/SeDuMi mu-based strategy. Diagonal preprocessing stores Barrier initialization
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 (2)
cpp/src/barrier/barrier.cu (1)
2187-2236: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSOC initial-point branches are unreachable for cone problems
data.has_cones()forcesinit_strategyto2, and that branch returns immediately, so the laterhas_cones()/has_socpath ininitial_point()never runs for SOCPs. That makes the new SOC interior-enforcement helper and the sparse-SOCaug_sizepath dead code here, and it also silently ignores any explicitbarrier_dual_initial_point = 0/1setting on cone problems.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/barrier/barrier.cu` around lines 2187 - 2236, Update initial_point’s init_strategy selection and early-return flow so cone problems do not unconditionally force strategy 2. Preserve explicit barrier_dual_initial_point values 0/1 for cone problems, and route those cases through the existing SOC initialization/interior-enforcement and sparse-SOC aug_size logic; retain the mu-based strategy 2 behavior when selected.Source: Path instructions
python/cuopt_server/cuopt_server/tests/test_lp.py (1)
163-172: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
dual_initial_point=2to the test matrix. The current@pytest.mark.parametrizelist covers-1,0, and1, but not the new2value, so the mu-based path still isn’t exercised.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cuopt_server/cuopt_server/tests/test_lp.py` around lines 163 - 172, Update the pytest parameterization for test_barrier_solver_options to include dual_initial_point=2 alongside the existing -1, 0, and 1 values, ensuring the mu-based path is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cpp/src/barrier/barrier.cu`:
- Around line 2187-2236: Update initial_point’s init_strategy selection and
early-return flow so cone problems do not unconditionally force strategy 2.
Preserve explicit barrier_dual_initial_point values 0/1 for cone problems, and
route those cases through the existing SOC initialization/interior-enforcement
and sparse-SOC aug_size logic; retain the mu-based strategy 2 behavior when
selected.
In `@python/cuopt_server/cuopt_server/tests/test_lp.py`:
- Around line 163-172: Update the pytest parameterization for
test_barrier_solver_options to include dual_initial_point=2 alongside the
existing -1, 0, and 1 values, ensuring the mu-based path is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7b1686c5-e726-4b2a-8619-e97d44323b58
📒 Files selected for processing (5)
cpp/src/barrier/barrier.cucpp/src/dual_simplex/simplex_solver_settings.hppcpp/src/math_optimization/solver_settings.cupython/cuopt_server/cuopt_server/tests/test_lp.pypython/cuopt_server/cuopt_server/utils/linear_programming/data_definition.py
Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
130b9a4 to
8e29f92
Compare
CI Test Summary8 failed · 23 passed · 0 skipped
|
| // Linear orthant | ||
| for (i_t j = 0; j < cs; ++j) { | ||
| // Full primal+dual point; no factorization/solve (main loop factorizes later). | ||
| if (init_strategy == 2) { |
There was a problem hiding this comment.
Use enums instead of constants for initial point strategy type.
There was a problem hiding this comment.
Do you think we'd better rename barrier_dual_initial_point to barrier_initial_point now?
|
@yuwenchen95 did you run QCQP benchmark? are there any regressions? |
Current change won't have any impact on the performance of QCQP since I only add the options but force the initialization of QCQP as before. |
Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
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/barrier/barrier.cu (1)
2478-2479: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExclude direct-free variables from the z interior shift.
nonnegative_zis all ones, so a direct-freez[j] == 0participates in the SOC helper’s minimum calculation and can shift every other linear reduced cost byepsilon_adjust. Resetting only the free entries afterward does not undo that shift.Proposed fix
- ensure_interior(data.z, nonnegative_z); + ensure_interior(data.z, nonnegative_variables);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/barrier/barrier.cu` around lines 2478 - 2479, Update the z interior-shift preparation around ensure_interior so direct-free z entries are excluded from the SOC helper’s minimum calculation before shifting. Use the existing direct-free indicator to mask or otherwise remove those entries from nonnegative_z, while preserving the current handling of nonnegative_variables and ensuring free z values remain unchanged without influencing other reduced costs.
🧹 Nitpick comments (2)
cpp/include/cuopt/mathematical_optimization/constants.h (1)
186-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpose the new strategy values in public documentation and bindings.
These public constants add an externally configurable strategy contract. Please verify that the barrier parameter documentation and supported bindings expose
-1,0,1, and2, including the new SeDuMi-Mu option.As per path instructions, public C++ API changes should be checked for corresponding docs/updates.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/include/cuopt/mathematical_optimization/constants.h` around lines 186 - 189, Update the public barrier parameter documentation and all supported language bindings to expose the constants from the barrier dual initial-point strategy contract: automatic (-1), Lustig-Marsten-Shanno (0), dual least squares (1), and SeDuMi-Mu (2). Use the existing public symbols and binding-generation patterns, ensuring the new SeDuMi-Mu option is included consistently.Source: Path instructions
cpp/src/barrier/barrier.cu (1)
2195-2241: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the new initial-point branches.
Please cover automatic SOCP selection, explicit
SedumiMu, SOC identity points, augmented padding, and direct-free linear variables.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/barrier/barrier.cu` around lines 2195 - 2241, Add regression tests covering automatic SOCP initial-point selection and explicit SedumiMu initialization. Verify SOC entries use the identity point (mu*sqrt(2) in the first coordinate and zero elsewhere), augmented padding is initialized correctly, and direct-free linear variables receive the expected z value while other linear variables remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cpp/src/barrier/barrier.cu`:
- Around line 2478-2479: Update the z interior-shift preparation around
ensure_interior so direct-free z entries are excluded from the SOC helper’s
minimum calculation before shifting. Use the existing direct-free indicator to
mask or otherwise remove those entries from nonnegative_z, while preserving the
current handling of nonnegative_variables and ensuring free z values remain
unchanged without influencing other reduced costs.
---
Nitpick comments:
In `@cpp/include/cuopt/mathematical_optimization/constants.h`:
- Around line 186-189: Update the public barrier parameter documentation and all
supported language bindings to expose the constants from the barrier dual
initial-point strategy contract: automatic (-1), Lustig-Marsten-Shanno (0), dual
least squares (1), and SeDuMi-Mu (2). Use the existing public symbols and
binding-generation patterns, ensuring the new SeDuMi-Mu option is included
consistently.
In `@cpp/src/barrier/barrier.cu`:
- Around line 2195-2241: Add regression tests covering automatic SOCP
initial-point selection and explicit SedumiMu initialization. Verify SOC entries
use the identity point (mu*sqrt(2) in the first coordinate and zero elsewhere),
augmented padding is initialized correctly, and direct-free linear variables
receive the expected z value while other linear variables remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a43dc334-21f6-41fe-95a5-6904a172062f
📒 Files selected for processing (4)
cpp/include/cuopt/mathematical_optimization/constants.hcpp/include/cuopt/mathematical_optimization/utilities/internals.hppcpp/src/barrier/barrier.cucpp/src/dual_simplex/simplex_solver_settings.hpp
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/src/dual_simplex/simplex_solver_settings.hpp
| const std::vector<i_t>& cone_dims) | ||
| { | ||
| f_t min_linear = inf; | ||
| for (i_t j = 0; j < linear_end; ++j) { |
There was a problem hiding this comment.
Can we call values.ensure_positive(epsilon_adjust, linear_mask); instead of this loop?
There was a problem hiding this comment.
Good point. Now we push the entry when min_linear <= epsilon_adjust compared to min_x <= 0.0 in the existing ensure_positive. We should redo the LP/QP test to see if we can use a consistent ensure_positive @rg20.
There was a problem hiding this comment.
I agree it would be better to move this code into ensure_positive. But we should test if this code actually yields an improvement.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/include/cuopt/mathematical_optimization/constants.h (1)
197-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public barrier initial-point strategies.
These new public constants encode externally visible values (
-1automatic,0–2explicit strategies). Add or update user-facing API documentation describing their behavior, including automatic selection for cone problems, and keep it aligned with the solver setting contract.As per path instructions, public-header API changes should have corresponding documentation updates.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/include/cuopt/mathematical_optimization/constants.h` around lines 197 - 200, Document the public constants CUOPT_BARRIER_DUAL_INITIAL_POINT_AUTOMATIC, CUOPT_BARRIER_DUAL_INITIAL_POINT_LUSTIG_MARSTEN_SHANNO, CUOPT_BARRIER_DUAL_INITIAL_POINT_DUAL_LEAST_SQUARES, and CUOPT_BARRIER_DUAL_INITIAL_POINT_SEDUMI_MU in the user-facing API documentation. Describe their externally visible values and strategies, including automatic selection for cone problems, and ensure the documentation matches the solver setting contract.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cpp/include/cuopt/mathematical_optimization/constants.h`:
- Around line 197-200: Document the public constants
CUOPT_BARRIER_DUAL_INITIAL_POINT_AUTOMATIC,
CUOPT_BARRIER_DUAL_INITIAL_POINT_LUSTIG_MARSTEN_SHANNO,
CUOPT_BARRIER_DUAL_INITIAL_POINT_DUAL_LEAST_SQUARES, and
CUOPT_BARRIER_DUAL_INITIAL_POINT_SEDUMI_MU in the user-facing API documentation.
Describe their externally visible values and strategies, including automatic
selection for cone problems, and ensure the documentation matches the solver
setting contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4a030491-2341-476d-bc3f-c9728b76eec0
📒 Files selected for processing (3)
cpp/include/cuopt/mathematical_optimization/constants.hcpp/src/dual_simplex/simplex_solver_settings.hppcpp/src/math_optimization/solver_settings.cu
🚧 Files skipped from review as they are similar to previous changes (2)
- cpp/src/math_optimization/solver_settings.cu
- cpp/src/dual_simplex/simplex_solver_settings.hpp
Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
1fd6a32 to
8489cd4
Compare
| PSLP = CUOPT_PRESOLVE_PSLP | ||
| }; | ||
|
|
||
| /** |
There was a problem hiding this comment.
I think it would be better if you put this in barrier.hpp.
| settings.log.printf("min v %e min z %e\n", data.v.minimum(), data.z.minimum()); | ||
| #endif | ||
|
|
||
| // Residual checks below reflect the final initial point, after positivity shifts. |
There was a problem hiding this comment.
We did the residual checks before the positivity shift to see how well we are solving the least squares problem. We already print out the residuals in the log after the positivity shift. So I think these are not necessary. I would restore the previous code.
ramakrishnap-nv
left a comment
ramakrishnap-nv
left a comment
There was a problem hiding this comment.
Lets add any documentation if we are missing
5 participants
Description
Extend barrier initial-point handling for SOCP and fix augmented-path KKT assembly at initialization.
ensure_initial_point_interiorto push linear variables into the nonnegative orthant and SOC blocks into strict Lorentz cone interior; apply toxandzat the end ofinitial_point.diag.multiply_scalar(-1.0)so the first-call augmented build produces-(Q+D)on the primal block, matchingaugmented_multiplyand the refactor path.max(n+m, device_augmented.n)for sparse SOC expansion, zero-padding extra entries.d_diag_from hostdiag; skip IR for cone problems and direct-free linear variables where operator/factorization would disagree.inv_sqrt_diagmember and initialization.Issue
Checklist