fix(socp): canonicalize QC Q COO for rotated SOC detection (#1435)#1439
Open
yuwenchen95 wants to merge 6 commits into
Open
fix(socp): canonicalize QC Q COO for rotated SOC detection (#1435)#1439yuwenchen95 wants to merge 6 commits into
yuwenchen95 wants to merge 6 commits into
Conversation
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>
yuwenchen95
added
bug
📝 WalkthroughWalkthroughThis PR fixes a bug where rotated second-order cone detection incorrectly required symmetric off-diagonal pairs. It introduces ChangesCanonical QC COO and rotated-SOC bug fix
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
🚥 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.
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/translate_soc.hpp (1)
500-510:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAllow one-tail rotated cones.
Line 507 still rejects
q_nnz == 2, but canonicalx² - 2*y*z <= 0has exactly one tail diagonal plus one cross term. The precedingtail_vars.size() == q_nnz - 1check already validates the canonical count; require at least one tail instead.🐛 Proposed fix
- cuopt_expects(q_nnz >= 3, + cuopt_expects(!tail_vars.empty(), error_type_t::ValidationError, "Quadratic constraint '%s' rotated SOC Q must have at least 1 tail entry", qc.constraint_row_name.c_str());Please also add a one-tail RSOC regression such as
x0^2 - 2*y*z <= 0.As per coding guidelines, incorrect constraint handling/objective computation for RSOC/Q canonicalization is CRITICAL.
🤖 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 500 - 510, The second cuopt_expects check currently requires q_nnz >= 3, but canonical rotated SOC forms like x² - 2*y*z <= 0 have exactly q_nnz == 2 (one tail diagonal and one cross term). Since the preceding tail_vars.size() == q_nnz - 1 check already validates the correct count of tail variables, change the minimum requirement from q_nnz >= 3 to q_nnz >= 2 to allow at least one tail entry. Additionally, add a regression test case with a one-tail RSOC constraint matching the canonical form x0^2 - 2*y*z <= 0.Source: Coding guidelines
🧹 Nitpick comments (2)
cpp/tests/linear_programming/parser_test.cpp (1)
2768-2777: ⚡ Quick winCover mismatched MPS symmetric halves too.
Line 2768 covers a missing reverse entry, but the strict MPS path should also reject both halves when their values differ. Add a small test for
{(0,1,2), (1,0,3)}withrequire_symmetric_offdiagonal_pairs=true.🧪 Proposed test coverage
TEST(qc_coo_canonicalize, mps_requires_matching_symmetric_half) { std::vector<int> rows = {0}; std::vector<int> cols = {1}; std::vector<double> vals = {2.0}; qc_coo_canonicalize_options_t<double> opts; opts.require_symmetric_offdiagonal_pairs = true; opts.constraint_name = "QC0"; EXPECT_THROW(canonicalize_qc_coo(rows, cols, vals, opts), std::logic_error); } + +TEST(qc_coo_canonicalize, mps_rejects_mismatched_symmetric_halves) +{ + std::vector<int> rows = {0, 1}; + std::vector<int> cols = {1, 0}; + std::vector<double> vals = {2.0, 3.0}; + qc_coo_canonicalize_options_t<double> opts; + opts.require_symmetric_offdiagonal_pairs = true; + opts.constraint_name = "QC0"; + EXPECT_THROW(canonicalize_qc_coo(rows, cols, vals, opts), std::logic_error); +}As per coding guidelines, parser/SOCP tests for RSOC-related changes should cover edge cases.
🤖 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/linear_programming/parser_test.cpp` around lines 2768 - 2777, The current test mps_requires_matching_symmetric_half only covers the case where a reverse entry is missing. Add another test function to cover the case where both symmetric halves exist but with mismatched values. Create a new test that calls canonicalize_qc_coo with row indices {0, 1}, column indices {1, 0}, values {2.0, 3.0}, and require_symmetric_offdiagonal_pairs set to true, then verify it throws std::logic_error. This ensures the strict MPS path rejects both matching and mismatching symmetric pairs when the pairs have different values.Source: Coding guidelines
cpp/src/io/quadratic_constraint_coo.cpp (1)
115-117: 💤 Low valueNear-zero diagonal entries not filtered after aggregation.
Diagonal entries that aggregate to near-zero values (e.g.,
+1and-1for the same(i,i)) are still emitted to the output. The aggregation phase filters near-zero inputs (line 63), but aggregated near-zero results on the diagonal are not filtered, unlike off-diagonal entries which uselookup_coeffwith tolerance gating.This is a minor inconsistency; downstream consumers likely handle near-zero coefficients gracefully, but for cleanliness you could add a tolerance check before emitting diagonals.
♻️ Optional: filter near-zero diagonal aggregates
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 115 - 117, The loop iterating through aggregated entries emits diagonal entries (where rc.first == rc.second) without filtering near-zero aggregated values, while the aggregation phase filters near-zero inputs at line 63 and off-diagonal entries use tolerance gating via lookup_coeff. Add a tolerance check before emitting diagonal entries in the emplace_back call to filter out near-zero aggregated diagonal values, ensuring consistency with how off-diagonal entries are handled and maintaining code cleanliness.
🤖 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/translate_soc.hpp`:
- Around line 500-510: The second cuopt_expects check currently requires q_nnz
>= 3, but canonical rotated SOC forms like x² - 2*y*z <= 0 have exactly q_nnz ==
2 (one tail diagonal and one cross term). Since the preceding tail_vars.size()
== q_nnz - 1 check already validates the correct count of tail variables, change
the minimum requirement from q_nnz >= 3 to q_nnz >= 2 to allow at least one tail
entry. Additionally, add a regression test case with a one-tail RSOC constraint
matching the canonical form x0^2 - 2*y*z <= 0.
---
Nitpick comments:
In `@cpp/src/io/quadratic_constraint_coo.cpp`:
- Around line 115-117: The loop iterating through aggregated entries emits
diagonal entries (where rc.first == rc.second) without filtering near-zero
aggregated values, while the aggregation phase filters near-zero inputs at line
63 and off-diagonal entries use tolerance gating via lookup_coeff. Add a
tolerance check before emitting diagonal entries in the emplace_back call to
filter out near-zero aggregated diagonal values, ensuring consistency with how
off-diagonal entries are handled and maintaining code cleanliness.
In `@cpp/tests/linear_programming/parser_test.cpp`:
- Around line 2768-2777: The current test mps_requires_matching_symmetric_half
only covers the case where a reverse entry is missing. Add another test function
to cover the case where both symmetric halves exist but with mismatched values.
Create a new test that calls canonicalize_qc_coo with row indices {0, 1}, column
indices {1, 0}, values {2.0, 3.0}, and require_symmetric_offdiagonal_pairs set
to true, then verify it throws std::logic_error. This ensures the strict MPS
path rejects both matching and mismatching symmetric pairs when the pairs have
different values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 08edaebe-2c73-4a04-be32-3d00d718d07d
⛔ Files ignored due to path filters (2)
cpp/src/grpc/codegen/generated/generated_chunked_arrays_to_problem.incis excluded by!**/generated/**cpp/src/grpc/codegen/generated/generated_proto_to_problem.incis excluded by!**/generated/**
📒 Files selected for processing (31)
cpp/include/cuopt/linear_programming/io/data_model_view.hppcpp/include/cuopt/linear_programming/io/mps_data_model.hppcpp/include/cuopt/linear_programming/optimization_problem_interface.hppcpp/include/cuopt/linear_programming/optimization_problem_utils.hppcpp/src/barrier/translate_soc.hppcpp/src/grpc/codegen/generate_conversions.pycpp/src/grpc/grpc_problem_mapper.cppcpp/src/io/CMakeLists.txtcpp/src/io/data_model_view.cppcpp/src/io/lp_parser.cppcpp/src/io/lp_parser.hppcpp/src/io/mps_data_model.cppcpp/src/io/mps_parser.cppcpp/src/io/mps_writer.cppcpp/src/io/quadratic_constraint_coo.cppcpp/src/io/quadratic_constraint_coo.hppcpp/src/pdlp/cpu_optimization_problem.cppcpp/src/pdlp/optimization_problem.cucpp/tests/linear_programming/c_api_tests/c_api_test.ccpp/tests/linear_programming/c_api_tests/c_api_tests.cppcpp/tests/linear_programming/c_api_tests/c_api_tests.hcpp/tests/linear_programming/parser_test.cppcpp/tests/socp/general_quadratic_test.cudocs/cuopt/source/convex-features.rstdocs/cuopt/source/cuopt-c/convex/convex-c-api.rstdocs/cuopt/source/cuopt-c/convex/convex-examples.rstdocs/cuopt/source/cuopt-c/convex/examples/general_quadratic_example.cdocs/cuopt/source/cuopt-c/convex/examples/rotated_socp_example.cdocs/cuopt/source/cuopt-python/convex/convex-examples.rstdocs/cuopt/source/cuopt-python/convex/examples/rotated_socp_example.pypython/cuopt/cuopt/tests/socp/test_socp.py
mlubin
reviewed
Jun 15, 2026
| * @param vals, rows, cols COO triplets for Q; same length; may all be empty if Q is empty. | ||
| * Stored sorted by (row, col). | ||
| * Stored sorted by (row, col) in canonical form (one entry per variable pair). | ||
| * @param require_symmetric_q_offdiagonal When true (MPS QCMATRIX), each off-diagonal pair must |
Contributor
There was a problem hiding this comment.
What's the motivation for this extra argument? Is it an assertion or does the change the behavior of the function?
Contributor
Author
There was a problem hiding this comment.
This is the flag for additional check with MPS file input that requires symmetric halved inputs for cross terms.
| template <typename i_t, typename f_t> | ||
| void canonicalize_quadratic_constraints( | ||
| std::vector<typename mps_data_model_t<i_t, f_t>::quadratic_constraint_t>& constraints, | ||
| bool require_symmetric_q_offdiagonal = false); |
Contributor
There was a problem hiding this comment.
Same comment on require_symmetric_q_offdiagonal. Is it an assertion or does the change the behavior of the function?
Contributor
Author
There was a problem hiding this comment.
This is the flag for additional check with MPS file input that requires symmetric halved inputs for cross terms.
| (supply ``Q[x_0, x_1] = -2`` in COO, or ``-x_0*x_1`` for the ``<= x_0*x_1`` cone form; see :doc:`/convex-features`) | ||
|
|
||
| For the rotated second-order cone constraints, cuOpt expects the quadratic matrix to be symmetric. | ||
| MPS QCMATRIX requires symmetric off-diagonal halves; the C API accepts a single cross coefficient per variable pair (canonicalized on ingest). |
Contributor
There was a problem hiding this comment.
Be clear what's accepted. If the inputs are canonicalized then it sounds like the C API accepts more than just a single cross coefficient per variable pair.
Contributor
Author
There was a problem hiding this comment.
We allow users to give duplicate cross coefficients per variable pair and then merge them in the canonicalization step.
…im redundant canonicalization comments Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
Replace qc_coo options/wrappers with canonicalize_coo_matrix(rows,cols,vals,bool). Use machine epsilon for zero checks; sum both orientations into one (lo,hi) entry. Test via append_quadratic_constraint; update gRPC codegen call sites. Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/io/quadratic_constraint_coo.cpp`:
- Around line 22-31: The lookup_coeff function uses
std::numeric_limits<f_t>::epsilon() as a bare absolute threshold for filtering
zero coefficients, which is numerically inappropriate for coefficients with
varying magnitudes. Replace the hardcoded epsilon threshold with a fixed
tolerance value of 1e-9, which is consistent with the existing compare_equal()
function used elsewhere in the codebase. Specifically, modify the line where eps
is declared and update the condition in the if statement that checks
std::abs(it->second) <= eps to use the appropriate tolerance instead of epsilon.
🪄 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: de147a41-97d7-4cb3-bee0-acb904661cd9
⛔ Files ignored due to path filters (2)
cpp/src/grpc/codegen/generated/generated_chunked_arrays_to_problem.incis excluded by!**/generated/**cpp/src/grpc/codegen/generated/generated_proto_to_problem.incis excluded by!**/generated/**
📒 Files selected for processing (19)
cpp/include/cuopt/linear_programming/io/data_model_view.hppcpp/include/cuopt/linear_programming/io/mps_data_model.hppcpp/include/cuopt/linear_programming/optimization_problem_interface.hppcpp/src/grpc/codegen/generate_conversions.pycpp/src/io/lp_parser.cppcpp/src/io/mps_data_model.cppcpp/src/io/mps_parser.cppcpp/src/io/mps_writer.cppcpp/src/io/quadratic_constraint_coo.cppcpp/src/io/quadratic_constraint_coo.hppcpp/src/pdlp/cpu_optimization_problem.cppcpp/src/pdlp/optimization_problem.cucpp/tests/linear_programming/c_api_tests/c_api_test.ccpp/tests/linear_programming/parser_test.cppdocs/cuopt/source/convex-features.rstdocs/cuopt/source/cuopt-c/convex/convex-c-api.rstdocs/cuopt/source/cuopt-c/convex/convex-examples.rstdocs/cuopt/source/cuopt-c/convex/examples/general_quadratic_example.cdocs/cuopt/source/cuopt-c/convex/examples/rotated_socp_example.c
💤 Files with no reviewable changes (2)
- cpp/include/cuopt/linear_programming/optimization_problem_interface.hpp
- cpp/tests/linear_programming/c_api_tests/c_api_test.c
✅ Files skipped from review due to trivial changes (3)
- docs/cuopt/source/cuopt-c/convex/convex-c-api.rst
- docs/cuopt/source/cuopt-c/convex/convex-examples.rst
- cpp/include/cuopt/linear_programming/io/data_model_view.hpp
🚧 Files skipped from review as they are similar to previous changes (8)
- cpp/src/pdlp/cpu_optimization_problem.cpp
- docs/cuopt/source/cuopt-c/convex/examples/rotated_socp_example.c
- cpp/src/io/mps_parser.cpp
- cpp/src/io/mps_writer.cpp
- cpp/include/cuopt/linear_programming/io/mps_data_model.hpp
- cpp/src/io/mps_data_model.cpp
- cpp/src/io/lp_parser.cpp
- cpp/src/grpc/codegen/generate_conversions.py
Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
mlubin
reviewed
Jun 21, 2026
mlubin
left a comment
mlubin
left a comment
Contributor
There was a problem hiding this comment.
Big improvement!
We have test coverage of the C interface, what about also adding tests covering the python example in the original issue?
| * SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION & AFFILIATES. All rights | ||
| * reserved. SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
| #pragma once |
Contributor
There was a problem hiding this comment.
Consider renaming this file; the contents don't mention anything about quadratic constraints.
Contributor
Author
There was a problem hiding this comment.
I moved the canonicalization function into mps_parser_* files though it is also used for lp file input.
…out only for mps files Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cpp/src/pdlp/optimization_problem.cu (1)
13-13: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winMove the canonical COO declaration out of the MPS parser-internal header.
optimization_problem.cuis a PDLP ingestion path, so depending onmps_parser_internal.hppleaks parser internals into solver code. Since this helper is now shared across PDLP, gRPC, data-model, and writer paths, exposecanonicalize_coo_matrixfrom a neutral quadratic-COO utility header and keep MPS-only validation in the parser internals.Suggested include direction after moving the declaration
-#include <mps_parser_internal.hpp> +#include <quadratic_constraint_coo.hpp>🤖 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/optimization_problem.cu` at line 13, The include of `mps_parser_internal.hpp` in optimization_problem.cu exposes parser internals to the solver code. Move the `canonicalize_coo_matrix` declaration from the mps_parser_internal header to a neutral quadratic-COO utility header (either create a new one or use an existing shared header), keeping MPS-specific validation logic in mps_parser_internal.hpp. Then update the include statement in optimization_problem.cu to reference the new neutral header instead of mps_parser_internal.hpp to properly separate solver concerns from parser implementation details.
🤖 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/io/mps_parser.cpp`:
- Around line 172-174: The code uses machine epsilon (via
std::numeric_limits<f_t>::epsilon()) as an absolute zero cutoff threshold for Q
coefficients, which aggressively removes valid small non-zero values and
incorrectly changes the quadratic constraint during canonicalization. Replace
this hardcoded machine epsilon check with an explicit parser or solver tolerance
constant that employs relative scaling instead of absolute machine epsilon as
the storage-level zero threshold. Update the condition in the zero check that
compares std::abs(it->second) <= eps to use this new explicit tolerance, and
apply the same fix to all other occurrences of similar epsilon-based comparisons
in the file (around lines 216-217, 276-277, and 295-300 as noted in the
comment).
---
Nitpick comments:
In `@cpp/src/pdlp/optimization_problem.cu`:
- Line 13: The include of `mps_parser_internal.hpp` in optimization_problem.cu
exposes parser internals to the solver code. Move the `canonicalize_coo_matrix`
declaration from the mps_parser_internal header to a neutral quadratic-COO
utility header (either create a new one or use an existing shared header),
keeping MPS-specific validation logic in mps_parser_internal.hpp. Then update
the include statement in optimization_problem.cu to reference the new neutral
header instead of mps_parser_internal.hpp to properly separate solver concerns
from parser implementation details.
🪄 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: d403fc47-c696-4cf8-bfd3-0119c147e514
📒 Files selected for processing (12)
cpp/include/cuopt/linear_programming/io/mps_data_model.hppcpp/src/grpc/grpc_problem_mapper.cppcpp/src/io/mps_data_model.cppcpp/src/io/mps_parser.cppcpp/src/io/mps_parser_internal.hppcpp/src/io/mps_writer.cppcpp/src/pdlp/cpu_optimization_problem.cppcpp/src/pdlp/optimization_problem.cucpp/tests/linear_programming/parser_test.cppcpp/tests/socp/general_quadratic_test.cudocs/cuopt/source/convex-features.rstpython/cuopt/cuopt/tests/socp/test_socp.py
✅ Files skipped from review due to trivial changes (1)
- cpp/src/grpc/grpc_problem_mapper.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- cpp/tests/socp/general_quadratic_test.cu
- cpp/src/pdlp/cpu_optimization_problem.cpp
- docs/cuopt/source/convex-features.rst
- cpp/tests/linear_programming/parser_test.cpp
Comment on lines
+172
to
+174
| const f_t eps = std::numeric_limits<f_t>::epsilon(); | ||
| const auto it = agg.find({r, c}); | ||
| if (it == agg.end() || std::abs(it->second) <= eps) { return f_t(0); } |
There was a problem hiding this comment.
Preserve non-zero Q coefficients during canonicalization.
Using numeric_limits<f_t>::epsilon() as an absolute zero cutoff drops valid small coefficients; with float, any |Q_ij| <= ~1e-7 is silently removed, so canonicalization can change the quadratic constraint instead of only normalizing its COO representation.
Suggested direction
- const f_t eps = std::numeric_limits<f_t>::epsilon();
const auto it = agg.find({r, c});
- if (it == agg.end() || std::abs(it->second) <= eps) { return f_t(0); }
+ if (it == agg.end()) { return f_t(0); }
return it->second;- if (std::abs(v) <= std::numeric_limits<f_t>::epsilon()) { continue; }
+ if (v == f_t(0)) { continue; }- const f_t eps = std::numeric_limits<f_t>::epsilon();
const f_t v_lo_hi = qcmatrix_lookup_coeff(agg, lo, hi);
const f_t v_hi_lo = qcmatrix_lookup_coeff(agg, hi, lo);
const f_t cross = v_lo_hi + v_hi_lo;
- if (std::abs(cross) > eps) { out.emplace_back(lo, hi, cross); }
+ if (cross != f_t(0)) { out.emplace_back(lo, hi, cross); }If symmetry comparison still needs tolerance, use an explicit parser/solver tolerance with relative scaling rather than treating machine epsilon as a storage-level zero threshold.
As per coding guidelines, “Prevent numerical instability (overflow, underflow, precision loss) producing wrong results” and avoid hardcoded tolerances that fail on degenerate problems.
Also applies to: 216-217, 276-277, 295-300
🤖 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/mps_parser.cpp` around lines 172 - 174, The code uses machine
epsilon (via std::numeric_limits<f_t>::epsilon()) as an absolute zero cutoff
threshold for Q coefficients, which aggressively removes valid small non-zero
values and incorrectly changes the quadratic constraint during canonicalization.
Replace this hardcoded machine epsilon check with an explicit parser or solver
tolerance constant that employs relative scaling instead of absolute machine
epsilon as the storage-level zero threshold. Update the condition in the zero
check that compares std::abs(it->second) <= eps to use this new explicit
tolerance, and apply the same fix to all other occurrences of similar
epsilon-based comparisons in the file (around lines 216-217, 276-277, and
295-300 as noted in the comment).
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Quadratic-constraint
Qis now stored in canonical COO internally: one coefficient per variable pair (e.g. a single-2for2·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). MPSQCMATRIXstill 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·ufor||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).Issue
closes #1435
Checklist
Test plan
./build.sh libcuoptwith conda env activectest --test-dir cpp/build -R 'SOCP_TEST|MPS_PARSER_TEST|C_API_TEST'qc_cross_term_stored_canonicaland related LP/MPS QC COO testsC_API_TESTpytest -v cuopt/cuopt/tests/socp/test_socp.py -k rotated_soc-2*t*ucross term → objective ≈ √2 (not ~0)