✨ Add Euler decomposition and supporting decomposition primitives#1672
Open
simon1hofmann wants to merge 7 commits intomainfrom
Open
✨ Add Euler decomposition and supporting decomposition primitives#1672simon1hofmann wants to merge 7 commits intomainfrom
simon1hofmann wants to merge 7 commits intomainfrom
Conversation
simon1hofmann
added
feature
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a new Euler decomposition subsystem to the MLIR QCO dialect: gate/sequence abstractions, Euler-basis enums and mapping, algorithms to factor 2×2 unitaries into basis-specific gate sequences with exact global-phase accounting, matrix utilities, helpers, and extensive unit tests. Changes
Sequence DiagramsequenceDiagram
participant User
participant EulerDecomposition
participant AngleExtraction
participant GateEmitter
participant GateSequence
User->>EulerDecomposition: generateCircuit(basis, unitary, simplify, atol)
activate EulerDecomposition
EulerDecomposition->>AngleExtraction: anglesFromUnitary(unitary, basis)
activate AngleExtraction
AngleExtraction->>AngleExtraction: paramsZyz/Zxz/Xyx/Xzx/U1x(unitary)
AngleExtraction-->>EulerDecomposition: (theta, phi, lambda, phase)
deactivate AngleExtraction
alt Basis is Z-Y-Z / Z-X-Z / X-Y-X / X-Z-X
EulerDecomposition->>GateEmitter: decomposeKAK(angles, kGate, aGate, simplify, atol)
else Basis is ZSX / ZSXX
EulerDecomposition->>GateEmitter: decomposePsxGen(angles, allowXShortcut, simplify, atol)
else Basis is U / U321 / U3
EulerDecomposition->>GateEmitter: emit single U gate
end
activate GateEmitter
GateEmitter->>GateSequence: add Gate(type, params, qubits)
GateEmitter->>GateSequence: set globalPhase
GateEmitter-->>EulerDecomposition: OneQubitGateSequence
deactivate GateEmitter
EulerDecomposition-->>User: OneQubitGateSequence
deactivate EulerDecomposition
User->>GateSequence: getUnitaryMatrix()
activate GateSequence
GateSequence->>GateSequence: multiply gate matrices in order
GateSequence->>GateSequence: apply globalPhaseFactor
GateSequence-->>User: Eigen::Matrix4cd
deactivate GateSequence
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 51 minutes and 29 seconds.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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/include/mlir/Dialect/QCO/Transforms/Decomposition/Helpers.h`:
- Around line 28-32: isUnitaryMatrix currently only checks
(matrix.transpose().conjugate() * matrix).isIdentity(...) which allows
rectangular matrices with orthonormal columns to pass; modify isUnitaryMatrix to
first confirm the matrix is square (matrix.rows() == matrix.cols()) and return
false if not, then perform the unitary check (using adjoint if preferred) so
only true square unitary matrices are accepted; reference isUnitaryMatrix and
the matrix.transpose().conjugate() * matrix identity check to locate where to
add the square check.
In `@mlir/lib/Dialect/QCO/Transforms/Decomposition/EulerDecomposition.cpp`:
- Around line 79-87: The ZYZ branch currently returns the raw phase from
paramsZyz(matrix) which exposes γ; to preserve the existing
anglesFromUnitary(..., EulerBasis::ZYZ) contract you must absorb the qco.u
intrinsic offset into the returned phase: call paramsZyz(matrix) to get the
decomposition, then adjust its phase by adding (phi + lambda) / 2 (i.e., phase
+= (phi + lambda)/2) before returning; reference EulerBasis::ZYZ, paramsZyz, and
anglesFromUnitary to locate and apply this change.
In `@mlir/lib/Dialect/QCO/Transforms/Decomposition/Helpers.cpp`:
- Around line 48-57: The getComplexity function incorrectly applies the
multi-qubit cost before checking for decomposition::GateKind::GPhase, causing
getComplexity(GPhase, n>1) to return a positive cost; change the logic in
getComplexity so that GateKind::GPhase is handled first (or otherwise
short-circuited) and always returns 0 regardless of numOfQubits—update the
function around the multi-qubit branch to check for
decomposition::GateKind::GPhase before applying the multiQubitFactor or add an
explicit early return for GPhase.
In
`@mlir/unittests/Dialect/QCO/Transforms/Decomposition/test_decomposition_helpers.cpp`:
- Around line 35-42: Add a regression assertion to the existing
TEST(DecompositionHelpersTest, GetComplexitySingleQubitAndGphase) that verifies
getComplexity(GateKind::GPhase, N) returns 0 for multi-qubit inputs (e.g., call
getComplexity(GateKind::GPhase, 2) and assert EXPECT_EQ(..., 0U)); this ensures
the zero-cost invariant for GateKind::GPhase holds when numOfQubits > 1.
In
`@mlir/unittests/Dialect/QCO/Transforms/Decomposition/test_euler_decomposition.cpp`:
- Around line 118-123: The test currently reconstructs only
u3Matrix(angles[0],angles[1],angles[2]) and uses isEquivalentUpToGlobalPhase,
which ignores angles[3]; update the test to include the extracted phase from
EulerDecomposition::anglesFromUnitary (angles[3]) by multiplying u3Matrix(...)
by exp(i*angles[3]) and assert that this phased reconstruction equals the
original hadamard (using a direct matrix equality/tolerance check rather than
isEquivalentUpToGlobalPhase) so the returned phase is validated for the
SingleQubitMode::U3 contract.
🪄 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: 60ac46cf-2573-4949-b882-448254fc15d0
📒 Files selected for processing (18)
mlir/include/mlir/Dialect/QCO/Transforms/Decomposition/EulerBasis.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/EulerDecomposition.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/Gate.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/GateKind.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/GateSequence.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/Helpers.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/UnitaryMatrices.hmlir/lib/Dialect/QCO/Transforms/Decomposition/EulerBasis.cppmlir/lib/Dialect/QCO/Transforms/Decomposition/EulerDecomposition.cppmlir/lib/Dialect/QCO/Transforms/Decomposition/GateSequence.cppmlir/lib/Dialect/QCO/Transforms/Decomposition/Helpers.cppmlir/lib/Dialect/QCO/Transforms/Decomposition/UnitaryMatrices.cppmlir/unittests/Dialect/QCO/Transforms/CMakeLists.txtmlir/unittests/Dialect/QCO/Transforms/Decomposition/CMakeLists.txtmlir/unittests/Dialect/QCO/Transforms/Decomposition/decomposition_test_utils.hmlir/unittests/Dialect/QCO/Transforms/Decomposition/test_decomposition_helpers.cppmlir/unittests/Dialect/QCO/Transforms/Decomposition/test_euler_decomposition.cppmlir/unittests/TestCaseUtils.h
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Co-authored-by: Tamino Bauknecht <dev@tb6.eu>
simon1hofmann
force-pushed
the
decomp/euler
branch
from
5b752b4 to
e5280c5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@mlir/unittests/Dialect/QCO/Transforms/Decomposition/decomposition_test_utils.h`:
- Around line 24-49: randomUnitaryMatrix currently only enforces fixed-size
matrices but can accept non-square fixed-size types; add a compile-time check to
require square matrices by adding a static_assert that
MatrixType::RowsAtCompileTime == MatrixType::ColsAtCompileTime (with a clear
message like "randomUnitaryMatrix requires square matrices") near the existing
fixed-size static_assert in the randomUnitaryMatrix template so misuse with
rectangular fixed-size MatrixType is prevented; keep the existing use of
MatrixType and dim as-is.
In
`@mlir/unittests/Dialect/QCO/Transforms/Decomposition/test_euler_decomposition.cpp`:
- Around line 173-180: The test for ZSXX collapsing to a bare X should assert
there are no other gate kinds or extra gates: after calling
EulerDecomposition::generateCircuit(EulerBasis::ZSXX, pauliX, true,
std::nullopt) and the existing EXPECT_EQ(countGatesOfType(seq, GateKind::X), 1U)
and sequenceMatchesSingleQubitMatrix check, also assert the total gate count
equals 1 (e.g., using countTotalGates(seq) or summing known GateKind counts)
and/or assert that the set of gate kinds present is exactly {GateKind::X} (e.g.,
ensure countGatesOfType(seq, GateKind::RZ)==0 and similarly zero for SX, H,
etc.) so no RZ/SX/other gates accompany the X; reference the TEST name
EulerDecompositionTest::ZsxxPauliXUsesSingleXGate,
EulerDecomposition::generateCircuit, countGatesOfType, GateKind, and
sequenceMatchesSingleQubitMatrix when adding these assertions.
🪄 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: cce6649a-1227-417d-8c76-5eead11f9fca
📒 Files selected for processing (5)
mlir/include/mlir/Dialect/QCO/Transforms/Decomposition/Helpers.hmlir/lib/Dialect/QCO/Transforms/Decomposition/Helpers.cppmlir/unittests/Dialect/QCO/Transforms/Decomposition/decomposition_test_utils.hmlir/unittests/Dialect/QCO/Transforms/Decomposition/test_decomposition_helpers.cppmlir/unittests/Dialect/QCO/Transforms/Decomposition/test_euler_decomposition.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlir/lib/Dialect/QCO/Transforms/Decomposition/UnitaryMatrices.cpp`:
- Around line 163-165: The current code silently returns
Eigen::Matrix4cd::Identity() when gate.qubitId.empty(), which masks malformed
input; instead, validate qubitId explicitly and fail fast: if gate.qubitId is
empty and gate.kind is not the explicit identity kind, emit a clear error or
assertion (e.g., llvm::report_fatal_error or assert) rather than returning an
identity matrix; only allow returning Eigen::Matrix4cd::Identity() when
gate.kind is the identity gate and qubitId legitimately indicates an identity
operation. Locate the branch using gate.qubitId and Eigen::Matrix4cd::Identity()
and replace the unconditional identity fallback with the described input check
and failure.
In
`@mlir/unittests/Dialect/QCO/Transforms/Decomposition/test_euler_decomposition.cpp`:
- Around line 90-110: The test TEST(EulerDecompositionTest, Random) uses
maxIterations = 10000 which is too slow for regular unit tests; change this test
to use a much smaller deterministic iteration count (e.g., 100) by reducing
maxIterations and keeping the same rng seed and logic, and move the heavy loop
into a new separate stress test (e.g., TEST(EulerDecompositionStress,
RandomStress)) that runs 10000 iterations; ensure the new stress test uses the
same calls (randomUnitaryMatrix, EulerDecomposition::generateCircuit,
EulerDecompositionTest::restore) and preserves the existing EXPECT_TRUE/isApprox
checks so CI unit tests stay fast while stress runs still validate the full
randomized workload.
🪄 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: f3dacb7d-3453-4bde-8bba-2f70a91add21
📒 Files selected for processing (17)
mlir/include/mlir/Dialect/QCO/Transforms/Decomposition/EulerBasis.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/EulerDecomposition.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/Gate.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/GateKind.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/GateSequence.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/Helpers.hmlir/include/mlir/Dialect/QCO/Transforms/Decomposition/UnitaryMatrices.hmlir/lib/Dialect/QCO/Transforms/Decomposition/EulerBasis.cppmlir/lib/Dialect/QCO/Transforms/Decomposition/EulerDecomposition.cppmlir/lib/Dialect/QCO/Transforms/Decomposition/GateSequence.cppmlir/lib/Dialect/QCO/Transforms/Decomposition/Helpers.cppmlir/lib/Dialect/QCO/Transforms/Decomposition/UnitaryMatrices.cppmlir/unittests/Dialect/QCO/Transforms/CMakeLists.txtmlir/unittests/Dialect/QCO/Transforms/Decomposition/CMakeLists.txtmlir/unittests/Dialect/QCO/Transforms/Decomposition/decomposition_test_utils.hmlir/unittests/Dialect/QCO/Transforms/Decomposition/test_decomposition_helpers.cppmlir/unittests/Dialect/QCO/Transforms/Decomposition/test_euler_decomposition.cpp
Co-authored-by: Copilot <copilot@github.com>
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
Split up from #1665 implementing Euler decomposition.
Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.