✨ Add Native Gate Decomposer to Zoned Neutral Atom Compiler#1050
Draft
ystade wants to merge 145 commits into
Draft
✨ Add Native Gate Decomposer to Zoned Neutral Atom Compiler#1050ystade wants to merge 145 commits into
ystade wants to merge 145 commits into
Conversation
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
Not necesary anymore as the function `getOps` is not used anymore, we directly use the iteration functionailty of the compound operation. This reverts commit cbc845e.
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds a ChangesNative Gate Decomposition Feature
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Compiler
participant NativeGateDecomposer
participant DAG as DirectedGraph
Compiler->>NativeGateDecomposer: decompose(nQubits, singleQubitLayers, twoQubitLayers)
NativeGateDecomposer->>NativeGateDecomposer: transformToU3(layers)
alt thetaOptSchedule enabled
NativeGateDecomposer->>DAG: convertCircuitToDAG(schedule)
NativeGateDecomposer->>NativeGateDecomposer: scheduleRemaining(subproblem, memo)
NativeGateDecomposer->>NativeGateDecomposer: buildSchedule(minimalPath)
else default
NativeGateDecomposer->>NativeGateDecomposer: emit RZ + global RY layers
end
NativeGateDecomposer-->>Compiler: DecompositionResult
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 16
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)
185-192: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse an explicit
std::array<size_t, 3>here
std::array{std::numeric_limits<size_t>::max(), 0UL, 0UL}relies on CTAD, andstd::arrayrequires all elements to have the exact same type. On LLP64/Windows,size_tandunsigned longdiffer, so this initializer is ill-formed and breaks the build; the explicitstd::array<size_t, 3>{...}avoids it.🤖 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 `@include/na/zoned/Compiler.hpp` around lines 185 - 192, The accumulator initialization in Compiler::compile uses CTAD with std::array, which can fail when the literal types don’t exactly match size_t on some platforms. Update the std::accumulate initial value to an explicit std::array<size_t, 3> in the twoQubitGateLayers stats block, keeping the rest of the min/sum/max logic unchanged.
🤖 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 `@bindings/na/register_zoned.cpp`:
- Around line 526-538: The stats() binding in routingAwareNativeGateCompiler is
typed too narrowly as dict[str, float] even though getStatistics() can serialize
nested objects like layoutSynthesizerStatistics. Update the nb::cast/typing in
stats() to reflect a nested dictionary shape instead of forcing float values,
and make the docstring/stub match the actual JSON structure returned by
RoutingAwareNativeGateCompiler::getStatistics().
In `@eval/na/zoned/eval_native_gate_decomposition.py`:
- Around line 66-125: The timeout/OOM guard in process_benchmark is currently
bypassed because the direct _compile_wrapper call is used instead of
run_with_process_timeout, leaving _proc_target, TIMEOUT, and the
TimeoutError/MemoryError handling effectively dead. Restore the safeguarded call
path in process_benchmark so the compile step runs through
run_with_process_timeout with the existing timeout translation logic, or remove
the unused timeout helpers and exception branches if this protection is
intentionally disabled.
- Around line 76-78: Document the intentional lint suppressions in the native
gate decomposition helpers by adding a brief justification next to the affected
`except Exception` path and the `it` parameter usage in the worker/dispatch
code. In the function that uses `q.put(...)` and the dispatch helper that takes
`it`, explain that `# noqa: BLE001` is required to catch and relay exceptions
across the process boundary and that `# noqa: ARG002` is needed because `it` is
part of a uniform signature even when unused. Keep the suppression only where
necessary and reference the relevant worker/dispatch symbols so future readers
understand why the warnings are being ignored.
- Around line 10-17: The script metadata in eval_native_gate_decomposition.py
pins mqt.qmap to an unavailable stable release, which can break uv run --script
dependency resolution. Update the dependency entry in the script header to a
published mqt.qmap version such as 3.7.0, or explicitly choose an available
pre-release if that is intended, while keeping the rest of the script metadata
unchanged.
In `@include/na/zoned/decomposer/NativeGateDecomposer.hpp`:
- Around line 16-20: Make NativeGateDecomposer.hpp self-contained by adding
direct includes for every standard-library type or utility it uses instead of
relying on transitive headers. Review the declarations in this header and ensure
each referenced symbol from the standard library is covered by its own include
so the header can be used independently regardless of include order.
- Around line 404-411: The memo map in scheduleRemaining currently relies on
std::hash for std::array<std::vector<size_t>, 3>, which is non-conforming
because the key is entirely standard-library types. Introduce and use a
project-owned hasher for this key, then thread it through the std::unordered_map
parameter in NativeGateDecomposer::scheduleRemaining and any related call sites
so memoization continues to work without specializing std::hash.
In `@src/na/zoned/decomposer/NativeGateDecomposer.cpp`:
- Around line 18-19: NativeGateDecomposer.cpp is relying on several standard
facilities without including their owning headers directly. Update the include
block near the top of the file to add the standard headers for the algorithms,
ranges, math utilities, assertions, numeric helpers, exceptions, streams, and
unordered sets used by the implementation, alongside the existing variant/vector
includes, so the translation unit is self-contained.
- Around line 465-479: Replace the TODO in NativeGateDecomposer::decompose with
an explicit schedule-shape validation before iterating. Check that
schedule.first has exactly one more layer than schedule.second, and that both
are non-empty as expected, so the later use of schedule.first.at(i),
schedule.second.at(i), and schedule.first.back() is safe. If the shape is
invalid, fail fast with a clear error instead of relying on out-of-range access
or skipping a single-qubit layer.
- Around line 381-385: The current single-qubit layer handling in
NativeGateDecomposer::decompose assumes vSort[0] always exists, but this breaks
when currentSingleQubitGates is empty. Add an explicit empty-check before any
access to vSort[0] in this block, and skip or finalize the layer safely when
there are no single-qubit gates to process. Keep the fix localized around the
currentSingleQubitGates/vSort path so the existing U3Gate-based logic only runs
when a gate is actually present.
- Around line 424-425: The qubit accumulation in
NativeGateDecomposer::pushQubits is mutating pSquareQubits by merging it into
pushQubits, which clears the accumulated set for later iterations. Update this
logic in NativeGateDecomposer.cpp so pSquareQubits remains intact by copying or
inserting its contents into pushQubits instead of using merge, and keep the
surrounding accumulation flow in the same loop consistent with the existing
pushQubits/pSquareQubits symbols.
- Around line 535-543: The sift logic in NativeGateDecomposer is only marking
single-qubit operands as removed, so selected two-qubit gates can leave their
qubits available for later operations in the same subproblem. Update the
std::visit handling in the sift path to also record both qubits from the
two-qubit gate case in the removed set, alongside the existing single-qubit
handling, so dependencies on those operands are not reordered.
- Around line 403-438: Clear emplaceBackNodes before it is reused for Condition
2 in NativeGateDecomposer::decompose so nodes collected during Condition 3 do
not leak into the next potentialArg iteration. Keep the existing flow around
pSquareQubits, vP1Star, and vc1Star, but reset emplaceBackNodes at the
appropriate point before repopulating it from the vP1Star scan, and again before
the vc1Star scan if needed.
In `@test/na/zoned/matcher.hpp`:
- Around line 20-35: AnglesNear currently uses plain fabs differences for theta,
phi, and lambda, which fails for equivalent angles across the ±π/TAU wraparound.
Update the matcher in matcher.hpp to compare each component using
wraparound-aware angular distance instead of direct subtraction, so values like
π and -π are treated as equal within tolerance. Keep the existing AnglesNear
matcher name and adjust its comparison logic and failure message formatting
accordingly.
In `@test/na/zoned/test_native_gate_decomposer.cpp`:
- Around line 338-513: The decomposition tests repeat the same RZ assertion
sequence many times, especially in NativeGateDecomposerTest cases like
OneRXOneQubit, OneU3OneQubit, TwoGatesOneQubit, and TwoGatesTwoQubits. Extract a
small local helper near the test fixtures, such as an expectRZ-style function
that takes an operation, target qubit, and angle, and centralizes the
getType/getTargets/getParameter checks with NativeGateDecomposer::epsilon. Then
replace the repeated inline assertions throughout those test cases with the
helper to reduce boilerplate and keep the tests easier to extend.
In `@test/na/zoned/test_theta_opt_scheduler.cpp`:
- Around line 195-219: Remove the unused subproblemGraph setup from the affected
theta scheduler tests: in NextMomentsPush, NextMomentsCond2, and
NextMomentsCond3, the DirectedGraph instance is created and addNode({}) is
called but the variable is never used. Keep the test focused on the actual
getPossibleLayers/sift assertions, or if the graph was intended to matter, wire
it into those calls and assertions instead of leaving it as dead setup.
- Around line 144-147: The test uses std::iota in the theta opt scheduler test,
but the include list does not directly provide it. Update the test’s includes in
test_theta_opt_scheduler.cpp to add <numeric> so the use of std::iota is
self-contained and not dependent on indirect headers; the relevant location is
the test that calls NativeGateDecomposer::sift.
---
Outside diff comments:
In `@include/na/zoned/Compiler.hpp`:
- Around line 185-192: The accumulator initialization in Compiler::compile uses
CTAD with std::array, which can fail when the literal types don’t exactly match
size_t on some platforms. Update the std::accumulate initial value to an
explicit std::array<size_t, 3> in the twoQubitGateLayers stats block, keeping
the rest of the min/sum/max logic unchanged.
🪄 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: da0921a2-f8d0-4b1f-8228-9deef8ae73d7
📒 Files selected for processing (16)
bindings/na/register_zoned.cppeval/na/zoned/eval_native_gate_decomposition.pyinclude/na/zoned/Compiler.hppinclude/na/zoned/decomposer/DecomposerBase.hppinclude/na/zoned/decomposer/NativeGateDecomposer.hppinclude/na/zoned/decomposer/NoOpDecomposer.hpppython/mqt/qmap/na/zoned.pyisrc/na/zoned/decomposer/NativeGateDecomposer.cppsrc/na/zoned/decomposer/NoOpDecomposer.cpptest/na/zoned/CMakeLists.txttest/na/zoned/matcher.hpptest/na/zoned/test_compiler.cpptest/na/zoned/test_heuristic_placer.cpptest/na/zoned/test_independent_set_router.cpptest/na/zoned/test_native_gate_decomposer.cpptest/na/zoned/test_theta_opt_scheduler.cpp
Comment on lines
+526
to
538
| routingAwareNativeGateCompiler.def( | ||
| "stats", | ||
| [](const na::zoned::RoutingAwareNativeGateCompiler& self) { | ||
| const auto json = nb::module_::import_("json"); | ||
| const auto loads = json.attr("loads"); | ||
| const nlohmann::json stats = self.getStatistics(); | ||
| const auto dict = loads(stats.dump()); | ||
| return nb::cast<nb::typed<nb::dict, nb::str, nb::float_>>(dict); | ||
| }, | ||
| R"pb(Get the statistics of the last compilation. | ||
|
|
||
| Returns: | ||
| The statistics as a dictionary)pb"); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
stats() typed as dict[str, float] but contains a nested object.
nb::typed<nb::dict, nb::str, nb::float_> (and the resulting dict[str, float] stub) claims every value is a float, but Statistics includes a nested layoutSynthesizerStatistics sub-struct that serializes to a nested JSON object, not a scalar. This is confirmed by the eval script accessing stats["layoutSynthesizerStatistics"]["placementTime"]. The same pattern already exists (unchanged) for RoutingAwareCompiler.stats(), so this duplicates a pre-existing typing inaccuracy into the new class.
💡 Suggested fix
- return nb::cast<nb::typed<nb::dict, nb::str, nb::float_>>(dict);
+ return nb::cast<nb::typed<nb::dict, nb::str, nb::object>>(dict);📝 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
| routingAwareNativeGateCompiler.def( | |
| "stats", | |
| [](const na::zoned::RoutingAwareNativeGateCompiler& self) { | |
| const auto json = nb::module_::import_("json"); | |
| const auto loads = json.attr("loads"); | |
| const nlohmann::json stats = self.getStatistics(); | |
| const auto dict = loads(stats.dump()); | |
| return nb::cast<nb::typed<nb::dict, nb::str, nb::float_>>(dict); | |
| }, | |
| R"pb(Get the statistics of the last compilation. | |
| Returns: | |
| The statistics as a dictionary)pb"); | |
| routingAwareNativeGateCompiler.def( | |
| "stats", | |
| [](const na::zoned::RoutingAwareNativeGateCompiler& self) { | |
| const auto json = nb::module_::import_("json"); | |
| const auto loads = json.attr("loads"); | |
| const nlohmann::json stats = self.getStatistics(); | |
| const auto dict = loads(stats.dump()); | |
| return nb::cast<nb::typed<nb::dict, nb::str, nb::object>>(dict); | |
| }, | |
| R"pb(Get the statistics of the last compilation. | |
| Returns: | |
| The statistics as a dictionary)pb"); |
🤖 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 `@bindings/na/register_zoned.cpp` around lines 526 - 538, The stats() binding
in routingAwareNativeGateCompiler is typed too narrowly as dict[str, float] even
though getStatistics() can serialize nested objects like
layoutSynthesizerStatistics. Update the nb::cast/typing in stats() to reflect a
nested dictionary shape instead of forcing float values, and make the
docstring/stub match the actual JSON structure returned by
RoutingAwareNativeGateCompiler::getStatistics().
Comment on lines
+10
to
+17
| # /// script | ||
| # dependencies = [ | ||
| # "mqt.bench==2.1.0", | ||
| # "mqt.qmap==3.7.1", | ||
| # ] | ||
| # [tool.uv] | ||
| # exclude-newer = "2026-06-30T12:59:59Z" | ||
| # /// |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What is the latest released version of mqt.qmap on PyPI?
💡 Result:
The latest stable released version of mqt.qmap on PyPI is 3.7.0, which was released on June 17, 2026 [1][2].
Citations:
🌐 Web query:
mqt.qmap 3.7.1 PyPI
💡 Result:
As of July 2, 2026, the mqt.qmap package on PyPI has not officially released version 3.7.1 [1][2]. The most recent stable version available is 3.7.0 [2]. Documentation for a development version, 3.7.1.dev2, exists [3], indicating that 3.7.1 is in pre-release or development status and has not yet been pushed to the stable PyPI index [1][3].
Citations:
- 1: https://pypi.org/project/mqt.qmap/
- 2: https://pypi.org/project/mqt.qmap/3.7.0/
- 3: https://mqtqmap.readthedocs.io/
Use a published mqt.qmap version here — 3.7.1 is not on the stable PyPI index, so uv run --script can fail to resolve this dependency. 3.7.0 is the latest stable release; otherwise pin an available pre-release explicitly.
🤖 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 `@eval/na/zoned/eval_native_gate_decomposition.py` around lines 10 - 17, The
script metadata in eval_native_gate_decomposition.py pins mqt.qmap to an
unavailable stable release, which can break uv run --script dependency
resolution. Update the dependency entry in the script header to a published
mqt.qmap version such as 3.7.0, or explicitly choose an available pre-release if
that is intended, while keeping the rest of the script metadata unchanged.
Comment on lines
+66
to
+125
| def _proc_target(q: Queue, func: Callable[P, R], args: P.args, kwargs: P.kwargs) -> None: | ||
| """Target function for the process to run the given function and put the result in the queue. | ||
|
|
||
| Args: | ||
| q: The queue to put the result in. | ||
| func: The function to run. | ||
| args: The positional arguments to pass to the function. | ||
| kwargs: The keyword arguments to pass to the function. | ||
| """ | ||
| try: | ||
| q.put(("ok", func(*args, **kwargs))) | ||
| except Exception as e: # noqa: BLE001 | ||
| q.put(("err", e)) | ||
|
|
||
|
|
||
| def run_with_process_timeout(func: Callable[P, R], timeout: float, *args: P.args, **kwargs: P.kwargs) -> R: | ||
| """Run a function in a separate process and timeout after the given timeout. | ||
|
|
||
| Args: | ||
| func: The function to run. | ||
| timeout: The timeout in seconds. | ||
| *args: The positional arguments to pass to the function. | ||
| **kwargs: The keyword arguments to pass to the function. | ||
|
|
||
| Returns: | ||
| The result of the function. | ||
|
|
||
| Raises: | ||
| TimeoutError: If the function times out. | ||
| Exception: If the function raises an exception. | ||
| """ | ||
| # "fork" context avoids pickling bound methods but is Unix/macOS only. | ||
| ctx = get_context("fork") # use fork so bound methods don't need to be pickled on macOS/Unix | ||
| q = ctx.Queue() | ||
| p = ctx.Process(target=_proc_target, args=(q, func, args, kwargs)) | ||
| p.start() | ||
| try: | ||
| status, payload = q.get(block=True, timeout=timeout) | ||
| except queue.Empty as e: | ||
| msg = f"Timed out after {timeout}s" | ||
| raise TimeoutError(msg) from e | ||
| finally: | ||
| if p.is_alive(): | ||
| p.terminate() | ||
| p.join(2) | ||
| if p.is_alive(): | ||
| p.kill() | ||
| p.join() | ||
| if status == "ok": | ||
| return payload | ||
| if ( | ||
| status == "err" | ||
| and isinstance(payload, Exception) | ||
| and payload.args | ||
| and isinstance(payload.args[0], str) | ||
| and "Maximum number of nodes reached" in payload.args[0] | ||
| ): | ||
| msg = "Out of memory" | ||
| raise MemoryError(msg) | ||
| raise payload |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Timeout/OOM-detection mechanism is dead code — the guarded call is commented out.
_proc_target and run_with_process_timeout implement process-isolated timeout and "Maximum number of nodes reached" → MemoryError translation, but process_benchmark calls _compile_wrapper directly with the safeguarded call commented out (line 204). This makes TIMEOUT, _proc_target, and run_with_process_timeout unused, and the except TimeoutError/except MemoryError branches (205-212) effectively unreachable. Given benchmarks run up to 100 qubits with a recursive theta-optimization scheduler, losing this guard risks the eval run hanging indefinitely on pathological inputs.
♻️ Restore the guarded call (or remove the now-dead code if intentionally disabled)
- code, stats = _compile_wrapper(
- compiler, qc
- ) # run_with_process_timeout(_compile_wrapper, TIMEOUT, compiler, qc)
+ code, stats = run_with_process_timeout(_compile_wrapper, TIMEOUT, compiler, qc)Also applies to: 202-204
🤖 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 `@eval/na/zoned/eval_native_gate_decomposition.py` around lines 66 - 125, The
timeout/OOM guard in process_benchmark is currently bypassed because the direct
_compile_wrapper call is used instead of run_with_process_timeout, leaving
_proc_target, TIMEOUT, and the TimeoutError/MemoryError handling effectively
dead. Restore the safeguarded call path in process_benchmark so the compile step
runs through run_with_process_timeout with the existing timeout translation
logic, or remove the unused timeout helpers and exception branches if this
protection is intentionally disabled.
Comment on lines
+76
to
+78
| q.put(("ok", func(*args, **kwargs))) | ||
| except Exception as e: # noqa: BLE001 | ||
| q.put(("err", e)) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Document the reason for # noqa suppressions.
Both # noqa: BLE001 (broad except, needed to relay exceptions across process boundary) and # noqa: ARG002 (unused it, needed for uniform dispatch signature) are plausible, but neither is accompanied by a comment explaining why the suppression is necessary.
As per coding guidelines, "PREFER fixing reported warnings over suppressing them (for example, avoid # noqa unless necessary and document why)."
📝 Example
try:
q.put(("ok", func(*args, **kwargs)))
- except Exception as e: # noqa: BLE001
+ except Exception as e: # noqa: BLE001 -- must forward any exception from the child process to the parent via the queue
q.put(("err", e))Also applies to: 444-451
🤖 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 `@eval/na/zoned/eval_native_gate_decomposition.py` around lines 76 - 78,
Document the intentional lint suppressions in the native gate decomposition
helpers by adding a brief justification next to the affected `except Exception`
path and the `it` parameter usage in the worker/dispatch code. In the function
that uses `q.put(...)` and the dispatch helper that takes `it`, explain that `#
noqa: BLE001` is required to catch and relay exceptions across the process
boundary and that `# noqa: ARG002` is needed because `it` is part of a uniform
signature even when unused. Keep the suppression only where necessary and
reference the relevant worker/dispatch symbols so future readers understand why
the warnings are being ignored.
Source: Coding guidelines
Comment on lines
+16
to
+20
| #include <format> | ||
| #include <unordered_map> | ||
| #include <unordered_set> | ||
| #include <variant> | ||
| #include <vector> |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Make this header self-contained.
This header uses several standard-library symbols without directly including their headers, making it dependent on transitive include order.
Proposed include fix
+#include <algorithm>
+#include <array>
+#include <cstddef>
`#include` <format>
+#include <functional>
+#include <limits>
+#include <ranges>
+#include <sstream>
+#include <stdexcept>
`#include` <unordered_map>
`#include` <unordered_set>
+#include <utility>
`#include` <variant>
`#include` <vector>📝 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
| #include <format> | |
| #include <unordered_map> | |
| #include <unordered_set> | |
| #include <variant> | |
| #include <vector> | |
| `#include` <algorithm> | |
| `#include` <array> | |
| `#include` <cstddef> | |
| `#include` <format> | |
| `#include` <functional> | |
| `#include` <limits> | |
| `#include` <ranges> | |
| `#include` <sstream> | |
| `#include` <stdexcept> | |
| `#include` <unordered_map> | |
| `#include` <unordered_set> | |
| `#include` <utility> | |
| `#include` <variant> | |
| `#include` <vector> |
🤖 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 `@include/na/zoned/decomposer/NativeGateDecomposer.hpp` around lines 16 - 20,
Make NativeGateDecomposer.hpp self-contained by adding direct includes for every
standard-library type or utility it uses instead of relying on transitive
headers. Review the declarations in this header and ensure each referenced
symbol from the standard library is covered by its own include so the header can
be used independently regardless of include order.
Comment on lines
+535
to
+543
| std::visit(overloads{[&singleQubitGates, &removed, | ||
| node](const U3Gate& u3) -> void { | ||
| singleQubitGates.emplace_back(node); | ||
| removed.emplace(u3.qubit); | ||
| }, | ||
| [&twoQubitGates, | ||
| node](const std::array<qc::Qubit, 2>&) -> void { | ||
| twoQubitGates.emplace_back(node); | ||
| }}, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Mark two-qubit operands as used in sift.
Selected CZ gates do not add their qubits to removed, allowing later operations on the same qubits into the same subproblem and potentially reordering dependencies.
Proposed fix
node](const U3Gate& u3) -> void {
singleQubitGates.emplace_back(node);
removed.emplace(u3.qubit);
},
- [&twoQubitGates,
- node](const std::array<qc::Qubit, 2>&) -> void {
+ [&twoQubitGates, &removed,
+ node](const std::array<qc::Qubit, 2>& cz) -> void {
twoQubitGates.emplace_back(node);
+ removed.emplace(cz[0]);
+ removed.emplace(cz[1]);
}},
op);📝 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
| std::visit(overloads{[&singleQubitGates, &removed, | |
| node](const U3Gate& u3) -> void { | |
| singleQubitGates.emplace_back(node); | |
| removed.emplace(u3.qubit); | |
| }, | |
| [&twoQubitGates, | |
| node](const std::array<qc::Qubit, 2>&) -> void { | |
| twoQubitGates.emplace_back(node); | |
| }}, | |
| std::visit(overloads{[&singleQubitGates, &removed, | |
| node](const U3Gate& u3) -> void { | |
| singleQubitGates.emplace_back(node); | |
| removed.emplace(u3.qubit); | |
| }, | |
| [&twoQubitGates, &removed, | |
| node](const std::array<qc::Qubit, 2>& cz) -> void { | |
| twoQubitGates.emplace_back(node); | |
| removed.emplace(cz[0]); | |
| removed.emplace(cz[1]); | |
| }}, |
🤖 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 `@src/na/zoned/decomposer/NativeGateDecomposer.cpp` around lines 535 - 543, The
sift logic in NativeGateDecomposer is only marking single-qubit operands as
removed, so selected two-qubit gates can leave their qubits available for later
operations in the same subproblem. Update the std::visit handling in the sift
path to also record both qubits from the two-qubit gate case in the removed set,
alongside the existing single-qubit handling, so dependencies on those operands
are not reordered.
Comment on lines
+20
to
+35
| MATCHER_P2(AnglesNear, expected, tolerance, | ||
| std::string("angles ") + (negation ? "aren't" : "are") + | ||
| " near expected") { | ||
| if (std::fabs(arg.theta - expected.theta) <= tolerance && | ||
| std::fabs(arg.phi - expected.phi) <= tolerance && | ||
| std::fabs(arg.lambda - expected.lambda) <= tolerance) { | ||
| return true; | ||
| } | ||
| *result_listener << "actual: {theta=" << arg.theta << ", phi=" << arg.phi | ||
| << ", lambda=" << arg.lambda << "} " | ||
| << "expected: {theta=" << expected.theta | ||
| << ", phi=" << expected.phi << ", lambda=" << expected.lambda | ||
| << "} " | ||
| << "(tolerance=" << tolerance << ")"; | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Angle comparison doesn't handle wraparound.
AnglesNear compares theta/phi/lambda via plain fabs difference. Since decomposition angles are computed with fmod(..., qc::TAU) upstream (e.g. NativeGateDecomposer::getDecompositionAngles), values near the ±π boundary (e.g. π vs -π) are mathematically equal but would fail this tolerance check. This risks flaky test failures as more angle-producing code paths are exercised.
💡 Suggested wraparound-aware comparison
+constexpr auto angleDiff = [](double a, double b) {
+ double d = std::fmod(a - b, 2 * M_PI);
+ if (d > M_PI) d -= 2 * M_PI;
+ if (d < -M_PI) d += 2 * M_PI;
+ return std::fabs(d);
+};
MATCHER_P2(AnglesNear, expected, tolerance, ...) {
- if (std::fabs(arg.theta - expected.theta) <= tolerance &&
- std::fabs(arg.phi - expected.phi) <= tolerance &&
- std::fabs(arg.lambda - expected.lambda) <= tolerance) {
+ if (angleDiff(arg.theta, expected.theta) <= tolerance &&
+ angleDiff(arg.phi, expected.phi) <= tolerance &&
+ angleDiff(arg.lambda, expected.lambda) <= tolerance) {📝 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
| MATCHER_P2(AnglesNear, expected, tolerance, | |
| std::string("angles ") + (negation ? "aren't" : "are") + | |
| " near expected") { | |
| if (std::fabs(arg.theta - expected.theta) <= tolerance && | |
| std::fabs(arg.phi - expected.phi) <= tolerance && | |
| std::fabs(arg.lambda - expected.lambda) <= tolerance) { | |
| return true; | |
| } | |
| *result_listener << "actual: {theta=" << arg.theta << ", phi=" << arg.phi | |
| << ", lambda=" << arg.lambda << "} " | |
| << "expected: {theta=" << expected.theta | |
| << ", phi=" << expected.phi << ", lambda=" << expected.lambda | |
| << "} " | |
| << "(tolerance=" << tolerance << ")"; | |
| return false; | |
| } | |
| constexpr auto angleDiff = [](double a, double b) { | |
| double d = std::fmod(a - b, 2 * M_PI); | |
| if (d > M_PI) d -= 2 * M_PI; | |
| if (d < -M_PI) d += 2 * M_PI; | |
| return std::fabs(d); | |
| }; | |
| MATCHER_P2(AnglesNear, expected, tolerance, | |
| std::string("angles ") + (negation ? "aren't" : "are") + | |
| " near expected") { | |
| if (angleDiff(arg.theta, expected.theta) <= tolerance && | |
| angleDiff(arg.phi, expected.phi) <= tolerance && | |
| angleDiff(arg.lambda, expected.lambda) <= tolerance) { | |
| return true; | |
| } | |
| *result_listener << "actual: {theta=" << arg.theta << ", phi=" << arg.phi | |
| << ", lambda=" << arg.lambda << "} " | |
| << "expected: {theta=" << expected.theta | |
| << ", phi=" << expected.phi << ", lambda=" << expected.lambda | |
| << "} " | |
| << "(tolerance=" << tolerance << ")"; | |
| return false; | |
| } |
🤖 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 `@test/na/zoned/matcher.hpp` around lines 20 - 35, AnglesNear currently uses
plain fabs differences for theta, phi, and lambda, which fails for equivalent
angles across the ±π/TAU wraparound. Update the matcher in matcher.hpp to
compare each component using wraparound-aware angular distance instead of direct
subtraction, so values like π and -π are treated as equal within tolerance. Keep
the existing AnglesNear matcher name and adjust its comparison logic and failure
message formatting accordingly.
Comment on lines
+338
to
+513
| TEST_F(NativeGateDecomposerTest, OneRXOneQubit) { | ||
| // ┌───────┐ | ||
| // q: ┤ Rx(π) ├ | ||
| // └───────┘ | ||
| qc::QuantumComputation qc(1); | ||
| qc.rx(qc::PI, 0); | ||
| const auto& [singleQubitLayers, twoQubitLayers] = scheduler.schedule(qc); | ||
| const auto& decompSingleQubitLayers = | ||
| decomposer.decompose(qc.getNqubits(), singleQubitLayers, twoQubitLayers) | ||
| .singleQubitLayers; | ||
| EXPECT_EQ(decompSingleQubitLayers.size(), 1); | ||
| EXPECT_EQ(decompSingleQubitLayers[0].size(), 5); | ||
| EXPECT_EQ(decompSingleQubitLayers[0][0]->getType(), qc::RZ); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][0]->getTargets(), | ||
| ::testing::ElementsAre(0)); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][0]->getParameter(), | ||
| ::testing::ElementsAre(::testing::DoubleNear( | ||
| qc::PI_2, NativeGateDecomposer::epsilon))); | ||
| EXPECT_TRUE(decompSingleQubitLayers[0][1]->isCompoundOperation()); | ||
| EXPECT_TRUE(decompSingleQubitLayers[0][1]->isGlobal(1)); | ||
| EXPECT_EQ(decompSingleQubitLayers[0][2]->getType(), qc::RZ); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][2]->getTargets(), | ||
| ::testing::ElementsAre(0)); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][2]->getParameter(), | ||
| ::testing::ElementsAre(::testing::DoubleNear( | ||
| qc::PI, NativeGateDecomposer::epsilon))); | ||
| EXPECT_TRUE(decompSingleQubitLayers[0][3]->isCompoundOperation()); | ||
| EXPECT_TRUE(decompSingleQubitLayers[0][3]->isGlobal(1)); | ||
| EXPECT_EQ(decompSingleQubitLayers[0][4]->getType(), qc::RZ); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][4]->getTargets(), | ||
| ::testing::ElementsAre(0)); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][4]->getParameter(), | ||
| ::testing::ElementsAre(::testing::DoubleNear( | ||
| qc::PI_2, NativeGateDecomposer::epsilon))); | ||
| } | ||
|
|
||
| TEST_F(NativeGateDecomposerTest, OneU3OneQubit) { | ||
| // ┌─────────────┐ | ||
| // q: ┤ U3(0,π,π/2) ├ | ||
| // └─────────────┘ | ||
| qc::QuantumComputation qc(1); | ||
| qc.u(0.0, qc::PI, qc::PI_2, 0); | ||
| const auto& [singleQubitLayers, twoQubitLayers] = scheduler.schedule(qc); | ||
| const auto& decompSingleQubitLayers = | ||
| decomposer.decompose(qc.getNqubits(), singleQubitLayers, twoQubitLayers) | ||
| .singleQubitLayers; | ||
| EXPECT_EQ(decompSingleQubitLayers.size(), 1); | ||
| EXPECT_EQ(decompSingleQubitLayers[0].size(), 5); | ||
|
|
||
| EXPECT_EQ(decompSingleQubitLayers[0][0]->getType(), qc::RZ); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][0]->getTargets(), | ||
| ::testing::ElementsAre(0)); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][0]->getParameter(), | ||
| ::testing::ElementsAre( | ||
| ::testing::DoubleNear(0, NativeGateDecomposer::epsilon))); | ||
| EXPECT_TRUE(decompSingleQubitLayers[0][1]->isCompoundOperation()); | ||
| EXPECT_TRUE(decompSingleQubitLayers[0][1]->isGlobal(1)); | ||
| EXPECT_EQ(decompSingleQubitLayers[0][2]->getType(), qc::RZ); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][2]->getTargets(), | ||
| ::testing::ElementsAre(0)); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][2]->getParameter(), | ||
| ::testing::ElementsAre(::testing::DoubleNear( | ||
| qc::PI, NativeGateDecomposer::epsilon))); | ||
| EXPECT_TRUE(decompSingleQubitLayers[0][3]->isCompoundOperation()); | ||
| EXPECT_TRUE(decompSingleQubitLayers[0][3]->isGlobal(1)); | ||
| EXPECT_EQ(decompSingleQubitLayers[0][4]->getType(), qc::RZ); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][4]->getTargets(), | ||
| ::testing::ElementsAre(0)); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][4]->getParameter(), | ||
| ::testing::ElementsAre(::testing::DoubleNear( | ||
| qc::PI_2, NativeGateDecomposer::epsilon))); | ||
| } | ||
|
|
||
| TEST_F(NativeGateDecomposerTest, TwoGatesOneQubit) { | ||
| // ┌───────┐ ┌───────┐ | ||
| // q: ┤ X ├──┤ Z ├ | ||
| // └───────┘ └───────┘ | ||
| qc::QuantumComputation qc(1); | ||
| qc.x(0); | ||
| qc.z(0); | ||
| const auto& [singleQubitLayers, twoQubitLayers] = scheduler.schedule(qc); | ||
| const auto& decompSingleQubitLayers = | ||
| decomposer.decompose(qc.getNqubits(), singleQubitLayers, twoQubitLayers) | ||
| .singleQubitLayers; | ||
|
|
||
| EXPECT_EQ(decompSingleQubitLayers.size(), 1); | ||
| EXPECT_EQ(decompSingleQubitLayers[0].size(), 5); | ||
| EXPECT_EQ(decompSingleQubitLayers[0][0]->getType(), qc::RZ); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][0]->getTargets(), | ||
| ::testing::ElementsAre(0)); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][0]->getParameter(), | ||
| ::testing::ElementsAre(::testing::DoubleNear( | ||
| qc::PI_2, NativeGateDecomposer::epsilon))); | ||
| EXPECT_TRUE(decompSingleQubitLayers[0][1]->isCompoundOperation()); | ||
| EXPECT_TRUE(decompSingleQubitLayers[0][1]->isGlobal(1)); | ||
| EXPECT_EQ(decompSingleQubitLayers[0][2]->getType(), qc::RZ); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][2]->getTargets(), | ||
| ::testing::ElementsAre(0)); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][2]->getParameter(), | ||
| ::testing::ElementsAre(::testing::DoubleNear( | ||
| qc::PI, NativeGateDecomposer::epsilon))); | ||
| EXPECT_TRUE(decompSingleQubitLayers[0][3]->isCompoundOperation()); | ||
| EXPECT_TRUE(decompSingleQubitLayers[0][3]->isGlobal(1)); | ||
| EXPECT_EQ(decompSingleQubitLayers[0][4]->getType(), qc::RZ); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][4]->getTargets(), | ||
| ::testing::ElementsAre(0)); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][4]->getParameter(), | ||
| ::testing::ElementsAre(::testing::DoubleNear( | ||
| 3 * qc::PI_2, NativeGateDecomposer::epsilon))); | ||
| } | ||
|
|
||
| TEST_F(NativeGateDecomposerTest, TwoGatesTwoQubits) { | ||
| // ┌───────┐ | ||
| // q_0: ─┤ X ├─ | ||
| // └───────┘ | ||
| // ┌───────┐ | ||
| // q_1: ─┤ Z ├─ | ||
| // └───────┘ | ||
| qc::QuantumComputation qc(2); | ||
| qc.x(0); | ||
| qc.z(1); | ||
| const auto& [singleQubitLayers, twoQubitLayers] = scheduler.schedule(qc); | ||
| const auto& decompSingleQubitLayers = | ||
| decomposer.decompose(qc.getNqubits(), singleQubitLayers, twoQubitLayers) | ||
| .singleQubitLayers; | ||
| EXPECT_EQ(decompSingleQubitLayers.size(), 1); | ||
| EXPECT_EQ(decompSingleQubitLayers[0].size(), 8); | ||
|
|
||
| EXPECT_EQ(decompSingleQubitLayers[0][0]->getType(), qc::RZ); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][0]->getTargets(), | ||
| ::testing::ElementsAre(0)); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][0]->getParameter(), | ||
| ::testing::ElementsAre(::testing::DoubleNear( | ||
| qc::PI_2, NativeGateDecomposer::epsilon))); | ||
|
|
||
| EXPECT_EQ(decompSingleQubitLayers[0][1]->getType(), qc::RZ); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][1]->getTargets(), | ||
| ::testing::ElementsAre(1)); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][1]->getParameter(), | ||
| ::testing::ElementsAre(::testing::DoubleNear( | ||
| qc::PI_2, NativeGateDecomposer::epsilon))); | ||
|
|
||
| EXPECT_TRUE(decompSingleQubitLayers[0][2]->isCompoundOperation()); | ||
| EXPECT_TRUE(decompSingleQubitLayers[0][2]->isGlobal(2)); | ||
|
|
||
| EXPECT_EQ(decompSingleQubitLayers[0][3]->getType(), qc::RZ); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][3]->getTargets(), | ||
| ::testing::ElementsAre(0)); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][3]->getParameter(), | ||
| ::testing::ElementsAre(::testing::DoubleNear( | ||
| qc::PI, NativeGateDecomposer::epsilon))); | ||
|
|
||
| EXPECT_EQ(decompSingleQubitLayers[0][4]->getType(), qc::RZ); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][4]->getTargets(), | ||
| ::testing::ElementsAre(1)); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][4]->getParameter(), | ||
| ::testing::ElementsAre( | ||
| ::testing::DoubleNear(0, NativeGateDecomposer::epsilon))); | ||
|
|
||
| EXPECT_TRUE(decompSingleQubitLayers[0][5]->isCompoundOperation()); | ||
| EXPECT_TRUE(decompSingleQubitLayers[0][5]->isGlobal(2)); | ||
|
|
||
| EXPECT_EQ(decompSingleQubitLayers[0][6]->getType(), qc::RZ); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][6]->getTargets(), | ||
| ::testing::ElementsAre(0)); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][6]->getParameter(), | ||
| ::testing::ElementsAre(::testing::DoubleNear( | ||
| qc::PI_2, NativeGateDecomposer::epsilon))); | ||
|
|
||
| EXPECT_EQ(decompSingleQubitLayers[0][7]->getType(), qc::RZ); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][7]->getTargets(), | ||
| ::testing::ElementsAre(1)); | ||
| EXPECT_THAT(decompSingleQubitLayers[0][7]->getParameter(), | ||
| ::testing::ElementsAre(::testing::DoubleNear( | ||
| qc::PI_2, NativeGateDecomposer::epsilon))); | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract a helper to reduce duplicated RZ-assertion boilerplate.
OneRXOneQubit, OneU3OneQubit, TwoGatesOneQubit, TwoGatesTwoQubits, and TwoQubitsTwoLayers each repeat the same 4-line pattern (getType()==RZ, getTargets(), getParameter() near-check) many times. A small local helper would cut duplication substantially and make new decomposition tests easier to add.
♻️ Suggested helper
namespace {
void expectRZ(const std::unique_ptr<qc::Operation>& op, qc::Qubit qubit,
qc::fp angle) {
EXPECT_EQ(op->getType(), qc::RZ);
EXPECT_THAT(op->getTargets(), ::testing::ElementsAre(qubit));
EXPECT_THAT(op->getParameter(),
::testing::ElementsAre(
::testing::DoubleNear(angle, NativeGateDecomposer::epsilon)));
}
} // namespaceThen e.g.:
- EXPECT_EQ(decompSingleQubitLayers[0][0]->getType(), qc::RZ);
- EXPECT_THAT(decompSingleQubitLayers[0][0]->getTargets(),
- ::testing::ElementsAre(0));
- EXPECT_THAT(decompSingleQubitLayers[0][0]->getParameter(),
- ::testing::ElementsAre(::testing::DoubleNear(
- qc::PI_2, NativeGateDecomposer::epsilon)));
+ expectRZ(decompSingleQubitLayers[0][0], 0, qc::PI_2);🤖 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 `@test/na/zoned/test_native_gate_decomposer.cpp` around lines 338 - 513, The
decomposition tests repeat the same RZ assertion sequence many times, especially
in NativeGateDecomposerTest cases like OneRXOneQubit, OneU3OneQubit,
TwoGatesOneQubit, and TwoGatesTwoQubits. Extract a small local helper near the
test fixtures, such as an expectRZ-style function that takes an operation,
target qubit, and angle, and centralizes the getType/getTargets/getParameter
checks with NativeGateDecomposer::epsilon. Then replace the repeated inline
assertions throughout those test cases with the helper to reduce boilerplate and
keep the tests easier to extend.
Comment on lines
+144
to
+147
| // Perform Sift | ||
| std::vector<std::size_t> allNodes(graph.size()); | ||
| std::iota(allNodes.begin(), allNodes.end(), 0); | ||
| const auto& subproblem1 = NativeGateDecomposer::sift(graph, allNodes, 3); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether <numeric> is included directly or transitively via project headers
rg -n '`#include`' test/na/zoned/test_theta_opt_scheduler.cpp
rg -n 'include <numeric>' include/na/zoned/decomposer/NativeGateDecomposer.hpp test/na/zoned/matcher.hppRepository: munich-quantum-toolkit/qmap
Length of output: 491
🏁 Script executed:
#!/bin/bash
set -e
printf '--- test/na/zoned/test_theta_opt_scheduler.cpp ---\n'
sed -n '1,40p' test/na/zoned/test_theta_opt_scheduler.cpp
printf '\n--- include/na/zoned/decomposer/NativeGateDecomposer.hpp ---\n'
sed -n '1,80p' include/na/zoned/decomposer/NativeGateDecomposer.hpp
printf '\n--- test/na/zoned/matcher.hpp ---\n'
sed -n '1,120p' test/na/zoned/matcher.hppRepository: munich-quantum-toolkit/qmap
Length of output: 256
🏁 Script executed:
#!/bin/bash
set -e
echo '--- test/na/zoned/test_theta_opt_scheduler.cpp ---'
sed -n '1,50p' test/na/zoned/test_theta_opt_scheduler.cpp
echo
echo '--- include/na/zoned/decomposer/NativeGateDecomposer.hpp ---'
sed -n '1,120p' include/na/zoned/decomposer/NativeGateDecomposer.hpp
echo
echo '--- test/na/zoned/matcher.hpp ---'
sed -n '1,120p' test/na/zoned/matcher.hpp
echo
echo '--- direct <numeric> mentions in these files ---'
rg -n 'include <numeric>|`#include`' test/na/zoned/test_theta_opt_scheduler.cpp include/na/zoned/decomposer/NativeGateDecomposer.hpp test/na/zoned/matcher.hppRepository: munich-quantum-toolkit/qmap
Length of output: 9579
🏁 Script executed:
#!/bin/bash
set -e
echo '--- test/na/zoned/test_theta_opt_scheduler.cpp ---'
sed -n '1,50p' test/na/zoned/test_theta_opt_scheduler.cpp
echo
echo '--- include/na/zoned/decomposer/NativeGateDecomposer.hpp ---'
sed -n '1,140p' include/na/zoned/decomposer/NativeGateDecomposer.hpp
echo
echo '--- test/na/zoned/matcher.hpp ---'
sed -n '1,140p' test/na/zoned/matcher.hpp
echo
echo '--- includes only ---'
rg -n '^`#include`' test/na/zoned/test_theta_opt_scheduler.cpp include/na/zoned/decomposer/NativeGateDecomposer.hpp test/na/zoned/matcher.hppRepository: munich-quantum-toolkit/qmap
Length of output: 10552
🏁 Script executed:
#!/bin/bash
set -e
echo '--- <numeric> mentions in direct test includes ---'
rg -n '`#include` <numeric>|std::iota|`#include`' \
ir/QuantumComputation.hpp \
na/zoned/scheduler/ASAPScheduler.hpp \
include/na/zoned/decomposer/NativeGateDecomposer.hpp \
test/na/zoned/matcher.hpp
echo
echo '--- include trees for the direct headers ---'
for f in ir/QuantumComputation.hpp na/zoned/scheduler/ASAPScheduler.hpp; do
echo "### $f"
sed -n '1,120p' "$f"
echo
doneRepository: munich-quantum-toolkit/qmap
Length of output: 1084
🏁 Script executed:
#!/bin/bash
set -e
fd -a 'QuantumComputation.hpp|ASAPScheduler.hpp|DecomposerBase.hpp|Types.hpp' .Repository: munich-quantum-toolkit/qmap
Length of output: 341
🏁 Script executed:
#!/bin/bash
set -e
for f in \
include/na/zoned/Types.hpp \
include/na/zoned/decomposer/DecomposerBase.hpp \
include/na/zoned/scheduler/ASAPScheduler.hpp
do
echo "### $f"
rg -n '^`#include`|std::iota|<numeric>' "$f"
echo
doneRepository: munich-quantum-toolkit/qmap
Length of output: 763
🏁 Script executed:
#!/bin/bash
set -e
fd -a 'QuantumComputation.hpp' include ir .
echo
echo '--- include/ir/QuantumComputation.hpp ---'
sed -n '1,140p' include/ir/QuantumComputation.hppRepository: munich-quantum-toolkit/qmap
Length of output: 334
🏁 Script executed:
#!/bin/bash
set -e
fd -a 'QuantumComputation.hpp' .Repository: munich-quantum-toolkit/qmap
Length of output: 165
Add <numeric> to the test includes. std::iota is used here, so the test should include <numeric> directly instead of relying on indirect headers.
🤖 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 `@test/na/zoned/test_theta_opt_scheduler.cpp` around lines 144 - 147, The test
uses std::iota in the theta opt scheduler test, but the include list does not
directly provide it. Update the test’s includes in test_theta_opt_scheduler.cpp
to add <numeric> so the use of std::iota is self-contained and not dependent on
indirect headers; the relevant location is the test that calls
NativeGateDecomposer::sift.
Comment on lines
+195
to
+219
| NativeGateDecomposer::DirectedGraph< | ||
| std::pair<std::vector<std::size_t>, std::vector<std::size_t>>> | ||
| subproblemGraph; | ||
| subproblemGraph.addNode({}); | ||
| const auto& subproblem2 = | ||
| NativeGateDecomposer::sift(graph, subproblem1[2], 3); | ||
| const auto& layers = NativeGateDecomposer::getPossibleLayers( | ||
| graph, subproblem1[1], subproblem2, false); | ||
|
|
||
| EXPECT_EQ(layers.size(), 2); | ||
|
|
||
| EXPECT_THAT(layers[0].second, | ||
| ::testing::DoubleNear(qc::PI, NativeGateDecomposer::epsilon)); | ||
| EXPECT_THAT(layers[0].first[0], ::testing::UnorderedElementsAre(0, 1)); | ||
| EXPECT_THAT(layers[0].first[1], ::testing::UnorderedElementsAre(2, 4)); | ||
| EXPECT_THAT(layers[0].first[2], ::testing::UnorderedElementsAre(3, 5, 6)); | ||
| EXPECT_THAT(layers[0].first[3], ::testing::UnorderedElementsAre(7)); | ||
|
|
||
| EXPECT_THAT(layers[1].second, | ||
| ::testing::DoubleNear(qc::PI_4, NativeGateDecomposer::epsilon)); | ||
| EXPECT_THAT(layers[1].first[0], ::testing::UnorderedElementsAre(1)); | ||
| EXPECT_THAT(layers[1].first[1], ::testing::UnorderedElementsAre(2)); | ||
| EXPECT_THAT(layers[1].first[2], ::testing::UnorderedElementsAre(3, 0)); | ||
| EXPECT_THAT(layers[1].first[3], ::testing::UnorderedElementsAre(4, 5, 6, 7)); | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Dead subproblemGraph local variable.
subproblemGraph is constructed and given a single addNode({}) call but never used afterward in NextMomentsPush. The same unused pattern recurs in NextMomentsCond2 (Lines 249-253) and NextMomentsCond3 (Lines 294-298). This looks like leftover copy-paste from BuildSchedule; consider removing it from these three tests unless it was meant to be passed into getPossibleLayers/asserted against.
🤖 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 `@test/na/zoned/test_theta_opt_scheduler.cpp` around lines 195 - 219, Remove
the unused subproblemGraph setup from the affected theta scheduler tests: in
NextMomentsPush, NextMomentsCond2, and NextMomentsCond3, the DirectedGraph
instance is created and addNode({}) is called but the variable is never used.
Keep the test focused on the actual getPossibleLayers/sift assertions, or if the
graph was intended to matter, wire it into those calls and assertions instead of
leaving it as dead setup.
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Sandbox execution failed: failed |
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
4 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 PR adds a real native gate decomposer to the compilation pipeline of the zned neutral atom compiler. It converts single qubit gates into global y-rotations and local z-rotations.
Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.