✨ Add Native Gate Decomposer to Zoned Neutral Atom Compiler#1050
Open
ystade wants to merge 168 commits into
Open
✨ Add Native Gate Decomposer to Zoned Neutral Atom Compiler#1050ystade wants to merge 168 commits into
ystade wants to merge 168 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
![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: 4
🤖 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 `@include/na/zoned/Compiler.hpp`:
- Around line 314-326: Add a Doxygen-style class comment for the public API type
RoutingAwareNativeGateCompiler in Compiler.hpp, matching the documentation style
used for other exposed compiler classes. Place the comment directly above the
class declaration and briefly describe that it is the routing-aware native gate
compiler used for the nanobind/stub-exposed API.
In `@src/na/zoned/decomposer/NativeGateDecomposer.cpp`:
- Around line 677-697: The NativeGateDecomposer::scheduleRemaining logic assumes
args is never empty, but minNode is only assigned inside the loop and then
written into memo[subproblem]. Make this safe by either explicitly initializing
minNode before the loop or adding a defensive assertion/check that args is not
empty before iterating, so the memo entry never reads an indeterminate value.
- Around line 35-38: The NativeGateDecomposer constructor currently assigns
config_ inside the body instead of initializing it directly, which triggers the
static analysis warning. Update NativeGateDecomposer::NativeGateDecomposer to
use a member-initializer list for config_ and keep the constructor body empty if
no other setup is needed; this should be applied in the constructor definition
that takes Architecture and Config.
- Around line 29-34: Add a Doxygen-style comment for the local `overloads`
helper in `NativeGateDecomposer.cpp` so it is documented per C++ guidelines.
Keep the anonymous-namespace placement, and describe that `overloads` is a
variadic helper struct combining multiple lambda call operators via `using
Ts::operator()...;`. Use the `overloads` symbol to locate the spot and ensure
the comment is in Doxygen format.
🪄 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: f0403c19-1de4-43f6-aabb-8ab8b41c68bd
📒 Files selected for processing (19)
bindings/na/register_zoned.cppeval/na/zoned/eval_framework.pyeval/na/zoned/eval_ids_relaxed_routing.pyeval/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.cpptest/python/na/test_zoned.py
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 489-510: The static JSON factory methods return compilers that
store architecture_ by reference, so the Python architecture can be collected
too early. Add nb::keep_alive<0, 1>() to
RoutingAwareNativeGateCompiler::from_json_string and apply the same lifetime tie
to RoutingAgnosticCompiler::from_json_string and
RoutingAwareCompiler::from_json_string so the returned compiler keeps the arch
argument alive.
In `@eval/na/zoned/eval_ids_relaxed_routing.py`:
- Line 14: The dependency pin for qiskit is still commented out, so this script
can resolve an unpinned transitive version and lose reproducibility. In
eval_ids_relaxed_routing.py, uncomment the qiskit==2.4.2 entry in the dependency
list so it is included in resolution, matching the sibling evaluation scripts
and keeping eval_framework’s module-level qiskit import on a fixed version.
In `@include/na/zoned/decomposer/NativeGateDecomposer.hpp`:
- Around line 174-178: The NativeGateDecomposer::decompose override is missing
the required Doxygen-style documentation block, unlike the other public
declarations in this header. Add a full Doxygen comment immediately above
decompose(size_t nQubits, const std::vector<SingleQubitGateRefLayer>&, const
std::vector<TwoQubitGateLayer>&) to describe its purpose, parameters, and return
value, keeping the style consistent with the rest of NativeGateDecomposer.
- Around line 16-28: The NativeGateDecomposer.hpp header is relying on
transitive nlohmann/json includes for Config, which can break the
NLOHMANN_DEFINE_TYPE_INTRUSIVE_WITH_DEFAULT macro usage. Add a direct include
for nlohmann/json.hpp in this header near the other includes so the Config
serialization definition is self-contained and no longer depends on Types.hpp or
DecomposerBase.hpp to provide the JSON header.
- Around line 195-199: The DirectedGraph constructor is assigning adjacencies_
and nodeValues_ in the body instead of initializing them directly. Update
DirectedGraph() in NativeGateDecomposer.hpp to use a member-initializer list for
nNodes_, adjacencies_, and nodeValues_, following the same pattern already used
for config_ elsewhere in the PR.
In `@src/na/zoned/decomposer/NativeGateDecomposer.cpp`:
- Around line 685-698: Rename the structured binding in NativeGateDecomposer’s
loop over args so the second element reflects that it is the qc::fp cost
returned by getPossibleLayers, not a gate collection. Update the local variable
name used in addNodeToSubproblemGraph, scheduleRemaining, and the minWeight
assignment to something cost-related (for example, layerCost) to avoid
misleading future readers and prevent accidental misuse.
In `@test/na/zoned/matcher.hpp`:
- Around line 20-29: Add a brief Doxygen-style comment for the new angleDiff
helper in matcher.hpp so it matches the documentation style used by the matcher
macros below. Place the comment immediately above the angleDiff lambda and
describe its purpose clearly and concisely, keeping it consistent with the
surrounding Doxygen docs and the coding guideline for C++ headers.
🪄 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: 0df383e9-22d4-4e3c-b725-9b41b33f482e
📒 Files selected for processing (19)
bindings/na/register_zoned.cppeval/na/zoned/eval_framework.pyeval/na/zoned/eval_ids_relaxed_routing.pyeval/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.cpptest/python/na/test_zoned.py
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@eval/na/zoned/eval_framework.py`:
- Around line 202-211: The code in eval_framework.py is only stripping the @+ u
header, so partially stripped u blocks can still reach evaluator.evaluate() and
trigger unrecognized operations. Update the code around the code normalization
step to remove the full u block before calling evaluate(), or keep a separate
copy for the .naviz write_text output and pass a fully cleaned version into
evaluator.evaluate(), using the existing code, evaluator.evaluate(), and
write_text flow as the key locations.
In `@src/na/zoned/decomposer/NativeGateDecomposer.cpp`:
- Around line 459-467: The strict-cost path in
NativeGateDecomposer::getPossibleLayers is currently untested because all
existing ThetaOptTest and routing config paths force checkFinalCond false. Add a
test case that sets checkFinalCond true and uses a circuit where
pot.second.second + newVc1Cost is not less than origCombCost so the candidate is
rejected, verifying the Condition 4 branch behaves correctly and remains
covered.
- Around line 536-565: The sift logic in NativeGateDecomposer::scheduleRemaining
is rescanning the entire circuit on every recursive call instead of only the
still-pending nodes. Update the loop in sift() to iterate over remainingNodes
(sorting them once if needed, since convertCircuitToDAG already provides
topological node ids) and remove the repeated vRemaining.contains(node)
full-range scan; keep the existing disjunct/removed/remainingGates behavior
unchanged while reducing the work per call.
In `@test/na/zoned/CMakeLists.txt`:
- Line 10: The SOURCES glob in the test CMakeLists includes headers as well as
sources, which appears to be an intentional IDE-visible grouping rather than a
build requirement; review the target definition that uses file(GLOB SOURCES
*.cpp *.hpp) and either keep the *.hpp entry with a clear intent or remove it if
it was added by copy-paste and not meant to expose matcher.hpp in project trees.
In `@test/na/zoned/test_theta_opt_scheduler.cpp`:
- Around line 590-606: The Complete test currently validates only the number of
decomposed single-qubit layers, so it can miss gate reordering or corruption.
Update the test in ThetaOptTest::Complete to assert the actual gate content and
qubit assignments in decompSingleQubitLayers, using the same style as the other
scheduler tests such as CompleteTestSmall with U3GateNear and ElementsAre, while
keeping the existing layer-count check if desired.
🪄 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: 16db12f7-f572-4e57-a380-1b63b58066db
📒 Files selected for processing (19)
bindings/na/register_zoned.cppeval/na/zoned/eval_framework.pyeval/na/zoned/eval_ids_relaxed_routing.pyeval/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.cpptest/python/na/test_zoned.py
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@include/na/zoned/decomposer/NativeGateDecomposer.hpp`:
- Around line 175-183: Remove the stray backtick in the Doxygen comment for
NativeGateDecomposer’s decomposition method so the `@returns` tag is valid plain
documentation text. Update the comment near the decomposer declaration to read
cleanly without the leftover markdown character, keeping the rest of the
parameter and summary text unchanged.
- Around line 35-47: SubproblemHasher currently flattens all three sub-vectors
into one stream, so different groupings with the same elements can hash the same
and increase memo-table collisions. Update SubproblemHasher::operator() to mix
in each sub-vector’s identity, such as its index and/or size before hashing its
elements, so the hash reflects both contents and boundaries. Keep the existing
element hashing, but ensure the unordered_map used by scheduleRemaining gets a
better-distributed key for distinct subproblems.
In `@include/na/zoned/decomposer/NoOpDecomposer.hpp`:
- Around line 51-55: The NoOpDecomposer::decompose declaration matches the base
signature, but it is missing a Doxygen comment like the other public API members
in this header. Add a Doxygen-style comment directly above the decompose method
in NoOpDecomposer.hpp describing its purpose and parameters, keeping the style
consistent with the class documentation and other public declarations.
In `@test/na/zoned/test_native_gate_decomposer.cpp`:
- Around line 54-226: Refactor the repetitive NativeGateDecomposerTest
translation cases into a table-driven parameterized test. The current TEST_F
cases around NativeGateDecomposer::convertGateToQuaternion only vary by
qc::StandardOperation setup and expected NativeGateDecomposer::Quaternion, so
replace them with a TEST_P using a list of gate factory inputs and expected
values. Keep the existing coverage for special cases like U/U2/Vdg/H while
centralizing the shared EXPECT_THAT logic in one test body.
🪄 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: e05bae8d-db55-4f3d-a504-3cdb3ca86185
📒 Files selected for processing (19)
bindings/na/register_zoned.cppeval/na/zoned/eval_framework.pyeval/na/zoned/eval_ids_relaxed_routing.pyeval/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.cpptest/python/na/test_zoned.py
Collaborator
Author
|
@coderabbitai Do a full review. While doing that, resolve all open comments that are resolved. If you should find an unresolved comment, explain why it is not resolved in the corresponding thread of the comment. |
Contributor
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh api graphql -f query='
query {
repository(owner: "munich-quantum-toolkit", name: "qmap") {
pullRequest(number: 1050) {
reviewThreads(first: 100) {
nodes {
id
isResolved
isOutdated
path
line
comments(first: 10) {
nodes {
id
databaseId
author { login }
body
url
}
}
}
}
}
}
}
' --jq '.data.repository.pullRequest.reviewThreads.nodes[] | {id, isResolved, isOutdated, path, line, comments: [.comments.nodes[] | {author: .author.login, url}]}'Length of output: 16744 🏁 Script executed: #!/bin/bash
set -euo pipefail
for id in PRRT_kwDOEgwI3c6O5yHh PRRT_kwDOEgwI3c6O-fNL PRRT_kwDOEgwI3c6O-fNa PRRT_kwDOEgwI3c6O-fNh PRRT_kwDOEgwI3c6O-fNo; do
echo "=== Thread $id ==="
gh api graphql -f query='
query($id: ID!) {
node(id: $id) {
... on PullRequestReviewThread {
isResolved
isOutdated
path
line
comments(first: 10) {
nodes {
databaseId
author { login }
body
url
}
}
}
}
}' -f id="$id" --jq '.data.node'
echo
doneLength of output: 18538 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== eval_framework.py header/def process_benchmark ==="
grep -n "def process_benchmark" -A15 eval/na/zoned/eval_framework.py
echo
echo "=== eval_framework.py around @+ u handling ==="
grep -n "@+ u\|write_text(\|evaluate(" eval/na/zoned/eval_framework.py
echo
echo "=== NativeGateDecomposer.hpp SubproblemHasher ==="
grep -n "SubproblemHasher" -A15 include/na/zoned/decomposer/NativeGateDecomposer.hpp
echo
echo "=== NativeGateDecomposer.hpp around line 184 (Doxygen) ==="
sed -n '165,195p' include/na/zoned/decomposer/NativeGateDecomposer.hpp
echo
echo "=== NoOpDecomposer.hpp around line 63 ==="
sed -n '1,70p' include/na/zoned/decomposer/NoOpDecomposer.hppLength of output: 6847 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '185,220p' eval/na/zoned/eval_framework.py
echo "---"
sed -n '590,620p' eval/na/zoned/eval_framework.pyLength of output: 3025 🏁 Script executed: #!/bin/bash
set -euo pipefail
grep -n "_process_u\b" -A25 eval/na/zoned/eval_framework.py | head -60Length of output: 2805 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Resolve thread: eval_framework.py drop_u_gates (line 173) ==="
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOEgwI3c6O-fNL"}) {
thread { id isResolved }
}
}'
echo "=== Resolve thread: NativeGateDecomposer.hpp SubproblemHasher (line 48) ==="
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOEgwI3c6O-fNa"}) {
thread { id isResolved }
}
}'
echo "=== Resolve thread: NativeGateDecomposer.hpp stray backtick (line 184) ==="
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOEgwI3c6O-fNh"}) {
thread { id isResolved }
}
}'
echo "=== Resolve thread: NoOpDecomposer.hpp missing Doxygen (line 63) ==="
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOEgwI3c6O-fNo"}) {
thread { id isResolved }
}
}'
echo "=== Reply on still-open thread: eval_framework.py partially stripped u blocks ==="
gh api -X POST repos/munich-quantum-toolkit/qmap/pulls/1050/comments/3536261142/replies \
-f body="This concern is still valid and remains unresolved: \`process_benchmark\` (in \`eval/na/zoned/eval_framework.py\`) only strips the \`@+ u ...\` header line via \`line.startswith(\"@+ u\")\`. For multi-line \`u\` blocks (header ending in \`[\`, followed by one atom name per line, closed by a standalone \`]\`), the header is removed but the atom-name lines and the closing \`]\` remain in \`code\`. Since \`evaluator.evaluate(...)\` is called with that same stripped \`code\` right after, those leftover lines don't match any of the \`elif line.startswith(...)\` branches in \`Evaluator.evaluate\` and will raise \`Unrecognized operation: ...\`. Keeping this open until the multi-line block is fully removed before parsing (or a separately-cleaned copy is used for \`evaluate()\`)."Length of output: 13827 All four fixed threads have been resolved, and I replied on the one that's still open explaining why it stays open. Now triggering the full review as requested.
Resolved (already fixed in code, verified against current source):
Left open, with explanation posted in-thread:
All other previously-flagged threads were already marked resolved on GitHub, so no further action was needed there. Now kicking off the full review. ✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@UPGRADING.md`:
- Around line 9-17: The script reference in the NativeGateDecomposer usage note
is incorrect and points to a non-existent path. Update the example path under
the RoutingAwareNativeGateCompiler/NativeGateDecomposer description to match the
actual script name, fixing both typos so the reference uses
eval/na/zoned/eval_native_gate_decomposition.py instead of the broken
zined/decomposer variant.
🪄 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: 05edbd0a-8a34-49db-b36c-feb566e0a592
📒 Files selected for processing (22)
CHANGELOG.mdUPGRADING.mdbindings/na/register_zoned.cppcmake/ExternalDependencies.cmakeeval/na/zoned/eval_framework.pyeval/na/zoned/eval_ids_relaxed_routing.pyeval/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.cpptest/python/na/test_zoned.py
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.
This PR was assited by GitHub Copilot powered by Claude Haiku 4.5 and GPT-5 mini.
Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.