Dual recovery for quadratic constaints in barrier method#1427
Dual recovery for quadratic constaints in barrier method#1427yuwenchen95 wants to merge 3 commits into
Conversation
… 1) standard soc 2) rotated 3) with affine term
yuwenchen95
added
non-breaking
📝 WalkthroughWalkthroughThis PR implements dual recovery for quadratic constraints solved via barrier methods with SOC (second-order cone) transformation. The changes track QC-to-SOC conversion paths, project barrier duals back to QC multipliers, update the Python API to extract QC duals correctly, and add comprehensive testing across all translation paths. ChangesQC Dual Recovery from Barrier SOC Solutions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 6
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/translate_soc.hpp (2)
121-130:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPrecomputed affine-head columns can alias GENERAL-path variables.
qc_affine_headsis reserved up front from the originaln, but GENERAL-path QCs later append new columns immediately fromcsr_A.n. If an earlier QC takes GENERAL and a later QC still takes AFFINE, that laterqc_affine_heads[qi]can point into the columns already consumed by the GENERAL expansion. A mixed ordering like[GENERAL-with-linear, AFFINE-with-linear]therefore reuses the same column for two different lifted variables and corrupts the transformed model. Allocate affine aux columns only after each QC’s path is known, or drive both GENERAL and AFFINE additions from a single monotonic column allocator.As per coding guidelines, "prioritize index-mapping correctness across problem transformations" and "avoid mixing original vs transformed representations inside the same function."
Also applies to: 589-590, 690-697, 850-853
🤖 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 121 - 130, The preallocation of qc_affine_heads and calculation of n_affine_linear_aux before accounting for GENERAL-path column insertions causes aliasing between AFFINE aux columns and GENERAL-expanded columns; update the allocation strategy so affine auxiliary columns are assigned from a single monotonic column allocator that is advanced as both GENERAL and AFFINE additions occur (instead of reserving from the original n). Concretely, remove the upfront loop that sets qc_affine_heads using n and n_affine_linear_aux; introduce a shared column_index (start = n) and, when processing each QC (symbols: qcs, qc_affine_heads, n_affine_linear_aux, n_with_affine_aux, and any use of csr_A.n), allocate columns atomically from column_index for either GENERAL expansions or AFFINE auxes depending on the QC’s chosen path, incrementing column_index accordingly and derive n_with_affine_aux from the final column_index. Ensure all other places mentioned (the other blocks around the referenced lines) use the same allocator to maintain monotonic, non-overlapping indices.Source: Coding guidelines
669-681:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe GENERAL path hard-codes an absolute LDLT pivot tolerance.
right_looking_ldlt(..., 1e-12, ...)makes convex QC recognition depend on the raw magnitude ofH, then immediately assumes any nonzero QCMATRIX row impliesrank >= 1. After model scaling, a perfectly valid PSD quadratic row with small coefficients can be treated as rank-0 or indefinite here, which either hits theassert(rank >= 1)in debug builds or builds the wrong SOC lift in release. This tolerance should be derived fromH/solver tolerances rather than fixed in absolute units.As per coding guidelines, "avoid hardcoded tolerances that fail on degenerate problems; ensure tolerance consistency with solver settings."
🤖 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 669 - 681, right_looking_ldlt is being called with a hardcoded absolute pivot tolerance f_t(1e-12) which makes PSD/indefinite decisions depend on raw H magnitude; replace the fixed 1e-12 with a tolerance derived from H and solver tolerances (e.g., compute a scale like matrix_norm = max_abs_entry(H_csc) or frobenius_norm(H_csc) and set pivot_tol = max(solver_relative_tolerance * matrix_norm, machine_eps * matrix_norm, minimum_relative_tol)), or if ldlt_settings exposes a pivot/pivot_tolerance field use that instead; update the call to right_looking_ldlt(..., pivot_tol, ldlt_start, ldlt_perm, L_factor, D_factor, ldlt_work) and remove the assumption of absolute magnitude so that rank and the subsequent assert(rank >= 1) reflect a scale-aware pivot criterion.Source: Coding guidelines
🤖 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 1393-1465: The apex test and division use an absolute epsilon
which incorrectly collapses small-scale active cones; update
lorentz_cone_block_at_apex and qc_multiplier_from_lorentz_soc_kkt to use a
scale-aware relative tolerance and a guarded denominator: compute a local scale
(e.g. max(cone_inf, std::abs(x[head_col]), uniform_s, f_t(1)) or similar) and
replace the apex check cone_inf <= apex_tol with cone_inf <= apex_tol * scale,
and before returning z_head/(2*s*x_head) check std::abs(x_head) > apex_tol *
scale (or a dedicated denom_eps = apex_tol * scale); if the head is effectively
zero return f_t(0) (or handle as specified) to avoid dividing by a near-zero
value. Ensure references to lorentz_cone_block_at_apex,
qc_multiplier_from_lorentz_soc_kkt, head_col and denom_scale are updated
accordingly.
In `@cpp/src/pdlp/solve.cu`:
- Around line 528-530: The QC multipliers appended by
detail::project_barrier_qcqp_duals_to_model(user_problem, solution) are being
flipped later when convert_dual_simplex_sol() negates the whole dual vector for
maximization problems; move the QC projection call so it happens after
convert_dual_simplex_sol() (or alternatively change convert_dual_simplex_sol()
to only negate the linear-row prefix and not the QC suffix) so that the
projected QC suffix in solution.y retains the user-facing mu >= 0 sign
convention (use user_problem.qc_dual_recovery and solution.y to locate the
affected data).
In `@cpp/tests/socp/general_quadratic_test.cu`:
- Around line 39-55: The test currently calls the helper
solve_qc_soc_barrier_with_dual_recovery which replays recovery logic directly;
instead add a regression that exercises the production QCQP/barrier entrypoint
in cpp/src/pdlp/solve.cu by invoking the public PDLP solve entry that triggers
solve_linear_program_with_barrier and the QC dual recovery hook, then assert the
final recovered dual layout matches expectations; ensure the new test calls the
same user_problem setup used here, runs the full production solve path (not
calling project_barrier_qcqp_duals_to_model or
project_barrier_solution_to_model_variables directly), and verifies recovered
duals/primals are in the expected format and order so regressions catch
missing/incorrect invocations or ordering in the production code.
- Around line 57-64: Add a new regression in
cpp/tests/socp/general_quadratic_test.cu that builds a problem with at least two
quadratic constraints (n_qc >= 2) instead of the single-element qcs vector, then
compute the projected solution and assert the full recovered-dual layout: verify
solution.y has length original_num_rows + n_qc (use
user_problem.original_num_rows) and assert for each qc index i that
solution.y[user_problem.original_num_rows + i] equals the expected QC multiplier
(use the same expected values used for linear dual checks); reference
qc_dual_from_projected_solution and lp_solution_t::y to locate where to add
these assertions so both QC slots are validated rather than only index 0.
In `@python/cuopt/cuopt/linear_programming/problem.py`:
- Around line 2192-2204: The loop that assigns constraint duals still calls
Constraint.compute_slack() for quadratic constraints; since compute_slack() only
uses vindex_coeff_dict (empty for QCMATRIX), set quadratic constraint Slack to
NaN instead of calling compute_slack(). Concretely, inside the loop over
self.constrs (using is_quadratic, qc_dual_idx, linear_row, and DualValue),
branch so that if constr.is_quadratic you assign constr.Slack = float("nan")
(after setting DualValue if provided) and only call constr.compute_slack() for
non-quadratic constraints.
In `@python/cuopt/cuopt/tests/socp/test_socp.py`:
- Around line 89-109: The test _assert_dual_layout currently only checks
internal consistency between Problem.get_dual_solution() and
Constraint.DualValue using the same n_linear + qc_idx mapping, so update it to
accept an explicit expected_duals sequence (or expected_mu_qc) and assert each
constraint's DualValue equals the corresponding provided expected value (using
pytest.approx with DUAL_TOL) rather than deriving the mapping from the solution;
then add at least one new regression test that builds a problem with multiple
quadratic constraints (and one case with n_linear == 0) using the dual_api
builders that document mu_QC, pass the known expected duals into
_assert_dual_layout, and assert the recovered get_dual_solution() layout matches
those numeric expectations to exercise qc_dual_idx > 0 and validate mu_QC
recovery (refer to _assert_dual_layout, Problem.populate_solution,
get_dual_solution, and Constraint.DualValue to locate the relevant code).
---
Outside diff comments:
In `@cpp/src/barrier/translate_soc.hpp`:
- Around line 121-130: The preallocation of qc_affine_heads and calculation of
n_affine_linear_aux before accounting for GENERAL-path column insertions causes
aliasing between AFFINE aux columns and GENERAL-expanded columns; update the
allocation strategy so affine auxiliary columns are assigned from a single
monotonic column allocator that is advanced as both GENERAL and AFFINE additions
occur (instead of reserving from the original n). Concretely, remove the upfront
loop that sets qc_affine_heads using n and n_affine_linear_aux; introduce a
shared column_index (start = n) and, when processing each QC (symbols: qcs,
qc_affine_heads, n_affine_linear_aux, n_with_affine_aux, and any use of
csr_A.n), allocate columns atomically from column_index for either GENERAL
expansions or AFFINE auxes depending on the QC’s chosen path, incrementing
column_index accordingly and derive n_with_affine_aux from the final
column_index. Ensure all other places mentioned (the other blocks around the
referenced lines) use the same allocator to maintain monotonic, non-overlapping
indices.
- Around line 669-681: right_looking_ldlt is being called with a hardcoded
absolute pivot tolerance f_t(1e-12) which makes PSD/indefinite decisions depend
on raw H magnitude; replace the fixed 1e-12 with a tolerance derived from H and
solver tolerances (e.g., compute a scale like matrix_norm = max_abs_entry(H_csc)
or frobenius_norm(H_csc) and set pivot_tol = max(solver_relative_tolerance *
matrix_norm, machine_eps * matrix_norm, minimum_relative_tol)), or if
ldlt_settings exposes a pivot/pivot_tolerance field use that instead; update the
call to right_looking_ldlt(..., pivot_tol, ldlt_start, ldlt_perm, L_factor,
D_factor, ldlt_work) and remove the assumption of absolute magnitude so that
rank and the subsequent assert(rank >= 1) reflect a scale-aware pivot criterion.
🪄 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: 946116db-0559-4da6-9081-e8d07461d3a4
📒 Files selected for processing (7)
cpp/cmake/thirdparty/FindCUDSS.cmakecpp/src/barrier/translate_soc.hppcpp/src/dual_simplex/user_problem.hppcpp/src/pdlp/solve.cucpp/tests/socp/general_quadratic_test.cupython/cuopt/cuopt/linear_programming/problem.pypython/cuopt/cuopt/tests/socp/test_socp.py
| template <typename i_t, typename f_t> | ||
| bool lorentz_cone_block_at_apex(const std::vector<f_t>& x, | ||
| i_t cone_offset, | ||
| i_t cone_dim, | ||
| f_t apex_tol = 1e-6) | ||
| { | ||
| f_t cone_inf = 0; | ||
| for (i_t k = 0; k < cone_dim; ++k) { | ||
| cone_inf = std::max(cone_inf, std::abs(x[cone_offset + k])); | ||
| } | ||
| return cone_inf <= apex_tol; | ||
| } | ||
|
|
||
| /** | ||
| * Recover the SOC KKT multiplier from barrier primal/reduced-cost pair (x_head, z_head). | ||
| * | ||
| * Barrier Lorentz block (uniform scale s > 0): | ||
| * phi = -s * x_head^2 + s * sum_{i in tail} x_i^2 <= 0. | ||
| * | ||
| * At a non-apex point, cone stationarity in the barrier's (x, z) coordinates gives | ||
| * nu_bar = z_head / (2 * s * x_head), | ||
| * where nu_bar is the cone dual in barrier export coordinates (not necessarily the textbook | ||
| * multiplier nu on phi <= 0 with the same sign). LORENTZ / ROTATED / AFFINE paths validate | ||
| * that nu_bar equals the exported user QC dual when the user row is already in phi-form. | ||
| * | ||
| * At the apex (||x||_inf <= apex_tol), grad phi = 0 so the multiplier is not unique; return 0. | ||
| * | ||
| * --- GENERAL (LDLT) path: extra factor of 2 (see path_t::GENERAL in dual projection) --- | ||
| * | ||
| * User QC (QCMATRIX / Python API) is stored without an extra 1/2 on the quadratic part: | ||
| * g(x) = sum_k v_k x_{r_k} x_{c_k} + c^T x - alpha <= 0 | ||
| * (MPS QCMATRIX coeffs are literal; unlike QUADOBJ/QMATRIX objectives where MPS uses 1/2 x^T Q x | ||
| * and the parser applies a 0.5 scale — see mps_parser.cpp value_scale notes.) | ||
| * LDLT builds symmetric H so (1/2) x^T H x equals that QCMATRIX sum; equivalently | ||
| * g(x) = (1/2) x^T H x + c^T x - alpha <= 0. | ||
| * LDLT lift (rank r) adds y, s_0, s_{r+1} and equalities | ||
| * y_k = sqrt(D_k) (L^T P x)_k, | ||
| * s_0 + c^T x = alpha + 1/2, s_{r+1} + c^T x = alpha - 1/2. | ||
| * On that manifold: (1/2)||y||^2 = (1/2) x^T H x and s_0 - s_{r+1} = 1, hence | ||
| * g = (1/2)||y||^2 + 1/2 - s_0 and phi = -s_0^2 + ||y||^2 + s_{r+1}^2 <= 0 | ||
| * describe the same QC inequality (g <= 0 <=> phi <= 0). | ||
| * | ||
| * Sensitivity / scaling on the LDLT coords (not MPS 1/2 objective vs constraint, and not a | ||
| * joint (lambda + 2*nu) = 0 identity with textbook signs): d g / d y_k = y_k because | ||
| * g carries (1/2)||y||^2, while d phi / d y_k = 2 y_k on tails. With lambda_user >= 0 | ||
| * the exported KKT multiplier on g and nu_bar from the head formula satisfy | ||
| * lambda_user = 2 * nu_bar (sign included: both are >= 0 at an active optimum in tests). | ||
| * | ||
| * nu_bar is recovered from (x, z) on the packed cone head via the formula above; export | ||
| * lambda_user = 2 * nu_bar = z_head / (s * x_head). | ||
| */ | ||
| template <typename i_t, typename f_t> | ||
| f_t qc_multiplier_from_lorentz_soc_kkt(const std::vector<f_t>& x, | ||
| const std::vector<f_t>& z, | ||
| i_t cone_offset, | ||
| i_t cone_dim, | ||
| f_t uniform_s, | ||
| f_t apex_tol = 1e-6) | ||
| { | ||
| if (cone_dim <= 0 || uniform_s <= 0) { return f_t(0); } | ||
| const i_t head_col = cone_offset; | ||
| cuopt_expects(head_col >= 0 && head_col < static_cast<i_t>(x.size()), | ||
| error_type_t::RuntimeError, | ||
| "SOC head column index out of range"); | ||
| cuopt_expects(static_cast<i_t>(z.size()) == static_cast<i_t>(x.size()), | ||
| error_type_t::RuntimeError, | ||
| "SOC KKT recovery requires x and z of equal length"); | ||
| // primal solution is 0, so mu is not unique and we return 0. | ||
| if (lorentz_cone_block_at_apex<i_t, f_t>(x, cone_offset, cone_dim, apex_tol)) { return f_t(0); } | ||
|
|
||
| // otherwise, we can compute mu = z_head / (2 s x_head) | ||
| const f_t denom_scale = f_t(2) * uniform_s; | ||
| return z[head_col] / (denom_scale * x[head_col]); |
There was a problem hiding this comment.
The apex check is absolute, so small-scale active cones recover a zero dual.
Any cone block with ||x||_inf <= 1e-6 is treated as apex-degenerate and qc_multiplier_from_lorentz_soc_kkt returns 0. That is scale dependent: a valid active cone at O(1e-9) is not the apex, but this path still zeroes the recovered QC multiplier. The recovery needs a relative/scale-aware degeneracy test, plus a near-zero denominator guard on x_head, instead of a fixed absolute cutoff.
As per coding guidelines, "flag/verify degenerate apex handling and any near-zero/epsilon comparisons used to decide uniqueness of SOC multipliers" and "guard division by zero or near-zero without epsilon check."
🤖 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 1393 - 1465, The apex test
and division use an absolute epsilon which incorrectly collapses small-scale
active cones; update lorentz_cone_block_at_apex and
qc_multiplier_from_lorentz_soc_kkt to use a scale-aware relative tolerance and a
guarded denominator: compute a local scale (e.g. max(cone_inf,
std::abs(x[head_col]), uniform_s, f_t(1)) or similar) and replace the apex check
cone_inf <= apex_tol with cone_inf <= apex_tol * scale, and before returning
z_head/(2*s*x_head) check std::abs(x_head) > apex_tol * scale (or a dedicated
denom_eps = apex_tol * scale); if the head is effectively zero return f_t(0) (or
handle as specified) to avoid dividing by a near-zero value. Ensure references
to lorentz_cone_block_at_apex, qc_multiplier_from_lorentz_soc_kkt, head_col and
denom_scale are updated accordingly.
Source: Coding guidelines
| if (!user_problem.qc_dual_recovery.empty()) { | ||
| detail::project_barrier_qcqp_duals_to_model(user_problem, solution); | ||
| } |
There was a problem hiding this comment.
Keep QC multipliers out of the blanket maximize sign flip.
project_barrier_qcqp_duals_to_model() appends user-facing QC KKT multipliers into solution.y here, but convert_dual_simplex_sol() later negates the entire dual vector for maximization problems. That will flip the recovered QC suffix as well, so maximize QCQPs with quadratic constraints and a linear objective will report μ with the wrong sign.
Please either negate only the linear-row prefix during maximize post-processing, or apply the QC projection after that sign normalization step so the recovered suffix keeps its documented sign convention.
Based on learnings from cpp/src/barrier/translate_soc.hpp, the projected suffix is already defined as user-facing mu >= 0 multipliers.
🤖 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/pdlp/solve.cu` around lines 528 - 530, The QC multipliers appended by
detail::project_barrier_qcqp_duals_to_model(user_problem, solution) are being
flipped later when convert_dual_simplex_sol() negates the whole dual vector for
maximization problems; move the QC projection call so it happens after
convert_dual_simplex_sol() (or alternatively change convert_dual_simplex_sol()
to only negate the linear-row prefix and not the QC suffix) so that the
projected QC suffix in solution.y retains the user-facing mu >= 0 sign
convention (use user_problem.qc_dual_recovery and solution.y to locate the
affected data).
Source: Coding guidelines
| /** Barrier solve on expanded SOC model, then project duals/primals back to the user QCQP. */ | ||
| static void solve_qc_soc_barrier_with_dual_recovery( | ||
| cuopt::linear_programming::dual_simplex::user_problem_t<i_t, f_t>& user_problem, | ||
| cuopt::linear_programming::dual_simplex::lp_solution_t<i_t, f_t>& solution) | ||
| { | ||
| using namespace cuopt::linear_programming::dual_simplex; | ||
| simplex_solver_settings_t<i_t, f_t> settings; | ||
| settings.barrier = true; | ||
| settings.barrier_presolve = true; | ||
| settings.dualize = 0; | ||
|
|
||
| const auto status = solve_linear_program_with_barrier(user_problem, settings, solution); | ||
| ASSERT_EQ(status, lp_status_t::OPTIMAL); | ||
|
|
||
| project_barrier_qcqp_duals_to_model(user_problem, solution); | ||
| project_barrier_solution_to_model_variables(user_problem, solution); | ||
| } |
There was a problem hiding this comment.
Exercise the production QCQP solve path here.
This helper replays the new post-barrier recovery sequence inside the test, so the suite will still pass if cpp/src/pdlp/solve.cu stops invoking QC dual recovery or calls it in the wrong order. Please add at least one regression that goes through the actual QCQP/barrier entrypoint and asserts the final recovered dual layout there.
As per coding guidelines, "Test quality: since this adds multiple recognition/recovery paths ... treat missing edge-case coverage or flaky behavior as HIGH"; PR objectives place the new recovery hook in cpp/src/pdlp/solve.cu.
🤖 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/tests/socp/general_quadratic_test.cu` around lines 39 - 55, The test
currently calls the helper solve_qc_soc_barrier_with_dual_recovery which replays
recovery logic directly; instead add a regression that exercises the production
QCQP/barrier entrypoint in cpp/src/pdlp/solve.cu by invoking the public PDLP
solve entry that triggers solve_linear_program_with_barrier and the QC dual
recovery hook, then assert the final recovered dual layout matches expectations;
ensure the new test calls the same user_problem setup used here, runs the full
production solve path (not calling project_barrier_qcqp_duals_to_model or
project_barrier_solution_to_model_variables directly), and verifies recovered
duals/primals are in the expected format and order so regressions catch
missing/incorrect invocations or ordering in the production code.
Source: Coding guidelines
| static f_t qc_dual_from_projected_solution( | ||
| const cuopt::linear_programming::dual_simplex::user_problem_t<i_t, f_t>& user_problem, | ||
| const cuopt::linear_programming::dual_simplex::lp_solution_t<i_t, f_t>& solution, | ||
| i_t qc_index = 0) | ||
| { | ||
| const i_t m_linear = user_problem.original_num_rows; | ||
| return solution.y[m_linear + qc_index]; | ||
| } |
There was a problem hiding this comment.
Add a multi-QC recovered-layout regression.
Every new case below builds a single-element qcs vector and reads solution.y[original_num_rows + 0], so this never exercises the documented [linear-row duals, one entry per quadratic row] contract beyond the first QC slot. An off-by-one or swapped mapping between multiple quadratic rows would still pass. Please add a case with at least two QCs and assert the full projected y layout.
As per coding guidelines, "validate that each recovered-dual entry corresponds to exactly 'linear-row duals + one entry per quadratic row' layout as documented/expected by tests" and "Add test coverage for edge cases ... when adding new code paths"; PR objectives define the recovered vector as [0, original_num_rows) for linear rows and [original_num_rows, original_num_rows + n_qc) for QC multipliers.
🤖 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/tests/socp/general_quadratic_test.cu` around lines 57 - 64, Add a new
regression in cpp/tests/socp/general_quadratic_test.cu that builds a problem
with at least two quadratic constraints (n_qc >= 2) instead of the
single-element qcs vector, then compute the projected solution and assert the
full recovered-dual layout: verify solution.y has length original_num_rows +
n_qc (use user_problem.original_num_rows) and assert for each qc index i that
solution.y[user_problem.original_num_rows + i] equals the expected QC multiplier
(use the same expected values used for linear dual checks); reference
qc_dual_from_projected_solution and lp_solution_t::y to locate where to add
these assertions so both QC slots are validated rather than only index 0.
Source: Coding guidelines
| for constr in self.constrs: | ||
| if constr.is_quadratic: | ||
| continue | ||
| if dual_sol is not None and len(dual_sol) > linear_row: | ||
| if ( | ||
| dual_sol is not None | ||
| and len(dual_sol) > n_linear + qc_dual_idx | ||
| ): | ||
| constr.DualValue = dual_sol[n_linear + qc_dual_idx] | ||
| qc_dual_idx += 1 | ||
| elif dual_sol is not None and len(dual_sol) > linear_row: | ||
| constr.DualValue = dual_sol[linear_row] | ||
| constr.Slack = constr.compute_slack() | ||
| linear_row += 1 | ||
| if not constr.is_quadratic: | ||
| linear_row += 1 |
There was a problem hiding this comment.
Don’t populate quadratic Slack with the linear-only helper.
This loop now assigns QC duals correctly, but it still calls compute_slack() for quadratic rows even though Constraint.compute_slack() only evaluates vindex_coeff_dict, which is empty for QCMATRIX constraints. The result is that every quadratic constraint gets a bogus slack value instead of the slack at the solved point.
A safe short-term fix is to leave quadratic Slack as NaN here until there is a dedicated quadratic slack evaluator.
Suggested minimal fix
for constr in self.constrs:
if constr.is_quadratic:
if (
dual_sol is not None
and len(dual_sol) > n_linear + qc_dual_idx
):
constr.DualValue = dual_sol[n_linear + qc_dual_idx]
qc_dual_idx += 1
elif dual_sol is not None and len(dual_sol) > linear_row:
constr.DualValue = dual_sol[linear_row]
- constr.Slack = constr.compute_slack()
+ if not constr.is_quadratic:
+ constr.Slack = constr.compute_slack()
if not constr.is_quadratic:
linear_row += 1🤖 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/cuopt/linear_programming/problem.py` around lines 2192 - 2204,
The loop that assigns constraint duals still calls Constraint.compute_slack()
for quadratic constraints; since compute_slack() only uses vindex_coeff_dict
(empty for QCMATRIX), set quadratic constraint Slack to NaN instead of calling
compute_slack(). Concretely, inside the loop over self.constrs (using
is_quadratic, qc_dual_idx, linear_row, and DualValue), branch so that if
constr.is_quadratic you assign constr.Slack = float("nan") (after setting
DualValue if provided) and only call constr.compute_slack() for non-quadratic
constraints.
| def _assert_dual_layout(problem: Problem, solution) -> None: | ||
| """``get_dual_solution()`` matches per-constraint ``DualValue`` on the user model.""" | ||
| dual = solution.get_dual_solution() | ||
| n_linear = sum(1 for c in problem.getConstraints() if not c.is_quadratic) | ||
| n_qc = sum(1 for c in problem.getConstraints() if c.is_quadratic) | ||
| assert len(dual) == n_linear + n_qc | ||
|
|
||
| lin_idx = 0 | ||
| qc_idx = 0 | ||
| for constr in problem.getConstraints(): | ||
| if constr.is_quadratic: | ||
| assert constr.DualValue == pytest.approx( | ||
| dual[n_linear + qc_idx], rel=0, abs=DUAL_TOL | ||
| ) | ||
| qc_idx += 1 | ||
| else: | ||
| assert constr.DualValue == pytest.approx( | ||
| dual[lin_idx], rel=0, abs=DUAL_TOL | ||
| ) | ||
| lin_idx += 1 | ||
|
|
There was a problem hiding this comment.
These dual-API tests only prove self-consistency, not correct QC dual recovery.
_assert_dual_layout() reimplements the same n_linear + qc_dual_idx mapping used by Problem.populate_solution(), and every new dual_api builder has exactly one quadratic row. So this suite never checks the documented mu_QC values from the builder docstrings, never exercises qc_dual_idx > 0, and never hits the n_linear == 0 offset case. A broken projection that returns the wrong QC multiplier, or shifts/duplicates later QC entries, would still pass as long as get_dual_solution() and Constraint.DualValue stay consistent with each other.
Please thread expected dual values into the helper and add at least one regression with multiple quadratic constraints (ideally one with no linear rows) so the layout contract is validated numerically, not just structurally.
Suggested direction
-def _solve_and_assert_dual_api(problem: Problem) -> None:
+def _solve_and_assert_dual_api(
+ problem: Problem, expected_dual: tuple[float, ...]
+) -> None:
solution = _solve_barrier(problem)
+ assert solution.get_dual_solution() == pytest.approx(
+ expected_dual, rel=0, abs=DUAL_TOL
+ )
_assert_dual_layout(problem, solution)
_assert_duals_finite(problem)
assert any(c.is_quadratic for c in problem.getConstraints())Then add one builder with n_qc > 1 (and preferably n_linear == 0) so the QC offset path is exercised beyond the first quadratic row.
As per coding guidelines, Python tests here should validate numerical correctness and edge cases, and the review guide explicitly calls out verifying the exact “linear-row duals + one entry per quadratic row” recovered-dual layout.
Also applies to: 124-128, 136-306
🤖 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/cuopt/tests/socp/test_socp.py` around lines 89 - 109, The test
_assert_dual_layout currently only checks internal consistency between
Problem.get_dual_solution() and Constraint.DualValue using the same n_linear +
qc_idx mapping, so update it to accept an explicit expected_duals sequence (or
expected_mu_qc) and assert each constraint's DualValue equals the corresponding
provided expected value (using pytest.approx with DUAL_TOL) rather than deriving
the mapping from the solution; then add at least one new regression test that
builds a problem with multiple quadratic constraints (and one case with n_linear
== 0) using the dual_api builders that document mu_QC, pass the known expected
duals into _assert_dual_layout, and assert the recovered get_dual_solution()
layout matches those numeric expectations to exercise qc_dual_idx > 0 and
validate mu_QC recovery (refer to _assert_dual_layout,
Problem.populate_solution, get_dual_solution, and Constraint.DualValue to locate
the relevant code).
Source: Coding guidelines
yuwenchen95
requested review from
chris-maes and
rg20
and removed request for
Bubullzz,
aliceb-nv and
tmckayus
1 participant
Description
Barrier solves for QCQP problems with convex quadratic constraints (
QCMATRIXrows) expand each QC into a second-order cone (SOC) model. Previously, dual multipliers for those problems were not returned:solve_qcqplogged a warning and filledget_dual_solution()/ reduced costs with NaN.This PR adds dual recovery that maps barrier SOCP duals back to the original user model after the barrier solve. During SOC conversion (
convert_quadratic_constraints_to_second_order_cones), per-constraint metadata is recorded inuser_problem.qc_dual_recoverydescribing which recognition path was used.project_barrier_qcqp_duals_to_modelthen projects expanded-model duals into the user-facing layout:[0, original_num_rows)— dual multipliers for linear rows[original_num_rows, original_num_rows + n_qc)— KKT multipliers μ ≥ 0 on each quadratic constraint g(x) ≤ 0Supported SOC translation paths:
-s·x_head² + s·‖tail‖² ≤ 0)x₂² + x₃² ≤ x₀·x₁), including degenerate apex casesC++: metadata on
user_problem_t, dual projection intranslate_soc.hpp, hook inpdlp/solve.cuafter barrier solve (before primal projection).Python:
Problemnow assignsConstraint.DualValuefor quadratic rows from the recovered dual vector.Tests: new C++ unit tests in
general_quadratic_test.cuand Pythontest_socp_dual_api_*tests covering all four paths (Lorentz, affine, rotated, general), including degenerate rotated/Lorentz cases.Issue
Checklist