Fix SOC index collision and simplify QC-to-SOC conversion by routing linear QCs through general LDLT path#1564
Fix SOC index collision and simplify QC-to-SOC conversion by routing linear QCs through general LDLT path#1564yuwenchen95 wants to merge 3 commits into
Conversation
Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
yuwenchen95
added
bug
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe SOC conversion now routes quadratic constraints with linear parts through the general LDLT/PSD lift, refines standard and rotated fast-path checks, removes affine auxiliary-variable linking, and adds mixed-conversion barrier-solver coverage. ChangesSOC conversion
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cpp/src/barrier/translate_soc.hpp (1)
155-181: 🚀 Performance & Scalability | 🔵 TrivialGeneral-path routing for affine QCs trades performance for correctness.
Every QC with a nonzero linear part now goes through dense LDLT factorization plus
rank+2new variables/rows, instead of the previous lightweight single auxiliary variable + link row. This is the right fix for the index-collision bug, but for models with many simple affine QCs this is a meaningful cost increase (dense local Hessian assembly, LDLT factorization per QC). Worth keeping an eye on for large models with many affine-style QCs.🤖 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/translate_soc.hpp` around lines 155 - 181, Review the nonzero-linear-term routing around has_linear_part so simple affine quadratic constraints can retain the lightweight single-auxiliary-variable and link-row path when safe, while still using the general LDLT rank+2 path for cases affected by index collisions. Preserve the existing validation and ensure the collision-safe path remains correct.
🤖 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.
Inline comments:
In `@cpp/src/barrier/translate_soc.hpp`:
- Around line 320-346: The degenerate SOC conversion in the
pos_diag_rows.empty() branch must validate the head variable’s nonnegative lower
bound before constructing the one-element cone. Reuse the existing head-bound
validation from the multi-term SOC path, applying it before setting head or
accepting the q_nnz == 1 conversion, while preserving the current diagonal and
tail validations.
- Around line 274-279: Update the standard_soc_eligible condition to require
exactly one negative diagonal row by changing the neg_diag_rows size check from
allowing zero or one to requiring one. Leave rotated_soc_eligible and the
general-path selection unchanged so all-positive-diagonal cases use the LDLT
fallback.
---
Nitpick comments:
In `@cpp/src/barrier/translate_soc.hpp`:
- Around line 155-181: Review the nonzero-linear-term routing around
has_linear_part so simple affine quadratic constraints can retain the
lightweight single-auxiliary-variable and link-row path when safe, while still
using the general LDLT rank+2 path for cases affected by index collisions.
Preserve the existing validation and ensure the collision-safe path remains
correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b698de37-fe2d-4f66-8eb6-a8a989a5edc6
📒 Files selected for processing (2)
cpp/src/barrier/translate_soc.hppcpp/tests/socp/general_quadratic_test.cu
| // 2) rotated SOC — one off-diagonal cross -2*d on two heads plus tail diagonals +s | ||
| // (d > 0, s > 0; stored as Q cross (head0, head1, -2*d); lift uses sqrt(d/s)) | ||
| // | ||
| // General path (LDLT lift on H = Q + Q^T, must be PSD): |
There was a problem hiding this comment.
It's the general path for quadratic constraints where we need to factorize the quadratic cost. We are using LDL factorization for factorization and positive semidefinite check.
There was a problem hiding this comment.
I wrote that code. But I'm not sure how the word "lift" relates to that code.
There was a problem hiding this comment.
I think we considered any introduction of new variables as a lift.
Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
CI Test Summary✅ All 31 test job(s) passed. |
Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
3 participants
Description
Simplify SOC conversion in
translate_soc.hppand fix a variable-index collision when mixing general convex QCs with affine-style QCs (e.g.hub1.mps.gz).Bug fix: When a general LDLT QC was converted before an affine QC with a linear part, both paths could allocate overlapping auxiliary column indices. This produced
SOC variable index ... collisionat runtime.Refactor — merge affine fast path into general LDLT path: QCs with a nonzero linear part (
COLUMNS) no longer use a separate affine auxiliary-variable + linking-row fast path. They always route through the general LDLT lift, which handles the linear term directly in thes_0/s_{r+1}rows. This removes:qc_affine_heads/n_affine_linear_auxpre-allocationRefactor — clearer path routing: Classification is inverted to match the header comment: a QC is eligible for standard or rotated SOC fast paths when it matches those patterns (
rhs = 0, no linear part); otherwise it uses the general path.Tests: Add
mixed_general_and_affine_soc_conversionregression test that reproduces the pre-fix failure mode (general QC first, affine QC second).Issue
Checklist