fix Qc test and added canonicalization to allow CI tests to pass - #1594
fix Qc test and added canonicalization to allow CI tests to pass#1594Bubullzz wants to merge 7 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe fast MPS parser now validates and canonicalizes QCMATRIX COO data before storing quadratic constraints. It adds ChangesQCMATRIX canonicalization
PDLP header dependency
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/io/experimental_mps_fast/fast_parser.cpp (1)
2773-2783: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve valid small QCMATRIX coefficients.
canonicalize_coo_matrixdrops every merged value withabs(value) <= epsilon, not only exact cancellations. A legitimate coefficient such as1e-20can therefore disappear and change the optimization model. Use an exact-zero or scale-aware cancellation policy, and add a regression case for a small nonzero coefficient. As per path instructions, QCMATRIX canonicalization is correctness-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/io/experimental_mps_fast/fast_parser.cpp` around lines 2773 - 2783, Update canonicalize_coo_matrix in the QCMATRIX path to avoid dropping legitimate small coefficients: remove only exact cancellations or apply a scale-aware tolerance that preserves nonzero values such as 1e-20. Keep duplicate merging and upper-triangular canonicalization intact, and add a regression case covering a small nonzero QCMATRIX coefficient.Source: Path instructions
🧹 Nitpick comments (1)
cpp/src/io/experimental_mps_fast/fast_parser.cpp (1)
2773-2783: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid sorting QCMATRIX entries twice.
The
permsort and allocation before this call are redundant becausecanonicalize_coo_matrixperforms its own bucketing and per-row sort. Feed the entries directly intoqcor extend the canonicalizer to accept the tuple representation, removing the extraO(n log n)pass in finalization. As per path instructions, avoid extra passes and allocations in hot finalization paths.🤖 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/experimental_mps_fast/fast_parser.cpp` around lines 2773 - 2783, The QCMATRIX finalization currently sorts entries through perm before canonicalize_coo_matrix sorts them again. Remove the redundant perm-based ordering and intermediate reserve/copy path, feeding block.entries directly into qc or extending canonicalize_coo_matrix to consume the tuple representation while preserving canonical upper-triangular COO output and avoiding extra allocations and O(n log n) work.Source: Path instructions
🤖 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/io/experimental_mps_fast/fast_parser.cpp`:
- Around line 2773-2783: Update canonicalize_coo_matrix in the QCMATRIX path to
avoid dropping legitimate small coefficients: remove only exact cancellations or
apply a scale-aware tolerance that preserves nonzero values such as 1e-20. Keep
duplicate merging and upper-triangular canonicalization intact, and add a
regression case covering a small nonzero QCMATRIX coefficient.
---
Nitpick comments:
In `@cpp/src/io/experimental_mps_fast/fast_parser.cpp`:
- Around line 2773-2783: The QCMATRIX finalization currently sorts entries
through perm before canonicalize_coo_matrix sorts them again. Remove the
redundant perm-based ordering and intermediate reserve/copy path, feeding
block.entries directly into qc or extending canonicalize_coo_matrix to consume
the tuple representation while preserving canonical upper-triangular COO output
and avoiding extra allocations and O(n log n) work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 600971e8-a04c-4b87-85df-f1fffbdf036b
📒 Files selected for processing (3)
cpp/src/io/experimental_mps_fast/fast_parser.cppcpp/src/io/mps_parser.cppcpp/tests/linear_programming/experimental_mps_fast/fast_parser_edge_test.cpp
|
/ok to test a67fad3 |
|
/ok to test 897712a |
CI Test Summary✅ All 0 test job(s) passed. |
|
Thanks a lot! |
| qc.cols.push_back(col); | ||
| qc.vals.push_back(val); | ||
| } | ||
| canonicalize_coo_matrix(qc.rows, qc.cols, qc.vals); |
There was a problem hiding this comment.
I just realized we should have symmetric check check_symmetric_offdiagonal_pairs(qc.rows, qc.cols, qc.vals); just before canonicalize_coo_matrix. MPS file assumes symmetric input for off-diagonals in a quadratic constraint. The check_symmetric_offdiagonal_pairs is a new function I introduced after the PR is created, so it is missing in the current multithreaded MPS processing for quadratic constraints.
|
/ok to test 6b4739a |
|
/ok to test b87f701 |
|
/ok to test 32ff68a |
|
/ok to test d6791ba |
|
already merged |
4 participants
Description
Issue
Checklist