Decomposition #920
Open
Decomposition #920
Conversation
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
🧪 CI InsightsHere's what we observed from your CI run for 9134d22. 🟢 All jobs passed!But CI Insights is watching 👀 |
ystade
added
feature
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a RoutingAwareNativeGateCompiler and a new quaternion-based NativeGateDecomposer, changes DecomposerBase/NoOpDecomposer.decompose to accept nQubits (non-const), and adds unit and integration tests for the decomposer and compiler. (33 words) Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Compiler as RoutingAwareNativeGateCompiler
participant Decomposer as NativeGateDecomposer
participant Scheduler as ASAPScheduler
participant Reuse as VertexMatchingReuseAnalyzer
participant Synth as RoutingAwareSynthesizer
participant Codegen as CodeGenerator
User->>Compiler: submit scheduled circuit
Compiler->>Decomposer: decompose(nQubits, singleQubitLayers)
Decomposer-->>Compiler: per-layer SingleQubitGateLayer sequences
Compiler->>Scheduler: schedule decomposed layers
Scheduler-->>Compiler: timed layers
Compiler->>Reuse: analyze reuse/layout
Reuse-->>Compiler: reuse decisions
Compiler->>Synth: synthesize layout-aware single-qubit blocks
Synth-->>Compiler: synthesized gates
Compiler->>Codegen: generate target code
Codegen-->>User: compiled output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 20
🤖 Fix all issues with AI agents
In `@include/na/zoned/decomposer/Decomposer.hpp`:
- Around line 83-95: Fix the typos in the Doxygen comment above
get_decomposition_angles: change "hate" to "gate" in the first sentence (qubit
hate -> qubit gate) and change "qate" to "gate" in the `@param` description
(single-qubit qate layer -> single-qubit gate layer); update the comment block
associated with the get_decomposition_angles declaration in Decomposer.hpp
accordingly.
- Around line 54-61: The doc comment for get_U3_angles_from_quaternion in
Decomposer.hpp contains a typo ("U§") in the `@returns` description; update that
comment so the `@returns` line reads that it returns an array of three qc::fp
values [theta, phi, lambda] giving the U3 gate angles (replace "U§" with "U3")
to accurately document the function get_U3_angles_from_quaternion.
In `@src/na/zoned/decomposer/Decomposer.cpp`:
- Around line 144-152: The loop in Decomposer::calc_theta_max copies each
struct_U3 from the layer vector on every iteration; change the range loop to
iterate by const reference (e.g., const auto& gate) so you avoid unnecessary
copies when accessing gate.angles[0] while preserving immutability and behavior.
- Around line 169-172: The loop in Decomposer.cpp uses a signed loop variable
(auto i = 1) compared against qubit_gates.size() which is unsigned, causing a
signed/unsigned comparison; change the loop variable to an unsigned type (e.g.,
size_t i = 1) to match qubit_gates.size(), keeping the loop logic that calls
combine_quaternions(quat, convert_gate_to_quaternion(qubit_gates[i])) unchanged
and ensuring any other similar loops in the Decomposer class use size_t for
indices.
- Line 14: The include path casing is wrong: update the include in
Decomposer.cpp to match the actual header filename by replacing
"na/zoned/decomposer/decomposer.hpp" with the correctly-cased
"na/zoned/decomposer/Decomposer.hpp" so the compiler can find the header on
case-sensitive filesystems; check for any other mismatched includes referencing
"decomposer.hpp" and fix them likewise.
- Around line 135-139: The else branch in Decomposer.cpp currently sets phi and
lambda to 0 when an impossible path is hit; instead, make this an explicit
failure: replace the silent fallback with throwing a descriptive exception
(e.g., throw std::runtime_error) or an assert so the impossible state is caught
immediately, and add the necessary include (<stdexcept> or <cassert>). Reference
the variables phi and lambda and the containing Decomposer code path so the
thrown message explains the unexpected branch and includes any relevant
contextual values if available.
- Around line 225-241: The range-for is copying each struct_U3 into the loop
(for (auto gate : layer)), which is wasteful; change the loop to use a const
reference (for (const auto& gate : layer)) so you don't copy the struct before
calling get_decomposition_angles(gate.angles, theta_max) and constructing the
qc::StandardOperation objects that are emplaced into FrontLayer, MidLayer and
BackLayer.
- Around line 247-252: Replace raw new expressions used when populating GR_plus
and GR_minus with std::make_unique to avoid manual memory management: in the
loop that currently does GR_plus.emplace_back(new qc::StandardOperation(i,
qc::RY, {theta_max / 2})); and GR_minus.emplace_back(new
qc::StandardOperation(i, qc::RY, {-1 * theta_max / 2})); construct and emplace
unique_ptrs via std::make_unique<qc::StandardOperation>(...) instead; ensure the
container element type supports std::unique_ptr and include <memory> if not
already included.
- Line 23: The constructor Decomposer::Decomposer currently assigns N_qubits
inside the body; change it to use a member initializer list to initialize the
N_qubits member (e.g., initialize N_qubits with n_qubits in the constructor
signature) and remove the in-body assignment, keeping the constructor body
empty; update the Decomposer::Decomposer definition accordingly to use the
initializer list for efficiency and idiomatic C++.
- Around line 70-71: The Y gate quaternion is wrong: when checking
op.get().getType() == qc::Y in Decomposer.cpp, quat is incorrectly set to
{0,1,0,0} (same as X); change the assignment so quat is {0,0,1,0} to represent
the Pauli-Y (π rotation about Y-axis) in the branch that handles qc::Y.
- Around line 162-176: The loops currently copy elements (use "auto gate :
layer" and "auto qubit_gates : gates") causing unnecessary copies and potential
overhead; change those to const references (use "const auto& gate" and "const
auto& qubit_gates") to iterate by reference, and also ensure the index variable
for iterating qubit_gates.size() is an unsigned type (e.g., size_t) to avoid
signed/unsigned issues while using functions convert_gate_to_quaternion,
combine_quaternions, get_U3_angles_from_quaternion and constructing struct_U3.
In `@test/na/zoned/test_decomposer.cpp`:
- Around line 168-175: The test RXDecompositionTest declares an unused variable
n; remove the declaration of size_t n = 1; from the TEST(Test,
RXDecompositionTest) block so only the meaningful variables and the EXPECT_THAT
assertion remain (locate the TEST named RXDecompositionTest and delete the
unused `n` variable).
- Around line 206-209: The local variable n is declared as int but is passed to
qc::QuantumComputation and Decomposer which expect size_t, causing implicit
signed-to-unsigned conversion; change the declaration of n to size_t (used by
qc::QuantumComputation qc and Decomposer decomposer) so both
qc::QuantumComputation qc(n) and Decomposer decomposer = Decomposer(n) receive
the correct unsigned type and eliminate the warning.
- Around line 188-200: The test DoubleDecompositionTest declares an unused
variable `n` which should be removed; edit the TEST (DoubleDecompositionTest)
and delete the line "size_t n = 1;" so only the arrays x1 and z2 and the
EXPECT_THAT assertions remain (or alternatively suppress the unused-variable by
casting to void, but preferred fix is to remove the unused declaration).
- Around line 177-186: The test declares an unused variable 'n' in TEST(Test,
U3DecompositionTest); remove the unused declaration (the line "size_t n = 1;")
from test_decomposer.cpp so the test only sets up the u3 array and calls
Decomposer::get_decomposition_angles; if 'n' was intended to be used, instead
incorporate it into the test assertions or function call consistently rather
than leaving it unused.
- Around line 236-239: The test uses a signed int for the qubit count and
indices which can cause sign-conversion warnings; change the variable
declaration and related uses to size_t (replace "int n = 1;" with "size_t n =
1;" and construct Decomposer with Decomposer(n)), and ensure any index arguments
passed to qc::QuantumComputation::u and Decomposer constructors/methods use
size_t to keep types consistent (references: the variable n,
qc::QuantumComputation qc, qc.u(..., 0), and Decomposer decomposer).
- Around line 149-166: Remove the unused variable 'n' and eliminate the heap
allocation for op to fix the memory leak: create a stack-allocated
qc::StandardOperation (e.g., qc::StandardOperation op(0, qc::U, {0, qc::PI,
qc::PI_2});) and pass it into Decomposer::convert_gate_to_quaternion by wrapping
a const base-reference (e.g., std::reference_wrapper<const
qc::Operation>(static_cast<const qc::Operation&>(op))). This removes the need
for new/delete and removes the unused variable.
- Around line 131-147: The test allocates qc::StandardOperation with new and
declares an unused size_t n, causing a leak; replace the raw new and unused
variable by creating the operation as an automatic (stack) object or a smart
pointer and remove n. For example, instantiate qc::StandardOperation op(0,
qc::U, {qc::PI, qc::PI, qc::PI_2}) (or use std::make_unique and deref) and pass
std::reference_wrapper<const qc::Operation>(op) into
Decomposer::convert_gate_to_quaternion, eliminating the raw pointer and ensuring
no leak or unused variables remain.
- Around line 112-129: The test SingleU3GateAngleTest leaks memory by allocating
a qc::StandardOperation with raw new (op) and never deleting it, and declares an
unused variable n; replace the heap allocation with a stack-allocated
qc::StandardOperation (or use a smart pointer) and pass a reference to it into
Decomposer::convert_gate_to_quaternion, and remove the unused variable n so only
the used symbols remain (op -> stack object,
Decomposer::convert_gate_to_quaternion, and remove n).
- Around line 97-110: The test `SingleXGateAngleTest` leaks memory by allocating
`qc::Operation* op = new qc::StandardOperation(0, qc::X)` without deleting it;
replace the raw new with a stack-allocated `qc::StandardOperation` or manage it
with `std::unique_ptr` and pass the correct reference to
`Decomposer::convert_gate_to_quaternion`, and also remove the unused `n` and the
unused `qc` `QuantumComputation` instance; ensure you call
`Decomposer::convert_gate_to_quaternion` with a reference/wrapper to the managed
or stack object and leave `Decomposer::get_U3_angles_from_quaternion` unchanged.
ystade
requested changes
Jan 28, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/na/zoned/Compiler.hpp (1)
204-207: MissingnQubitsargument indecompose()call.The
decomposemethod signature was updated to requirenQubitsas the first parameter, but this call site was not updated. This will cause a compilation error.🐛 Proposed fix
SPDLOG_DEBUG("Decomposing..."); const auto decomposingStart = std::chrono::system_clock::now(); const auto& decomposedSingleQubitGateLayers = - SELF.decompose(singleQubitGateLayers); + SELF.decompose(qComp.getNqubits(), singleQubitGateLayers); const auto decomposingEnd = std::chrono::system_clock::now();
🤖 Fix all issues with AI agents
In `@include/na/zoned/Compiler.hpp`:
- Around line 316-319: The CRTP instantiation is using the wrong concrete type:
change the base of class RoutingAwareNativeGateCompiler from
Compiler<RoutingAwareCompiler, ...> to Compiler<RoutingAwareNativeGateCompiler,
...> so the SELF/CRTP casts are correct; update the template parameter in the
class declaration for RoutingAwareNativeGateCompiler to use its own class name
as the first template argument.
In `@include/na/zoned/decomposer/NativeGateDecomposer.hpp`:
- Around line 37-38: Document the rationale for the epsilon multiplier on the
constant epsilon used for quaternion tolerancing: add a brief inline comment
next to the declaration of constexpr static qc::fp epsilon that explains why
std::numeric_limits<qc::fp>::epsilon() is multiplied by 1024 (e.g., to allow ~10
bits of accumulated floating‑point error margin for quaternion operations and
comparisons), mentioning any empirical or theoretical basis if applicable; keep
the comment short and tied to the symbol epsilon in NativeGateDecomposer.hpp so
future readers understand the tolerance choice.
In `@src/na/zoned/decomposer/NativeGateDecomposer.cpp`:
- Around line 29-31: The quaternion for RZ and P gates is using the full
rotation angle; update the assignment in the branch checking op.get().getType()
== qc::RZ || op.get().getType() == qc::P so it uses half the angle (parameter/2)
when computing the quaternion components (i.e., mirror how RX/RY use
parameter/2) by reading op.get().getParameter().front(), dividing it by 2, and
using that value in the cos(...) and sin(...) calls.
- Around line 249-254: The loop uses a signed int for i which triggers
signed/unsigned comparison against this->nQubits_; change the loop variable to
an unsigned type (e.g. size_t or std::size_t) to match nQubits_ and avoid
warnings. Update the for loop that builds GR_plus and GR_minus (the loop
referencing this->nQubits_, qc::StandardOperation and qc::RY) so i has the same
unsigned type as nQubits_ and ensure any uses of i elsewhere in that scope
remain correct.
In `@test/na/zoned/test_native_gate_decomposer.cpp`:
- Around line 268-299: The final RZ angle assertion in the TwoPauliGatesOneQubit
test is brittle because equivalent phases like 3π/2 and -π/2 should both be
accepted; update the assertion on decomp[0][4]->getParameter() to compare angles
modulo 2π (or normalize both actual and expected to the principal range, e.g.,
(-π, π]) before using ::testing::DoubleNear) so equivalent phases pass; locate
this in the TEST_F named TwoPauliGatesOneQubit and change the expectation that
checks decomp[0][4]->getParameter() (currently comparing to 3 * qc::PI_2) to a
modulo-2π/normalization-based comparison.
Comment on lines
+37
to
+38
| constexpr static qc::fp epsilon = | ||
| std::numeric_limits<qc::fp>::epsilon() * 1024; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Epsilon constant is reasonable but consider documenting the multiplier choice.
The epsilon * 1024 multiplier provides tolerance for accumulated floating-point errors in quaternion operations. Consider adding a brief comment explaining why 1024 was chosen (approximately 10 bits of tolerance).
♻️ Suggested documentation
+ /// Tolerance for floating-point comparisons, scaled to account for
+ /// accumulated rounding errors in quaternion arithmetic (~10 bits margin).
constexpr static qc::fp epsilon =
std::numeric_limits<qc::fp>::epsilon() * 1024;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| constexpr static qc::fp epsilon = | |
| std::numeric_limits<qc::fp>::epsilon() * 1024; | |
| /// Tolerance for floating-point comparisons, scaled to account for | |
| /// accumulated rounding errors in quaternion arithmetic (~10 bits margin). | |
| constexpr static qc::fp epsilon = | |
| std::numeric_limits<qc::fp>::epsilon() * 1024; |
🤖 Prompt for AI Agents
In `@include/na/zoned/decomposer/NativeGateDecomposer.hpp` around lines 37 - 38,
Document the rationale for the epsilon multiplier on the constant epsilon used
for quaternion tolerancing: add a brief inline comment next to the declaration
of constexpr static qc::fp epsilon that explains why
std::numeric_limits<qc::fp>::epsilon() is multiplied by 1024 (e.g., to allow ~10
bits of accumulated floating‑point error margin for quaternion operations and
comparisons), mentioning any empirical or theoretical basis if applicable; keep
the comment short and tied to the symbol epsilon in NativeGateDecomposer.hpp so
future readers understand the tolerance choice.
Comment on lines
268
to
299
| TEST_F(DecomposerTest, TwoPauliGatesOneQubit) { | ||
| // ┌───────┐ ┌───────┐ | ||
| // q: ┤ X ├──┤ Z ├ | ||
| // └───────┘ └───────┘ | ||
| size_t n = 1; | ||
| qc::QuantumComputation qc(n); | ||
| qc.x(0); | ||
| qc.z(0); | ||
| const auto& sched = scheduler.schedule(qc); | ||
| auto decomp = decomposer.decompose(qc.getNqubits(), sched.first); | ||
|
|
||
| EXPECT_EQ(decomp.size(), 1); | ||
| EXPECT_EQ(decomp[0].size(), 5); | ||
| EXPECT_EQ(decomp[0][0]->getType(), qc::RZ); | ||
| EXPECT_THAT(decomp[0][0]->getTargets(), ::testing::ElementsAre(0)); | ||
| EXPECT_THAT(decomp[0][0]->getParameter(), | ||
| ::testing::ElementsAre(::testing::DoubleNear(qc::PI_2, epsilon))); | ||
| EXPECT_TRUE(decomp[0][1]->isCompoundOperation()); | ||
| EXPECT_TRUE(decomp[0][1]->isGlobal(n)); | ||
| EXPECT_EQ(decomp[0][2]->getType(), qc::RZ); | ||
| EXPECT_THAT(decomp[0][2]->getTargets(), ::testing::ElementsAre(0)); | ||
| EXPECT_THAT(decomp[0][2]->getParameter(), | ||
| ::testing::ElementsAre(::testing::DoubleNear(qc::PI, epsilon))); | ||
| EXPECT_TRUE(decomp[0][3]->isCompoundOperation()); | ||
| EXPECT_TRUE(decomp[0][3]->isGlobal(n)); | ||
| EXPECT_EQ(decomp[0][4]->getType(), qc::RZ); | ||
| EXPECT_THAT(decomp[0][4]->getTargets(), ::testing::ElementsAre(0)); | ||
| // TODO: FIgure out i this is always Positive | ||
| EXPECT_THAT( | ||
| decomp[0][4]->getParameter(), | ||
| ::testing::ElementsAre(::testing::DoubleNear(3 * qc::PI_2, epsilon))); | ||
| } |
Contributor
There was a problem hiding this comment.
Make the final RZ angle assertion robust to equivalent phases.
The TODO suggests the sign can flip. Consider accepting equivalent angles (e.g., 3π/2 and −π/2) to avoid a brittle test.
💡 Example adjustment
- EXPECT_THAT(
- decomp[0][4]->getParameter(),
- ::testing::ElementsAre(::testing::DoubleNear(3 * qc::PI_2, epsilon)));
+ EXPECT_THAT(
+ decomp[0][4]->getParameter(),
+ ::testing::ElementsAre(::testing::AnyOf(
+ ::testing::DoubleNear(3 * qc::PI_2, epsilon),
+ ::testing::DoubleNear(-qc::PI_2, epsilon))));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| TEST_F(DecomposerTest, TwoPauliGatesOneQubit) { | |
| // ┌───────┐ ┌───────┐ | |
| // q: ┤ X ├──┤ Z ├ | |
| // └───────┘ └───────┘ | |
| size_t n = 1; | |
| qc::QuantumComputation qc(n); | |
| qc.x(0); | |
| qc.z(0); | |
| const auto& sched = scheduler.schedule(qc); | |
| auto decomp = decomposer.decompose(qc.getNqubits(), sched.first); | |
| EXPECT_EQ(decomp.size(), 1); | |
| EXPECT_EQ(decomp[0].size(), 5); | |
| EXPECT_EQ(decomp[0][0]->getType(), qc::RZ); | |
| EXPECT_THAT(decomp[0][0]->getTargets(), ::testing::ElementsAre(0)); | |
| EXPECT_THAT(decomp[0][0]->getParameter(), | |
| ::testing::ElementsAre(::testing::DoubleNear(qc::PI_2, epsilon))); | |
| EXPECT_TRUE(decomp[0][1]->isCompoundOperation()); | |
| EXPECT_TRUE(decomp[0][1]->isGlobal(n)); | |
| EXPECT_EQ(decomp[0][2]->getType(), qc::RZ); | |
| EXPECT_THAT(decomp[0][2]->getTargets(), ::testing::ElementsAre(0)); | |
| EXPECT_THAT(decomp[0][2]->getParameter(), | |
| ::testing::ElementsAre(::testing::DoubleNear(qc::PI, epsilon))); | |
| EXPECT_TRUE(decomp[0][3]->isCompoundOperation()); | |
| EXPECT_TRUE(decomp[0][3]->isGlobal(n)); | |
| EXPECT_EQ(decomp[0][4]->getType(), qc::RZ); | |
| EXPECT_THAT(decomp[0][4]->getTargets(), ::testing::ElementsAre(0)); | |
| // TODO: FIgure out i this is always Positive | |
| EXPECT_THAT( | |
| decomp[0][4]->getParameter(), | |
| ::testing::ElementsAre(::testing::DoubleNear(3 * qc::PI_2, epsilon))); | |
| } | |
| TEST_F(DecomposerTest, TwoPauliGatesOneQubit) { | |
| // ┌───────┐ ┌───────┐ | |
| // q: ┤ X ├──┤ Z ├ | |
| // └───────┘ └───────┘ | |
| size_t n = 1; | |
| qc::QuantumComputation qc(n); | |
| qc.x(0); | |
| qc.z(0); | |
| const auto& sched = scheduler.schedule(qc); | |
| auto decomp = decomposer.decompose(qc.getNqubits(), sched.first); | |
| EXPECT_EQ(decomp.size(), 1); | |
| EXPECT_EQ(decomp[0].size(), 5); | |
| EXPECT_EQ(decomp[0][0]->getType(), qc::RZ); | |
| EXPECT_THAT(decomp[0][0]->getTargets(), ::testing::ElementsAre(0)); | |
| EXPECT_THAT(decomp[0][0]->getParameter(), | |
| ::testing::ElementsAre(::testing::DoubleNear(qc::PI_2, epsilon))); | |
| EXPECT_TRUE(decomp[0][1]->isCompoundOperation()); | |
| EXPECT_TRUE(decomp[0][1]->isGlobal(n)); | |
| EXPECT_EQ(decomp[0][2]->getType(), qc::RZ); | |
| EXPECT_THAT(decomp[0][2]->getTargets(), ::testing::ElementsAre(0)); | |
| EXPECT_THAT(decomp[0][2]->getParameter(), | |
| ::testing::ElementsAre(::testing::DoubleNear(qc::PI, epsilon))); | |
| EXPECT_TRUE(decomp[0][3]->isCompoundOperation()); | |
| EXPECT_TRUE(decomp[0][3]->isGlobal(n)); | |
| EXPECT_EQ(decomp[0][4]->getType(), qc::RZ); | |
| EXPECT_THAT(decomp[0][4]->getTargets(), ::testing::ElementsAre(0)); | |
| // TODO: FIgure out i this is always Positive | |
| EXPECT_THAT( | |
| decomp[0][4]->getParameter(), | |
| ::testing::ElementsAre(::testing::AnyOf( | |
| ::testing::DoubleNear(3 * qc::PI_2, epsilon), | |
| ::testing::DoubleNear(-qc::PI_2, epsilon)))); | |
| } |
🤖 Prompt for AI Agents
In `@test/na/zoned/test_native_gate_decomposer.cpp` around lines 268 - 299, The
final RZ angle assertion in the TwoPauliGatesOneQubit test is brittle because
equivalent phases like 3π/2 and -π/2 should both be accepted; update the
assertion on decomp[0][4]->getParameter() to compare angles modulo 2π (or
normalize both actual and expected to the principal range, e.g., (-π, π]) before
using ::testing::DoubleNear) so equivalent phases pass; locate this in the
TEST_F named TwoPauliGatesOneQubit and change the expectation that checks
decomp[0][4]->getParameter() (currently comparing to 3 * qc::PI_2) to a
modulo-2π/normalization-based comparison.
ystade
requested changes
Jan 28, 2026
Collaborator
ystade
left a comment
ystade
left a comment
There was a problem hiding this comment.
Hi @cs-tum-st, thanks for your efforts in implementing the decomposer in our neutral atom compiler. As you can see, I made some adoptions to the header file and incoporated the new omponent in the compilation flow.
I did not go through the code in the source file in detail yet because before doing that, I would like to request from you, that you satisfy the comments from @coderabbitai and fix all issues such that the CI show a green check mark. Thanks for your effort again.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/na/zoned/decomposer/NativeGateDecomposer.cpp`:
- Around line 178-203: The getDecompositionAngles function uses the integer
overload abs(...) on floating-point expressions (e.g., abs(angles[0] -
theta_max) and abs(cos(theta_max / 2))), which can cause incorrect behavior;
replace those calls with the floating-point variant (std::abs or std::fabs) and
ensure <cmath> is included so the correct overload for qc::fp is used, keeping
all other logic (chi/alpha calculation, kappa, beta, gamma_plus, gamma_minus)
unchanged.
- Around line 140-149: In NativeGateDecomposer::calcThetaMax, the integer
overload abs() is used on floating-point angle values (gate.angles[0]),
producing incorrect results; replace abs() with the floating-point variant
(e.g., std::abs or std::fabs) so theta_max (qc::fp) is computed correctly for
StructU3::angles elements, and add the appropriate <cmath> include if not
already present.
- Around line 49-55: The quaternion for the Ry(π/2) component in the U2 branch
is using qc::PI_2 (π) instead of the half-angle for the quaternion; in the
branch that checks op.get().getType() == qc::U2 and builds quat via
combineQuaternions, replace the middle quaternion literal {cos(qc::PI_2), 0,
sin(qc::PI_2), 0} with the half-angle values for Ry(π/2), e.g. {cos(qc::PI_4),
0, sin(qc::PI_4), 0}, so combineQuaternions produces Rz(φ)·Ry(π/2)·Rz(λ)
correctly.
- Around line 112-127: The comparisons using abs(quat[0]), abs(quat[3]),
abs(quat[1]), and abs(quat[2]) in NativeGateDecomposer.cpp (within the block
that computes theta, phi, lambda from quat) are calling the integer overload and
must be replaced with the floating-point absolute function; update those calls
to use std::abs (or std::fabs) for all checks (e.g., replace abs(quat[i]) with
std::abs(quat[i])) so the comparisons against epsilon work correctly, and ensure
<cmath> is included if not already.
In `@test/na/zoned/test_native_gate_decomposer.cpp`:
- Line 60: The global variable epsilon should be a compile-time constant; change
the declaration "qc::fp epsilon = 1e-5;" to a constexpr (or at least const)
declaration so it cannot be modified and can be optimized at compile time, e.g.
replace with "constexpr qc::fp epsilon = static_cast<qc::fp>(1e-5);" (or "const"
if constexpr isn't valid for qc::fp) to reference the symbol epsilon.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/na/zoned/decomposer/NativeGateDecomposer.cpp`:
- Around line 245-250: The loop in NativeGateDecomposer.cpp constructs raw
pointers with new for GR_plus and GR_minus; replace those allocations with
std::make_unique<qc::StandardOperation>(...) to create unique_ptrs (e.g., use
GR_plus.emplace_back(std::make_unique<qc::StandardOperation>(i, qc::RY,
{theta_max/2}))). Update the container types if needed to hold std::unique_ptr
and ensure <memory> is included; keep the same arguments for
qc::StandardOperation and mirror the change for both GR_plus and GR_minus.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/na/zoned/decomposer/NativeGateDecomposer.cpp`:
- Around line 86-91: The exception message in NativeGateDecomposer (where the
code builds oss and throws std::invalid_argument) contains ANSI color escape
sequences; remove those sequences and throw a plain, unformatted message instead
(e.g., "Unsupported single-qubit gate: " + op.get().getType()). Update the code
that constructs oss and the throw site to use the plain string so
callers/logs/CI won't receive terminal color codes.
- Line 16: Remove the commented-out include line "// `#include`
\"ir/operations/StandardOperation.hpp\"" from NativeGateDecomposer.cpp to
eliminate dead code; keep the file tidy and ensure no build/reference changes
are needed elsewhere (verify usage of StandardOperation in NativeGateDecomposer
class or related functions to confirm the include truly isn't required before
removal).
- Around line 191-196: The kappa computation can divide by a near-zero
denominator causing inf/nan; update the block that computes qc::fp kappa and
then sets alpha and chi to first check and/or clamp the denominator =
sin(theta_max)*sin(theta_max) - (sin(angles[0]/2)*sin(angles[0]/2)) against a
small floating epsilon (use std::numeric_limits<qc::fp>::epsilon() or a
configurable small constant) and handle the degenerate case by either (a)
setting kappa to a safe large finite value or (b) short-circuiting to a fallback
assignment for alpha and chi, and ensure any earlier comparisons use std::abs
for floating-point. Ensure references to symbols kappa, alpha, chi, angles[0],
theta_max, qc::fp, and qc::TAU are updated accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/na/zoned/decomposer/NativeGateDecomposer.cpp`:
- Around line 255-257: The code creates a temporary stacked variable cop via
qc::CompoundOperation(std::move(GR_plus), true) then copy-constructs it into
ryp, causing an unnecessary copy; instead construct the CompoundOperation
directly inside the std::make_unique call (e.g., std::make_unique<const
qc::CompoundOperation>(std::move(GR_plus), true) ) to avoid the intermediate
copy, and apply the same change for the GR_minus case referenced around Line 263
(replace the stacked temporary with direct in-place construction inside
make_unique for the corresponding ryp variable).
- Around line 227-238: The code currently creates a temporary
qc::StandardOperation (sop) on the stack and then copies it into a
std::unique_ptr for FrontLayer, MidLayer and BackLayer; remove the stack
temporary and construct each StandardOperation directly inside std::make_unique
to avoid the extra copy—replace the sop/op pattern with calls like
std::make_unique<qc::StandardOperation>(gate.qubit, qc::RZ, {decomp_angles[X]})
and emplace_back(std::move(...)) into FrontLayer, MidLayer and BackLayer for the
respective decomp_angles indices.
- Around line 139-148: In NativeGateDecomposer::calcThetaMax replace the
unqualified integer abs() call with the floating-point absolute (e.g. std::fabs
or std::abs) when computing theta_max from gate.angles[0]; locate the loop over
layer and change the assignment that uses abs(gate.angles[0]) so theta_max gets
the correct floating-point magnitude instead of a truncated integer.
In `@test/na/zoned/test_native_gate_decomposer.cpp`:
- Line 172: The inline comment "// gamm minus i - PI_" not PI_2 and game_plus is
0 not PI!!" in test_native_gate_decomposer.cpp is garbled; either remove it or
replace it with a concise, meaningful explanation of the assertion being tested
(for example: describe that the test checks that gamma_minus_i equals PI (not
PI/2) and that gamma_plus equals 0), referencing the test name/test block in
test_native_gate_decomposer.cpp so reviewers can locate it; ensure the new
comment clearly states the expected values and why they matter for the test's
validation.
- Around line 252-253: The two stale TODO comments "TEST with U3(o,?,?)" and
"TEST with two Single Qubit Layers" in test_native_gate_decomposer.cpp indicate
missing test cases; either implement those tests or replace each comment with a
tracked issue reference. To fix, add concrete unit tests exercising a U3 gate
decomposition (e.g., a test function that constructs a U3(o,?,?) gate, runs
NativeGateDecomposer::decompose and asserts expected output) and a test for
composing two single-qubit layers, or create and link issue IDs in the comments
(e.g., TODO: ISSUE-1234) so the placeholders are tracked; update or remove the
two specific comment lines accordingly. Ensure test functions are named clearly
(e.g., TEST(NativeGateDecomposer, U3Decomposition) and
TEST(NativeGateDecomposer, TwoSingleQubitLayers)) so they’re easy to find and
run.
- Line 91: Replace unqualified sqrt calls with std::sqrt in the test to avoid
relying on ADL/transitive includes: update the initialization of qc::fp r2 = 1 /
sqrt(2) to use std::sqrt and similarly change the other occurrences referenced
(the sqrt uses at the locations around lines where r2 and subsequent
calculations appear) so all calls use std::sqrt to fully qualify the function.
changed check for calculation of decomposition angles for sin/theta_m) near sin(theta)
changed check for calculation of decomposition angles for sin/theta_m) near sin(theta)
# Conflicts: # src/na/zoned/decomposer/NativeGateDecomposer.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/na/zoned/decomposer/NativeGateDecomposer.cpp`:
- Around line 124-125: Replace the garbled inline comment next to the theta
assignment (theta = qc::PI) in NativeGateDecomposer.cpp: remove the leftover
debug note `// or § PI if sin(theta/2)=-1... Relevant? Or is the -1= exp(i*pi)
just global phase` and replace it with a concise, clear comment or delete it
entirely (e.g., "// set theta to PI; global phase from exp(i*pi) is irrelevant")
so the intent of the theta = qc::PI assignment is unambiguous; ensure the change
targets the same statement where the variable theta and qc::PI are used.
- Around line 205-209: The method NativeGateDecomposer::decompose currently
mutates the member nQubits_, creating hidden state relied on by transformToU3;
remove this hidden coupling by either (preferable) adding a size_t nQubits
parameter to transformToU3 and any helpers it calls and pass the local nQubits
from decompose, or alternatively initialize nQubits_ once in the
NativeGateDecomposer constructor and stop assigning it in decompose; update all
calls to transformToU3 to use the new parameter (or rely on the
constructor-initialized member) and remove the in-method assignment to nQubits_
so the object remains stateless/reentrant.
Comment on lines
+124
to
+125
| theta = qc::PI; // or § PI if sin(theta/2)=-1... Relevant? Or is the -1= | ||
| // exp(i*pi) just global phase |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Garbled comment — leftover debug note.
// or § PI if sin(theta/2)=-1... Relevant? Or is the -1= exp(i*pi) just global phase reads like a development note. Clean up or remove.
🤖 Prompt for AI Agents
In `@src/na/zoned/decomposer/NativeGateDecomposer.cpp` around lines 124 - 125,
Replace the garbled inline comment next to the theta assignment (theta = qc::PI)
in NativeGateDecomposer.cpp: remove the leftover debug note `// or § PI if
sin(theta/2)=-1... Relevant? Or is the -1= exp(i*pi) just global phase` and
replace it with a concise, clear comment or delete it entirely (e.g., "// set
theta to PI; global phase from exp(i*pi) is irrelevant") so the intent of the
theta = qc::PI assignment is unambiguous; ensure the change targets the same
statement where the variable theta and qc::PI are used.
Comment on lines
+205
to
+209
| auto NativeGateDecomposer::decompose( | ||
| const size_t nQubits, | ||
| const std::vector<SingleQubitGateRefLayer>& singleQubitGateLayers) | ||
| -> std::vector<SingleQubitGateLayer> { | ||
| nQubits_ = nQubits; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Setting nQubits_ inside decompose makes the decomposer silently stateful.
decompose mutates nQubits_ (line 209) which is then read by transformToU3 (line 155). This couples two logically independent calls via hidden state, making the object non-reentrant and surprising for callers. Consider either passing nQubits directly to transformToU3 as a parameter, or setting nQubits_ once during construction.
🤖 Prompt for AI Agents
In `@src/na/zoned/decomposer/NativeGateDecomposer.cpp` around lines 205 - 209, The
method NativeGateDecomposer::decompose currently mutates the member nQubits_,
creating hidden state relied on by transformToU3; remove this hidden coupling by
either (preferable) adding a size_t nQubits parameter to transformToU3 and any
helpers it calls and pass the local nQubits from decompose, or alternatively
initialize nQubits_ once in the NativeGateDecomposer constructor and stop
assigning it in decompose; update all calls to transformToU3 to use the new
parameter (or rely on the constructor-initialized member) and remove the
in-method assignment to nQubits_ so the object remains stateless/reentrant.
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
Added a decomposer class which takes single qubit layers and decomposes them into local Rz and global Ry gates.
Checklist: