Skip to content

fix(qcqp): canonical QC COO ingestion without symmetrization && reject nonconvex general-path constraints - #1437

Closed
yuwenchen95 wants to merge 2 commits into
NVIDIA:mainfrom
yuwenchen95:fix-qcqp-input
Closed

fix(qcqp): canonical QC COO ingestion without symmetrization && reject nonconvex general-path constraints#1437
yuwenchen95 wants to merge 2 commits into
NVIDIA:mainfrom
yuwenchen95:fix-qcqp-input

Conversation

@yuwenchen95

Copy link
Copy Markdown
Contributor

Description

This PR fixes two related QCQP input / conversion bugs:

#1435 — Canonical quadratic-constraint Q COO and RSOC detection

Quadratic-constraint Q is now stored in canonical COO internally: one coefficient per variable pair (e.g. a single -2 for 2·x₀·x₁), with symmetric MPS halves merged at ingest. Canonicalization runs at ingest boundaries (MPS/LP parser, C API, Python→solver, gRPC, PDLP CPU/GPU problem setup). MPS QCMATRIX still accepts symmetric halves; the MPS writer expands canonical cross terms back to symmetric form on export.

The RSOC fast path now accepts a single eligible cross term (e.g. -2·t·u for ||tail||² ≤ 2·t·u) instead of requiring two symmetric off-diagonal entries. Previously, natural Python/API forms were misrouted to the general path or failed pattern matching, producing wrong optima (e.g. ~0 instead of √2).

#1434 — Reject nonconvex quadratics on the general path

Cross-only indefinite constraints (e.g. 2·x₀·x₁ ≤ 0.5 with H = [[0,2],[2,0]]) previously passed convexity checks: diagonal LDLT returned rank = 0 without INDEFINITE_MATRIX_RETURN, and a degenerate r = 0 SOC lift silently dropped the quadratic term (wrong optimum reported as Optimal).

Fixes:

  • right_looking_ldlt: return INDEFINITE_MATRIX_RETURN when factorization stalls at rank = 0 on a nonzero matrix.
  • translate_soc.hpp (general path): cuopt_expects(rank >= 1, …) before building the SOC lift.

Adds LDLT and SOC-conversion unit tests for the cross-only indefinite case.

Issue

closes#1435
closes#1434

Checklist

  • I am familiar with the Contributing Guidelines.
  • Testing
    • New or existing tests cover these changes
    • Added tests
    • Created an issue to follow-up
    • NA
  • Documentation
    • The documentation is up to date with these changes
    • Added new documentation
    • NA

Test plan

  • ./build.sh libcuopt with conda env active, then verify cuopt_cli uses installed libcuopt from $CONDA_PREFIX
  • C++: ctest --test-dir cpp/build -R 'DUAL_SIMPLEX_TEST|SOCP_TEST|MPS_PARSER_TEST|C_API_TEST'
  • C++ GTest filters:
    • right_looking_ldlt.indefinite_cross_only_2x2
    • general_quadratic.rejects_cross_only_indefinite
    • general_quadratic.rejects_non_convex
    • parser / C API canonical QC COO tests (qc_cross_term_stored_canonical, rotated SOC C API tests)
  • Python: pytest -v cuopt/cuopt/tests/socp/test_socp.py -k rotated_soc
  • Manual [BUG] Rotated second order cone detection assumes symmetrized inputs #1435 repro: rotated SOC with single -2*t*u cross term → objective ≈ √2
  • Manual [BUG] incorrect "optimal" solution on nonconvex qcqp #1434 repro: datasets/qcqp/issue_1434_nonconvex.lpValidationError (not Optimal obj=2)

Store quadratic-constraint Q in canonical COO (one cross coefficient per
variable pair) and run canonicalization at ingest boundaries (MPS/LP,
C API, Python→solver, gRPC, MPS export). The RSOC fast path now accepts
a single cross term (e.g. -2*x0*x1 for ||tail||^2 <= 2*x0*x1) instead of
requiring symmetric MPS-style halves, which previously routed natural
API forms to the general QC path and produced wrong optima.
MPS QCMATRIX still accepts symmetric halves; the writer expands canonical
cross terms back to MPS form on export. Adds tests and doc updates for
C API, parser, and examples.
Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
…path
Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
@yuwenchen95
yuwenchen95 requested review from a team as code ownersJune 15, 2026 16:57
@yuwenchen95
yuwenchen95 requested a review from Iroy30June 15, 2026 16:57
@yuwenchen95yuwenchen95 added bug Something isn't working non-breaking Introduces a non-breaking change improvement Improves an existing functionality labels Jun 15, 2026
@coderabbitai

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

A new canonicalize_qc_coo primitive consolidates quadratic-constraint COO data into a single canonical "one entry per variable pair" form. This replaces per-site symmetric expansion in the LP parser, MPS parser/writer, gRPC, PDLP, and data-model-view ingestion paths. The LDLᵀ factorizer gains detection of cross-only indefinite matrices, the SOC translator is updated for the new single-entry cross-term representation, and all tests and documentation are updated accordingly.

Changes

Canonical QC COO representation

Layer / File(s)Summary
COO canonicalization interface and implementation
cpp/src/io/quadratic_constraint_coo.hpp, cpp/src/io/quadratic_constraint_coo.cpp, cpp/src/io/CMakeLists.txt
New header defines qc_coo_canonicalize_options_t, declares canonicalize_qc_coo, and provides canonicalize_qc_entry. Implementation aggregates COO into a hash map, enforces optional symmetry, drops near-zero values, sorts output, and is registered in the build.
mps_data_model API: append and canonicalize_quadratic_constraints
cpp/include/cuopt/linear_programming/io/mps_data_model.hpp, cpp/src/io/mps_data_model.cpp
append_quadratic_constraint gains require_symmetric_q_offdiagonal parameter; inline sort is replaced by canonicalize_qc_entry. New canonicalize_quadratic_constraints function iterates a vector of constraints and canonicalizes each.
LP and MPS parser ingestion: defer/delegate canonicalization
cpp/src/io/lp_parser.cpp, cpp/src/io/lp_parser.hpp, cpp/src/io/mps_parser.cpp, cpp/src/io/mps_writer.cpp
LP parser removes build_symmetric_q_coo and passes raw triples to append_quadratic_constraint. MPS parser passes true for symmetric enforcement. MPS writer canonicalizes before emission and splits off-diagonal values to v/2 symmetric pairs.
Problem population helpers and interface docs
cpp/include/cuopt/linear_programming/optimization_problem_utils.hpp, cpp/include/cuopt/linear_programming/optimization_problem_interface.hpp, cpp/include/cuopt/linear_programming/io/data_model_view.hpp, cpp/src/io/data_model_view.cpp
populate_from_data_model_view calls io::canonicalize_quadratic_constraints before storing. Interface docs clarify canonical COO contract and on-ingest canonicalization.
PDLP and gRPC ingestion wiring
cpp/src/pdlp/cpu_optimization_problem.cpp, cpp/src/pdlp/optimization_problem.cu, cpp/src/grpc/grpc_problem_mapper.cpp, cpp/src/grpc/codegen/generate_conversions.py
PDLP GPU and CPU paths call io::canonicalize_qc_entry before storing each constraint. gRPC mapper includes the new header; codegen emits io::canonicalize_qc_entry in both unary and chunked decode paths for quadratic_constraints.
LDLᵀ factorizer: detect cross-only indefinite matrix
cpp/src/dual_simplex/right_looking_lu.cpp, cpp/src/dual_simplex/right_looking_lu.hpp, cpp/tests/dual_simplex/unit_tests/right_looking_ldlt.cpp
Caches input_nnz before factorization; returns INDEFINITE_MATRIX_RETURN when no diagonal pivot is found on a nonzero matrix with zero pivots produced. New unit test validates [[0,2],[2,0]] cross-only case.
SOC translator: single cross-term path and indefinite rejection
cpp/src/barrier/translate_soc.hpp
Re-describes rotated-SOC Q as a single (head0,head1,-2*d) entry; introduces rotated_soc_cross_eligible predicate; rewrites rotated-SOC specialized-path to require exactly one off-diagonal entry; upgrades general-path LDLT rank check to cuopt_expects(rank >= 1).
Parser, canonicalization, and SOCP unit tests
cpp/tests/linear_programming/parser_test.cpp, cpp/tests/socp/general_quadratic_test.cu, python/cuopt/cuopt/tests/socp/test_socp.py
Updates LP cross-term expectations to canonical form. Adds qc_coo_canonicalize tests. Adds rejects_cross_only_indefinite test. Updates rotated-SOC tests to 3-entry canonical encoding.
Rotated SOC C API end-to-end tests
cpp/tests/linear_programming/c_api_tests/c_api_test.c, cpp/tests/linear_programming/c_api_tests/c_api_tests.h, cpp/tests/linear_programming/c_api_tests/c_api_tests.cpp
Adds two new rotated SOC C API tests using the canonical single cross-term encoding, asserting optimal termination, objective, and solution values.
Documentation: canonical cross-term conventions
docs/cuopt/source/convex-features.rst, docs/cuopt/source/cuopt-c/convex/..., docs/cuopt/source/cuopt-python/convex/...
Updates RSOC formulation, Python/C API examples, and MPS guidance throughout to reflect the new single-entry canonical cross-term convention vs symmetric-half MPS convention.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • NVIDIA/cuopt#1290: Directly related at the code level — the quadratic-to-SOC conversion path (translate_soc.hpp) and QCMATRIX handling modified in that PR are the same components being restructured here for the new canonical cross-term representation.

Suggested reviewers

  • chris-maes
  • akifcorduk
  • rgsl888prabhu
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring coverage is 19.61% which is insufficient. The required threshold is 80.00%.Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly summarizes the main changes: fixing canonical QC COO ingestion and rejecting nonconvex general-path constraints, matching the primary objectives described in the PR.
Description check✅ PassedThe description is comprehensive and directly related to the changeset, explaining both bug fixes (#1435 and #1434), their solutions, and the testing approach.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
cpp/src/io/quadratic_constraint_coo.cpp (1)

67-117: 💤 Low value

Near-zero aggregated diagonal entries are emitted without tolerance check.

Off-diagonal entries use lookup_coeff which filters by opts.tol, but diagonal entries (Line 116) are added directly from agg without re-checking against tolerance. If two diagonal entries +0.5*tol and -0.5*tol aggregate to a near-zero value, it will still be emitted.

Consider filtering diagonal entries by tolerance before adding to output:

🔧 Suggested fix
 for (const auto& [rc, v] : agg) {
- if (rc.first == rc.second) { out.emplace_back(rc.first, rc.second, v); }+ if (rc.first == rc.second && std::abs(v) > opts.tol) {+ out.emplace_back(rc.first, rc.second, v);+ }
}
🤖 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/io/quadratic_constraint_coo.cpp` around lines 67 - 117, The second
loop that processes diagonal entries (where rc.first == rc.second) adds
aggregated values directly to the output without checking against opts.tol,
unlike off-diagonal entries which use lookup_coeff to filter by tolerance. Add a
tolerance check in the diagonal entry loop to only emit diagonal entries when
std::abs(v) > opts.tol, ensuring near-zero aggregated diagonal values are
filtered out consistently with the off-diagonal handling.
🤖 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/src/io/quadratic_constraint_coo.cpp`:
- Around line 67-117: The second loop that processes diagonal entries (where
rc.first == rc.second) adds aggregated values directly to the output without
checking against opts.tol, unlike off-diagonal entries which use lookup_coeff to
filter by tolerance. Add a tolerance check in the diagonal entry loop to only
emit diagonal entries when std::abs(v) > opts.tol, ensuring near-zero aggregated
diagonal values are filtered out consistently with the off-diagonal handling.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 053c6a6f-8393-4be2-b9ee-a75305726be2

📥 Commits

Reviewing files that changed from the base of the PR and between 03fe3fc and d6f334f.

⛔ Files ignored due to path filters (2)
  • cpp/src/grpc/codegen/generated/generated_chunked_arrays_to_problem.inc is excluded by !**/generated/**
  • cpp/src/grpc/codegen/generated/generated_proto_to_problem.inc is excluded by !**/generated/**
📒 Files selected for processing (34)
  • cpp/include/cuopt/linear_programming/io/data_model_view.hpp
  • cpp/include/cuopt/linear_programming/io/mps_data_model.hpp
  • cpp/include/cuopt/linear_programming/optimization_problem_interface.hpp
  • cpp/include/cuopt/linear_programming/optimization_problem_utils.hpp
  • cpp/src/barrier/translate_soc.hpp
  • cpp/src/dual_simplex/right_looking_lu.cpp
  • cpp/src/dual_simplex/right_looking_lu.hpp
  • cpp/src/grpc/codegen/generate_conversions.py
  • cpp/src/grpc/grpc_problem_mapper.cpp
  • cpp/src/io/CMakeLists.txt
  • cpp/src/io/data_model_view.cpp
  • cpp/src/io/lp_parser.cpp
  • cpp/src/io/lp_parser.hpp
  • cpp/src/io/mps_data_model.cpp
  • cpp/src/io/mps_parser.cpp
  • cpp/src/io/mps_writer.cpp
  • cpp/src/io/quadratic_constraint_coo.cpp
  • cpp/src/io/quadratic_constraint_coo.hpp
  • cpp/src/pdlp/cpu_optimization_problem.cpp
  • cpp/src/pdlp/optimization_problem.cu
  • cpp/tests/dual_simplex/unit_tests/right_looking_ldlt.cpp
  • cpp/tests/linear_programming/c_api_tests/c_api_test.c
  • cpp/tests/linear_programming/c_api_tests/c_api_tests.cpp
  • cpp/tests/linear_programming/c_api_tests/c_api_tests.h
  • cpp/tests/linear_programming/parser_test.cpp
  • cpp/tests/socp/general_quadratic_test.cu
  • docs/cuopt/source/convex-features.rst
  • docs/cuopt/source/cuopt-c/convex/convex-c-api.rst
  • docs/cuopt/source/cuopt-c/convex/convex-examples.rst
  • docs/cuopt/source/cuopt-c/convex/examples/general_quadratic_example.c
  • docs/cuopt/source/cuopt-c/convex/examples/rotated_socp_example.c
  • docs/cuopt/source/cuopt-python/convex/convex-examples.rst
  • docs/cuopt/source/cuopt-python/convex/examples/rotated_socp_example.py
  • python/cuopt/cuopt/tests/socp/test_socp.py

@yuwenchen95yuwenchen95 removed the improvement Improves an existing functionality label Jun 15, 2026
@mlubin

Copy link
Copy Markdown
Contributor

If fixing two separate issues, please open two separate pull requests especially when the fixes are large.

@yuwenchen95
yuwenchen95 deleted the fix-qcqp-input branch August 10, 2026 07:19
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugSomething isn't workingnon-breakingIntroduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Rotated second order cone detection assumes symmetrized inputs [BUG] incorrect "optimal" solution on nonconvex qcqp

4 participants

@yuwenchen95@mlubin@rg20@chris-maes