Add QCMATRIX: parsing and quadratic-constraint model#1105
Open
yuwenchen95 wants to merge 2 commits intoNVIDIA:mainfrom
Open
Add QCMATRIX: parsing and quadratic-constraint model#1105yuwenchen95 wants to merge 2 commits intoNVIDIA:mainfrom
yuwenchen95 wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
📝 WalkthroughWalkthroughAdded QCMATRIX support: a CSR-based quadratic_constraint_matrix_t in the data model, public append/get/has APIs, parser state and parsing logic for QCMATRIX blocks, CSR-builder refactor for configurable symmetrization/scale, and a unit test for the new API. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
🧹 Nitpick comments (2)
cpp/libmps_parser/src/mps_parser.cpp (1)
1328-1347: Consider using a map for O(1) duplicate detection.The
flush_qcmatrix_blockfunction uses a linear scan to detect duplicate QCMATRIX blocks for the same constraint row. For files with many quadratic constraints, this results in O(n²) complexity.Since this is in the parser (not a hot path during solving), the impact is likely minimal for typical use cases. This is a minor optimization opportunity.
♻️ Optional: Use unordered_set for duplicate detection
Add a member
std::unordered_set<i_t> qcmatrix_seen_rows_;and check/insert in O(1):+ // In mps_parser.hpp, add: + std::unordered_set<i_t> qcmatrix_seen_rows_{}; // In flush_qcmatrix_block: - for (const auto& b : qcmatrix_blocks_) { - mps_parser_expects(b.constraint_row_id != qcmatrix_active_row_id_, - error_type_t::ValidationError, - "Duplicate QCMATRIX block for the same constraint row (index %d)", - static_cast<int>(qcmatrix_active_row_id_)); - } + mps_parser_expects(qcmatrix_seen_rows_.insert(qcmatrix_active_row_id_).second, + error_type_t::ValidationError, + "Duplicate QCMATRIX block for the same constraint row (index %d)", + static_cast<int>(qcmatrix_active_row_id_));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/libmps_parser/src/mps_parser.cpp` around lines 1328 - 1347, The current flush_qcmatrix_block implementation does O(n) scans over qcmatrix_blocks_ to detect duplicate qcmatrix_active_row_id_, causing O(n^2) behavior; change to use an unordered_set to track seen constraint row ids: add a member std::unordered_set<i_t> qcmatrix_seen_rows_, on flush check if qcmatrix_active_row_id_ is already in qcmatrix_seen_rows_ (and throw the same mps_parser_expects error if found), otherwise insert the id and append the block to qcmatrix_blocks_ as before, then clear qcmatrix_current_entries_ and reset qcmatrix_active_row_id_. Ensure the set is updated exactly where blocks are added so duplicate detection becomes O(1) per block.cpp/libmps_parser/tests/mps_parser_test.cpp (1)
862-901: Good API test coverage, consider adding edge case tests.The test validates the basic
append_quadratic_constraint_matrixAPI with two matrices and checks storage correctness. This provides good coverage of the happy path.Consider adding tests for:
- Empty QCMATRIX block (0 entries)
- Duplicate constraint row detection (should fail per
flush_qcmatrix_blocklogic)- Parser integration test with actual QCMATRIX file content (could be added in follow-up)
💡 Example edge case test
TEST(qps_parser, qcmatrix_empty_entries) { using model_t = mps_data_model_t<int, double>; model_t model; // Empty CSR with just offsets const std::vector<double> empty_values; const std::vector<int> empty_indices; const std::vector<int> offsets = {0, 0}; // 1 row, 0 entries model.append_quadratic_constraint_matrix( 0, empty_values.data(), 0, empty_indices.data(), 0, offsets.data(), offsets.size()); ASSERT_TRUE(model.has_quadratic_constraints()); const auto& qcs = model.get_quadratic_constraint_matrices(); ASSERT_EQ(1u, qcs.size()); EXPECT_TRUE(qcs[0].values.empty()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/libmps_parser/tests/mps_parser_test.cpp` around lines 862 - 901, Add edge-case tests for the quadratic-constraint API: (1) an "empty entries" test that calls append_quadratic_constraint_matrix with empty values/indices and offsets like {0,0} and then asserts has_quadratic_constraints() is true and get_quadratic_constraint_matrices() contains a matrix with empty values/indices; (2) a "duplicate constraint row" test that attempts to append two matrices with the same constraint_row_index and asserts the parser/model rejects or flags the duplicate consistent with flush_qcmatrix_block behavior; and (3) a lightweight parser integration test that feeds a small QCMATRIX-formatted input through the parser and validates the resulting model via has_quadratic_constraints() and get_quadratic_constraint_matrices() to ensure end-to-end behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/libmps_parser/src/mps_data_model.cpp`:
- Around line 223-259: Validate that constraint_row_index is non-negative and
that CSR offsets are consistent before constructing
quadratic_constraint_matrix_t: in append_quadratic_constraint_matrix check
constraint_row_index >= 0 (reject negative with mps_parser_expects and an
appropriate ValidationError), and verify CSR consistency by asserting
Qc_offsets[size_offsets-1] == size_values (and optionally that offsets are
non-decreasing and within [0,size_values]); if any check fails, return a
ValidationError. Mirror the same style and error messages used in
set_quadratic_objective_matrix and push_back into quadratic_constraint_matrices_
only after all validations pass.
---
Nitpick comments:
In `@cpp/libmps_parser/src/mps_parser.cpp`:
- Around line 1328-1347: The current flush_qcmatrix_block implementation does
O(n) scans over qcmatrix_blocks_ to detect duplicate qcmatrix_active_row_id_,
causing O(n^2) behavior; change to use an unordered_set to track seen constraint
row ids: add a member std::unordered_set<i_t> qcmatrix_seen_rows_, on flush
check if qcmatrix_active_row_id_ is already in qcmatrix_seen_rows_ (and throw
the same mps_parser_expects error if found), otherwise insert the id and append
the block to qcmatrix_blocks_ as before, then clear qcmatrix_current_entries_
and reset qcmatrix_active_row_id_. Ensure the set is updated exactly where
blocks are added so duplicate detection becomes O(1) per block.
In `@cpp/libmps_parser/tests/mps_parser_test.cpp`:
- Around line 862-901: Add edge-case tests for the quadratic-constraint API: (1)
an "empty entries" test that calls append_quadratic_constraint_matrix with empty
values/indices and offsets like {0,0} and then asserts
has_quadratic_constraints() is true and get_quadratic_constraint_matrices()
contains a matrix with empty values/indices; (2) a "duplicate constraint row"
test that attempts to append two matrices with the same constraint_row_index and
asserts the parser/model rejects or flags the duplicate consistent with
flush_qcmatrix_block behavior; and (3) a lightweight parser integration test
that feeds a small QCMATRIX-formatted input through the parser and validates the
resulting model via has_quadratic_constraints() and
get_quadratic_constraint_matrices() to ensure end-to-end behavior.
🪄 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: Pro Plus
Run ID: 5d6dc11a-8039-473b-89c2-20ca25ccfc28
📒 Files selected for processing (6)
cpp/libmps_parser/include/mps_parser/mps_data_model.hppcpp/libmps_parser/include/mps_parser/parser.hppcpp/libmps_parser/src/mps_data_model.cppcpp/libmps_parser/src/mps_parser.cppcpp/libmps_parser/src/mps_parser.hppcpp/libmps_parser/tests/mps_parser_test.cpp
yuwenchen95
changed the title
yuwenchen95
changed the title
akifcorduk
added
non-breaking
Contributor
|
/ok to test a114099 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cpp/libmps_parser/src/mps_parser.cpp (2)
1336-1341: Consider using a set for O(1) duplicate detection.The linear scan for duplicate constraint row IDs results in O(n²) complexity when processing many QCMATRIX blocks. While unlikely to be a bottleneck in practice, using a
std::unordered_setfor already-seen row IDs would provide O(1) lookup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/libmps_parser/src/mps_parser.cpp` around lines 1336 - 1341, Replace the O(n²) duplicate check over qcmatrix_blocks_ with an O(1) lookup using a std::unordered_set: create a seen set (e.g., std::unordered_set<size_t> seen_row_ids), iterate qcmatrix_blocks_, and for each b check if b.constraint_row_id is already in seen_row_ids; if it is, call mps_parser_expects(..., error_type_t::ValidationError, ...) using qcmatrix_active_row_id_ (or b.constraint_row_id) as before, otherwise insert b.constraint_row_id into seen_row_ids; update the check site that currently compares every element to qcmatrix_active_row_id_ to use the set membership test instead.
1342-1346: Clearqcmatrix_current_entries_after move for defensive coding.After
std::move(qcmatrix_current_entries_), the vector is in a valid but unspecified state. While subsequentemplace_backcalls will work, explicitly clearing it makes the intent clear and prevents potential issues if the implementation changes.🔧 Proposed fix
block.entries = std::move(qcmatrix_current_entries_); + qcmatrix_current_entries_.clear(); qcmatrix_blocks_.push_back(std::move(block)); qcmatrix_active_row_id_ = -1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/libmps_parser/src/mps_parser.cpp` around lines 1342 - 1346, After moving qcmatrix_current_entries_ into the qcmatrix_raw_block_t and pushing it into qcmatrix_blocks_, explicitly reset the source vector by calling qcmatrix_current_entries_.clear() (or clear + shrink_to_fit if desired) so the vector is in a known empty state before further use; apply this change around the block creation code that sets block.constraint_row_id = qcmatrix_active_row_id_, block.entries = std::move(qcmatrix_current_entries_), qcmatrix_blocks_.push_back(std::move(block)), and qcmatrix_active_row_id_ = -1.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/libmps_parser/src/mps_parser.cpp`:
- Around line 1408-1412: In the free-format branch where you parse into
var1_name, var2_name and value, guard against failed numeric extraction:
initialize value and verify that (ss >> value) succeeds; if it fails, call
mps_parser_no_except (consistent with the fixed-format path using
get_numerical_bound) or otherwise report the parse error and return/skip
processing to avoid using an uninitialized value; update the block that
references var1_name, var2_name and value to perform this check before
continuing.
---
Nitpick comments:
In `@cpp/libmps_parser/src/mps_parser.cpp`:
- Around line 1336-1341: Replace the O(n²) duplicate check over qcmatrix_blocks_
with an O(1) lookup using a std::unordered_set: create a seen set (e.g.,
std::unordered_set<size_t> seen_row_ids), iterate qcmatrix_blocks_, and for each
b check if b.constraint_row_id is already in seen_row_ids; if it is, call
mps_parser_expects(..., error_type_t::ValidationError, ...) using
qcmatrix_active_row_id_ (or b.constraint_row_id) as before, otherwise insert
b.constraint_row_id into seen_row_ids; update the check site that currently
compares every element to qcmatrix_active_row_id_ to use the set membership test
instead.
- Around line 1342-1346: After moving qcmatrix_current_entries_ into the
qcmatrix_raw_block_t and pushing it into qcmatrix_blocks_, explicitly reset the
source vector by calling qcmatrix_current_entries_.clear() (or clear +
shrink_to_fit if desired) so the vector is in a known empty state before further
use; apply this change around the block creation code that sets
block.constraint_row_id = qcmatrix_active_row_id_, block.entries =
std::move(qcmatrix_current_entries_),
qcmatrix_blocks_.push_back(std::move(block)), and qcmatrix_active_row_id_ = -1.
🪄 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: Pro Plus
Run ID: 0b68e278-9861-44ca-8945-9e624b0213b8
📒 Files selected for processing (6)
cpp/libmps_parser/include/mps_parser/mps_data_model.hppcpp/libmps_parser/include/mps_parser/parser.hppcpp/libmps_parser/src/mps_data_model.cppcpp/libmps_parser/src/mps_parser.cppcpp/libmps_parser/src/mps_parser.hppcpp/libmps_parser/tests/mps_parser_test.cpp
✅ Files skipped from review due to trivial changes (2)
- cpp/libmps_parser/include/mps_parser/parser.hpp
- cpp/libmps_parser/tests/mps_parser_test.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- cpp/libmps_parser/src/mps_parser.hpp
- cpp/libmps_parser/src/mps_data_model.cpp
- cpp/libmps_parser/include/mps_parser/mps_data_model.hpp
Contributor
|
@yuwenchen95 do these changes include the mps writer as well? |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cpp/libmps_parser/src/mps_data_model.cpp (1)
223-262:⚠️ Potential issue | 🟠 MajorEnforce CSR structural invariants before storing QCMATRIX.
The function still accepts structurally invalid CSR (e.g., mismatched values/indices count, non-monotonic offsets, terminal offset not equal to nnz). This can break downstream constraint handling.
Proposed fix
mps_parser_expects( Qc_offsets != nullptr, error_type_t::ValidationError, "Qc_offsets cannot be null"); mps_parser_expects( size_offsets > 0, error_type_t::ValidationError, "size_offsets cannot be empty"); + mps_parser_expects( + size_values == size_indices, + error_type_t::ValidationError, + "size_values and size_indices must match for CSR"); + mps_parser_expects( + Qc_offsets[0] == 0, + error_type_t::ValidationError, + "Qc_offsets must start at 0"); + for (i_t k = 1; k < size_offsets; ++k) { + mps_parser_expects( + Qc_offsets[k] >= Qc_offsets[k - 1], + error_type_t::ValidationError, + "Qc_offsets must be non-decreasing"); + } + mps_parser_expects( + Qc_offsets[size_offsets - 1] == size_values, + error_type_t::ValidationError, + "last Qc_offsets entry must equal size_values");As per coding guidelines, "Validate algorithm correctness in optimization logic: ... constraint/objective handling must produce correct results."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/libmps_parser/src/mps_data_model.cpp` around lines 223 - 262, append_quadratic_constraint_matrix currently accepts CSR inputs without validating CSR invariants; update it to validate that size_values == size_indices (or both zero), that size_offsets > 0, that offsets are non-decreasing, that offsets.front() == 0 and offsets.back() == size_values (nnz), and that every offset is in [0, size_values]; perform these checks in append_quadratic_constraint_matrix using mps_parser_expects before constructing quadratic_constraint_matrix_t and push_back into quadratic_constraint_matrices_, so invalid Qc_values/Qc_indices/Qc_offsets are rejected early.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/libmps_parser/src/mps_data_model.cpp`:
- Around line 223-262: In append_quadratic_constraint_matrix, validate that
size_values and size_indices are non-negative before calling std::vector::resize
and before using Qc_values/Qc_indices: add mps_parser_expects(size_values >= 0,
...) and mps_parser_expects(size_indices >= 0, ...) (similar to the existing
checks for constraint_row_index and size_offsets), then only call
qcm.values.resize(size_values) and std::copy(Qc_values, Qc_values + size_values,
...) when size_values > 0, and likewise only resize/copy indices when
size_indices > 0; this prevents signed-to-unsigned conversion into huge sizes
and avoids resource exhaustion in append_quadratic_constraint_matrix.
---
Duplicate comments:
In `@cpp/libmps_parser/src/mps_data_model.cpp`:
- Around line 223-262: append_quadratic_constraint_matrix currently accepts CSR
inputs without validating CSR invariants; update it to validate that size_values
== size_indices (or both zero), that size_offsets > 0, that offsets are
non-decreasing, that offsets.front() == 0 and offsets.back() == size_values
(nnz), and that every offset is in [0, size_values]; perform these checks in
append_quadratic_constraint_matrix using mps_parser_expects before constructing
quadratic_constraint_matrix_t and push_back into quadratic_constraint_matrices_,
so invalid Qc_values/Qc_indices/Qc_offsets are rejected early.
🪄 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: Pro Plus
Run ID: e13f07ef-d48d-486d-bd6d-10fb0d9fc8e7
📒 Files selected for processing (2)
cpp/libmps_parser/src/mps_data_model.cppcpp/libmps_parser/src/mps_parser.cpp
✅ Files skipped from review due to trivial changes (1)
- cpp/libmps_parser/src/mps_parser.cpp
Comment on lines
+223
to
+262
| template <typename i_t, typename f_t> | ||
| void mps_data_model_t<i_t, f_t>::append_quadratic_constraint_matrix(i_t constraint_row_index, | ||
| const f_t* Qc_values, | ||
| i_t size_values, | ||
| const i_t* Qc_indices, | ||
| i_t size_indices, | ||
| const i_t* Qc_offsets, | ||
| i_t size_offsets) | ||
| { | ||
| mps_parser_expects( | ||
| constraint_row_index >= 0, error_type_t::ValidationError, "constraint_row_index must be non-negative"); | ||
|
|
||
| if (size_values != 0) { | ||
| mps_parser_expects( | ||
| Qc_values != nullptr, error_type_t::ValidationError, "Qc_values cannot be null"); | ||
| } | ||
| if (size_indices != 0) { | ||
| mps_parser_expects( | ||
| Qc_indices != nullptr, error_type_t::ValidationError, "Qc_indices cannot be null"); | ||
| } | ||
| mps_parser_expects( | ||
| Qc_offsets != nullptr, error_type_t::ValidationError, "Qc_offsets cannot be null"); | ||
| mps_parser_expects( | ||
| size_offsets > 0, error_type_t::ValidationError, "size_offsets cannot be empty"); | ||
|
|
||
| quadratic_constraint_matrix_t qcm; | ||
| qcm.constraint_row_index = constraint_row_index; | ||
| qcm.values.resize(size_values); | ||
| if (size_values > 0) { | ||
| std::copy(Qc_values, Qc_values + size_values, qcm.values.data()); | ||
| } | ||
| qcm.indices.resize(size_indices); | ||
| if (size_indices > 0) { | ||
| std::copy(Qc_indices, Qc_indices + size_indices, qcm.indices.data()); | ||
| } | ||
| qcm.offsets.resize(size_offsets); | ||
| std::copy(Qc_offsets, Qc_offsets + size_offsets, qcm.offsets.data()); | ||
|
|
||
| quadratic_constraint_matrices_.push_back(std::move(qcm)); | ||
| } |
There was a problem hiding this comment.
Validate non-negative CSR sizes before resize.
size_values and size_indices are signed (i_t). If either is negative, std::vector::resize can convert to a huge unsigned size and trigger resource exhaustion.
Proposed fix
void mps_data_model_t<i_t, f_t>::append_quadratic_constraint_matrix(i_t constraint_row_index,
const f_t* Qc_values,
i_t size_values,
const i_t* Qc_indices,
i_t size_indices,
const i_t* Qc_offsets,
i_t size_offsets)
{
mps_parser_expects(
constraint_row_index >= 0, error_type_t::ValidationError, "constraint_row_index must be non-negative");
+ mps_parser_expects(
+ size_values >= 0, error_type_t::ValidationError, "size_values cannot be negative");
+ mps_parser_expects(
+ size_indices >= 0, error_type_t::ValidationError, "size_indices cannot be negative");
+ mps_parser_expects(
+ size_offsets > 0, error_type_t::ValidationError, "size_offsets cannot be empty");
if (size_values != 0) {
mps_parser_expects(
Qc_values != nullptr, error_type_t::ValidationError, "Qc_values cannot be null");
}
if (size_indices != 0) {
mps_parser_expects(
Qc_indices != nullptr, error_type_t::ValidationError, "Qc_indices cannot be null");
}
mps_parser_expects(
Qc_offsets != nullptr, error_type_t::ValidationError, "Qc_offsets cannot be null");
- mps_parser_expects(
- size_offsets > 0, error_type_t::ValidationError, "size_offsets cannot be empty");As per coding guidelines, "Validate input sanitization to prevent buffer overflows and resource exhaustion attacks; avoid unsafe deserialization of problem files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/libmps_parser/src/mps_data_model.cpp` around lines 223 - 262, In
append_quadratic_constraint_matrix, validate that size_values and size_indices
are non-negative before calling std::vector::resize and before using
Qc_values/Qc_indices: add mps_parser_expects(size_values >= 0, ...) and
mps_parser_expects(size_indices >= 0, ...) (similar to the existing checks for
constraint_row_index and size_offsets), then only call
qcm.values.resize(size_values) and std::copy(Qc_values, Qc_values + size_values,
...) when size_values > 0, and likewise only resize/copy indices when
size_indices > 0; this prevents signed-to-unsigned conversion into huge sizes
and avoids resource exhaustion in append_quadratic_constraint_matrix.
Author
It's only relevant to the change for the constructor of |
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
3 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
This change adds support for reading QCMATRIX blocks from MPS/QPS-style input and representing each quadratic constraint matrix in CSR form on
mps_data_model_t.Included
QCMATRIX(fixed and free format), validate row/variable names, and reject duplicate blocks for the same constraint row.append_quadratic_constraint_matrix/get_quadratic_constraint_matrices, plushas_quadratic_constraints().QCMATRIXuses symmetric CSR without the extra ½ factor).qps_parser.qcmatrix_append_apifor the model append API.Not included (follow-up work)
Issue
N/A — incremental parser/model support; follow-up PR(s) will wire algorithms.
Checklist