✨ Add complex eigendecomposition for QCO matrices#1814
Open
simon1hofmann wants to merge 11 commits into
Open
Conversation
…add new tests for dynamic matrix eigenvalue computations
…s; add corresponding tests for new functionality
simon1hofmann
changed the title
simon1hofmann
added
enhancement
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds eigendecomposition support for QCO matrices. It introduces public complex and symmetric eigen APIs, implements fixed-size and dynamic solvers, routes matrix methods to those helpers, updates build wiring, and expands unit tests. ChangesQCO eigendecomposition support
Sequence Diagram(s)sequenceDiagram
participant DynamicMatrix
participant Matrix2x2
participant Matrix4x4
participant Eigensolver
alt dim == 1
DynamicMatrix->>Eigensolver: decomposeComplexEigen1x1(*this)
Eigensolver-->>DynamicMatrix: std::optional<ComplexEigen>
else dim == 2
DynamicMatrix->>Matrix2x2: assignFrom(*this)
Matrix2x2->>Eigensolver: decomposeComplexEigen2(matrix)
Eigensolver-->>Matrix2x2: std::optional<ComplexEigen2>
Matrix2x2->>Eigensolver: fromComplexEigen(eigen2)
Eigensolver-->>DynamicMatrix: ComplexEigen
else dim == 4
DynamicMatrix->>Matrix4x4: assignFrom(*this)
Matrix4x4->>Eigensolver: decomposeComplexEigen4(matrix)
Eigensolver-->>Matrix4x4: std::optional<ComplexEigen4>
Matrix4x4->>Eigensolver: fromComplexEigen(eigen4)
Eigensolver-->>DynamicMatrix: ComplexEigen
else other square size
DynamicMatrix->>Eigensolver: decomposeComplexEigenDynamic(*this)
Eigensolver-->>DynamicMatrix: std::optional<ComplexEigen>
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@mlir/lib/Dialect/QCO/Utils/CMakeLists.txt`:
- Around line 20-24: The public header set for MLIRQCOMatrix is missing
Eigensolver.h even though Eigensolver.cpp now belongs to that target, which
leaves the declarations owned by a different target. Update the target_sources
entry for MLIRQCOMatrix to export Eigensolver.h alongside Matrix.h, and adjust
the later MLIRQCOUtils header glob/filter so Eigensolver.h is excluded there and
not duplicated under the wrong target. Use the existing target_sources,
MLIRQCOMatrix, and MLIRQCOUtils sections in this CMakeLists.txt to make the
ownership consistent.
In `@mlir/lib/Dialect/QCO/Utils/Eigensolver.cpp`:
- Around line 187-189: The QL non-convergence path in symmetricTql2_4 only uses
an assert, so non-asserting builds can keep looping past the convergence limit.
Update this branch to terminate deterministically by returning a convergence
failure status or otherwise failing out immediately from symmetricTql2_4 when j
reaches the limit, and make sure the caller of this eigensolver path handles
that failure instead of continuing with invalid eigenpairs.
- Around line 697-702: Guard the exceptional shift in
Eigensolver::computeQrIteration so the 10/20-iteration branch does not read
hessenbergReal at activeEigenIndex - 2 when the active QR window has collapsed
to a 2x2 block. Add a condition around the shift calculation using
activeEigenIndex, activeEigenIndexMinus1, and rowLow so the second term is only
accessed when the column index is non-negative, and otherwise skip or adjust the
exceptional shift path to avoid the out-of-bounds access.
- Around line 1097-1104: The 2x2 diagonal fast-path in Eigensolver.cpp only
returns one basis vector twice when both eigenvalues match, which makes the
eigenvector matrix singular. Update the branch that handles std::abs(b) <=
MATRIX_TOLERANCE and std::abs(c) <= MATRIX_TOLERANCE so it returns a full
orthonormal basis for the diagonal case: keep the current behavior when lambda
matches a, but when a == d and both eigenvalues are equal, choose distinct basis
vectors for the two columns instead of always using [1, 0]. Make sure the logic
in the eigenvector construction path produces linearly independent vectors for
both eigenvalues.
In `@mlir/lib/Dialect/QCO/Utils/Matrix.cpp`:
- Around line 578-579: The invalid two-qubit reorder path in the
Matrix::reorderForQubits logic is currently only enforced by an assert, so it
becomes a silent fallthrough in release builds. Update the invalid-index
handling to use a fatal non-returning invariant failure, consistent with the
neighboring invalid-index branch, so public calls like reorderForQubits(0, 0)
cannot continue past the check. Use the existing Matrix::reorderForQubits and
related invalid-index handling as the anchor points when making the change.
In `@mlir/unittests/Dialect/QCO/Utils/test_unitary_matrix.cpp`:
- Around line 67-70: The eigenvalue checks in the unitary matrix tests are
comparing only magnitudes, which can hide conjugated or phase-shifted results.
Update the assertions in the fixed-vs-dynamic comparison loops to compare the
complex eigenvalues directly in the relevant test cases, using the existing
fixed and dynamic solver outputs from the test helper instead of std::abs on
fixed->eigenvalues and dynamic->eigenvalues. This should be applied in both the
2x2 and 4x4 paths so the dynamic solver is verified against the lifted fixed
result itself.
- Around line 161-184: The eigen decomposition helpers currently validate
residuals without first confirming the solver returned a full square
decomposition. Update expectComplexEigen and expectGeneralComplexEigen to assert
that ComplexEigen contains exactly matrix.rows() eigenvalues and that the
eigenvector/eigenpair matrix has the expected matching shape before checking
residuals or traces, using the existing complexEigen(), eigenvalues, and
eigenpairsSatisfy/maxEigenpairResidual helpers.
- Line 929: The deterministic seed initialization in test_unitary_matrix.cpp
should match the existing lint suppression pattern used elsewhere in this test
file. Update the std::mt19937 rng initialization in the unitary matrix test
helper to include the same NOLINT(cert-msc51-cpp) suppression used by the other
deterministic seeds (for example, the ones in the nearby test cases) so
clang-tidy stays consistent across these test utilities.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2937e65c-76ad-4598-8d4a-28f604ff67ca
📒 Files selected for processing (6)
mlir/include/mlir/Dialect/QCO/Utils/Eigensolver.hmlir/include/mlir/Dialect/QCO/Utils/Matrix.hmlir/lib/Dialect/QCO/Utils/CMakeLists.txtmlir/lib/Dialect/QCO/Utils/Eigensolver.cppmlir/lib/Dialect/QCO/Utils/Matrix.cppmlir/unittests/Dialect/QCO/Utils/test_unitary_matrix.cpp
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
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
1 participant
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
Adds self-contained eigendecomposition for QCO matrix types without an Eigen dependency, as groundwork for unitary matrix powers (
U^α = V diag(λ^α) V†).Fixes #1811
Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.