🐛 Fix native gates handling for mirror circuits #709
Conversation
- Added unit test to verify that `_create_mirror_circuit` correctly respects the provided Qiskit `Target` and preserves native gate operations. - Added TODO note to investigate mismatch in Statevector equivalence.
…x/mirror-native-gates # Conflicts: # tests/test_bench.py
…mark_generation`
…mark_generation`
…x/mirror-native-gates
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe Changes
Sequence DiagramsequenceDiagram
participant Caller as Caller
participant CreateMirror as _create_mirror_circuit
participant Compose as Compose
participant Transpile as Transpile
participant RestoreLayout as Restore Layout
Caller->>CreateMirror: call with target, optimization_level
CreateMirror->>Compose: invert gates & compose
Compose-->>CreateMirror: mirrored circuit
alt target is not None
CreateMirror->>Transpile: transpile to native gate set
Transpile-->>CreateMirror: transpiled circuit
CreateMirror->>RestoreLayout: restore initial_layout
RestoreLayout-->>CreateMirror: circuit with preserved layout
else target is None
CreateMirror-->>CreateMirror: use circuit as-is
end
CreateMirror->>CreateMirror: add measurements & finalize
CreateMirror-->>Caller: mirror circuit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes introduce new conditional logic with transpilation and layout handling across multiple files, requiring careful verification of gate compliance logic and test coverage. The scope is well-contained to a single function's enhancement, but the interplay between transpilation, layout preservation, and backward compatibility warrants thorough review. Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (3)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mqt/bench/benchmark_generation.py (1)
321-321: Passtargetparameter to_create_mirror_circuitin native gates and mapped contexts.The new
targetparameter enables transpilation of mirror circuits to native gate sets, but it's not being passed at these call sites. This means mirror circuits generated at theNATIVEGATESandMAPPEDlevels won't enforce native gate constraints, which appears to contradict the PR objective.Apply this diff to fix line 321:
compiled_circuit = pm.run(circuit) if generate_mirror_circuit: - return _create_mirror_circuit(compiled_circuit, inplace=True) + return _create_mirror_circuit(compiled_circuit, inplace=True, target=target, optimization_level=opt_level) return compiled_circuitApply this diff to fix line 387:
) if generate_mirror_circuit: - return _create_mirror_circuit(mapped_circuit, inplace=True) + return _create_mirror_circuit(mapped_circuit, inplace=True, target=target, optimization_level=opt_level) return mapped_circuitAlso applies to: 387-387
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/mqt/bench/benchmark_generation.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.14.0)
src/mqt/bench/benchmark_generation.py
79-79: Boolean-typed positional argument in function definition
(FBT001)
79-79: Boolean default positional argument in function definition
(FBT002)
🔇 Additional comments (1)
src/mqt/bench/benchmark_generation.py (1)
78-80: LGTM! Function signature extended appropriately.The addition of
targetandoptimization_levelparameters enables optional transpilation of mirror circuits to native gate sets, which aligns with the PR objective.
📝 WalkthroughWalkthroughAdds optional target and optimization_level parameters to _create_mirror_circuit. When target is provided, the function transpiles the mirrored circuit to the target’s native gate set using the specified optimization level; otherwise it returns the untranspiled mirror. Inplace behavior and mirror construction remain unchanged. Docstring updated. Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant BG as BenchmarkGeneration::_create_mirror_circuit
participant T as Transpiler
Caller->>BG: _create_mirror_circuit(circ, target=None|Target, optimization_level)
activate BG
BG->>BG: Construct mirrored circuit (inplace respected)
alt target provided
BG->>T: Transpile mirrored circuit to target native set<br/>with optimization_level
T-->>BG: Transpiled circuit
BG-->>Caller: Return transpiled mirror
else no target
BG-->>Caller: Return untranspiled mirror
end
deactivate BG
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/mqt/bench/benchmark_generation.py (2)
320-321: Critical: Pass target and opt_level to mirror circuit generation.The call to
_create_mirror_circuitdoesn't pass thetargetandopt_levelparameters, so the mirror circuit won't be transpiled to the native gate set. This defeats the purpose of this PR fix.Apply this diff:
compiled_circuit = pm.run(circuit) if generate_mirror_circuit: - return _create_mirror_circuit(compiled_circuit, inplace=True) + return _create_mirror_circuit(compiled_circuit, inplace=True, target=target, optimization_level=opt_level) return compiled_circuit
386-387: Critical: Pass target and opt_level to mirror circuit generation.The call to
_create_mirror_circuitdoesn't pass thetargetandopt_levelparameters, so the mirror circuit won't be transpiled to the target's native gate set. This defeats the purpose of this PR fix.Apply this diff:
) if generate_mirror_circuit: - return _create_mirror_circuit(mapped_circuit, inplace=True) + return _create_mirror_circuit(mapped_circuit, inplace=True, target=target, optimization_level=opt_level) return mapped_circuit
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/mqt/bench/benchmark_generation.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.14.0)
src/mqt/bench/benchmark_generation.py
79-79: Boolean-typed positional argument in function definition
(FBT001)
79-79: Boolean default positional argument in function definition
(FBT002)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/mqt/bench/benchmark_generation.py (1)
97-97: Correct parameter description.The
optimization_levelparameter controls the transpiler's optimization passes, not a device property.Apply this diff:
- optimization_level: Optimization level of the device. + optimization_level: Optimization level for the transpilation pass (0-3).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/mqt/bench/benchmark_generation.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.14.0)
src/mqt/bench/benchmark_generation.py
79-79: Boolean-typed positional argument in function definition
(FBT001)
79-79: Boolean default positional argument in function definition
(FBT002)
🔇 Additional comments (4)
src/mqt/bench/benchmark_generation.py (4)
78-80: LGTM!The function signature correctly adds
targetandoptimization_levelparameters to enable native gate set compliance for mirror circuits. The defaultoptimization_level=2aligns with the defaults used in calling functions, ensuring consistency.
116-118: LGTM! Core fix correctly addresses issue #654.The conditional transpilation correctly ensures that the mirrored circuit respects the target's native gate set. Using
routing_method=Noneis appropriate here—it decomposes non-native gates (e.g.,sxdg) to native equivalents without altering the qubit mapping. This fixes the bug where inverse operations in mirror circuits could violate the native gate set.
321-321: LGTM! Correctly enables native gate set compliance.The updated call now passes
targetandoptimization_levelto_create_mirror_circuit, ensuring that mirror circuits generated at the native-gates level respect the device's native gate set.
387-387: LGTM! Correctly enables native gate set compliance.The updated call now passes
targetandoptimization_levelto_create_mirror_circuit, ensuring that mirror circuits generated at the mapped level respect the device's native gate set.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/mqt/bench/benchmark_generation.py (1)
97-97: Fix parameter description.The description incorrectly states "Optimization level of the device." The
optimization_levelparameter controls the transpiler's optimization passes, not a device property.Apply this diff:
- optimization_level: Optimization level of the device. + optimization_level: Optimization level for the transpilation pass (0-3).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/mqt/bench/benchmark_generation.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.14.0)
src/mqt/bench/benchmark_generation.py
79-79: Boolean-typed positional argument in function definition
(FBT001)
79-79: Boolean default positional argument in function definition
(FBT002)
🔇 Additional comments (4)
src/mqt/bench/benchmark_generation.py (4)
78-80: LGTM! Function signature correctly extended.The new
targetandoptimization_levelparameters enable target-aware transpilation of mirror circuits. The defaultoptimization_level=2correctly aligns with calling functions.Note: Static analysis flags the boolean
inplacepositional argument (FBT001, FBT002), but this is acceptable for a private helper function.
88-89: LGTM! Docstring accurately describes native gate compliance.The description correctly explains that the mirrored circuit respects the target's native gate set when a target is provided.
321-321: LGTM! Caller correctly passes target parameters.The call correctly forwards
targetandoptimization_levelto enable native gate compliance for mirror circuits. This addresses the core issue from #654 where mirror circuits ignored the native gate set.
387-387: LGTM! Caller correctly passes target parameters.The call correctly forwards
targetandoptimization_level, ensuring mirror circuits respect the native gate set at the mapped benchmark level as well.
|
@burgholzer The test fails on the following assertion: assert qc_mirror.layout.initial_layout == qc_base.layout.initial_layoutThe error shows that the base circuit’s layout contains both logical and ancilla qubits (e.g., q[0..2] and ancilla[0..23]), while the mirrored circuit’s layout only contains the device qubits (q[0..26]). It seems that when the mirrored circuit is transpiled again (with target specified), the original TranspileLayout object — including the initial_layout — is not preserved and gets replaced by a new layout corresponding to the physical device mapping. I’d appreciate any suggestions on the correct or recommended way to preserve the original circuit layout through the mirror-generation and transpilation steps. |
Signed-off-by: burgholzer <[email protected]>
|
I just pushed a commit that should hopefully fix everything and preserve the layout. This looks pretty good by now. |
Sure! I’ll add the changelog entry and some extra assertions to ensure the mirrored circuit only contains native gates. Thanks for the feedback! |
This PR contains the following updates: | Update | Change | |---|---| | lockFileMaintenance | All locks refreshed | 🔧 This Pull Request updates lock files to use the latest dependency versions. --- ### Configuration 📅 **Schedule**: Branch creation - "before 4am on monday" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://redirect.github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR was generated by [Mend Renovate](https://mend.io/renovate/). View the [repository job log](https://developer.mend.io/github/munich-quantum-toolkit/bench). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MS4xNDMuMSIsInVwZGF0ZWRJblZlciI6IjQxLjE0My4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJkZXBlbmRlbmNpZXMiLCJweXRob24iXX0=--> Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…o v2025.10.11 (munich-quantum-toolkit#710) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [henryiii/validate-pyproject-schema-store](https://redirect.github.com/henryiii/validate-pyproject-schema-store) | repository | patch | `2025.10.03` -> `2025.10.11` | Note: The `pre-commit` manager in Renovate is not supported by the `pre-commit` maintainers or community. Please do not report any problems there, instead [create a Discussion in the Renovate repository](https://redirect.github.com/renovatebot/renovate/discussions/new) if you have any questions. --- ### Release Notes <details> <summary>henryiii/validate-pyproject-schema-store (henryiii/validate-pyproject-schema-store)</summary> ### [`v2025.10.11`](https://redirect.github.com/henryiii/validate-pyproject-schema-store/compare/2025.10.03...2025.10.11) [Compare Source](https://redirect.github.com/henryiii/validate-pyproject-schema-store/compare/2025.10.03...2025.10.11) </details> --- ### Configuration 📅 **Schedule**: Branch creation - "every weekend" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR was generated by [Mend Renovate](https://mend.io/renovate/). View the [repository job log](https://developer.mend.io/github/munich-quantum-toolkit/bench). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MS4xNDMuMSIsInVwZGF0ZWRJblZlciI6IjQxLjE0My4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJkZXBlbmRlbmNpZXMiLCJwcmUtY29tbWl0Il19--> Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mqt/bench/benchmark_generation.py (1)
130-134: Possible classical register name collision (“meas”).If the original circuit already had a creg named "meas" (removed measures don’t remove regs),
add_registerwill raise. Use a unique name.- new_creg = ClassicalRegister(len(active_qubits), "meas") - target_qc.add_register(new_creg) + # Use a unique name to avoid clashes with existing classical registers. + creg_name = "meas_mirror" + if any(creg.name == creg_name for creg in target_qc.cregs): + creg_name = f"{creg_name}_{len(target_qc.cregs)}" + new_creg = ClassicalRegister(len(active_qubits), creg_name) + target_qc.add_register(new_creg) target_qc.measure(active_qubits, new_creg)
♻️ Duplicate comments (1)
src/mqt/bench/benchmark_generation.py (1)
96-97: Docstring: fixoptimization_levelwording.It’s a transpilation setting, not a device property.
- target: Target device for transpilation. If provided, ensures native gate set compliance. - optimization_level: Optimization level of the device. + target: Target object specifying native gate set and device properties; if provided, ensures native gate set compliance. + optimization_level: Optimization level for the transpilation passes (0–3).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/mqt/bench/benchmark_generation.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.14.0)
src/mqt/bench/benchmark_generation.py
79-79: Boolean-typed positional argument in function definition
(FBT001)
79-79: Boolean default positional argument in function definition
(FBT002)
🔇 Additional comments (1)
src/mqt/bench/benchmark_generation.py (1)
331-331: Wiring looks correct; please add a native‑gate assertion in tests.Passing
targetandopt_levelthrough to the mirror ensures native gate compliance. Add an assertion that all non‑measure ops in the mirror are intarget.operation_names.Example test snippet:
gate_names = {inst.operation.name for inst in qc_mirror.data if inst.operation.name != "measure"} assert gate_names <= set(target.operation_names)Also applies to: 397-397
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
.pre-commit-config.yaml(1 hunks)CHANGELOG.md(1 hunks)
🔇 Additional comments (1)
.pre-commit-config.yaml (1)
132-132: Pre-commit hook update should be separated or its inclusion clarified.The validate-pyproject-schema-store version bump to 2025.10.11 is confirmed in a separate commit (962e6f1) with its own update message, distinct from the mirror circuit fix commits. While the update itself is benign, it represents maintenance work logically separate from the PR's core objective. Verify whether this bundling aligns with your contribution guidelines, or consider moving it to its own commit/PR.
Ensure mirror circuits only contain gates native to the target device, allowing barriers and measurements as exceptions. This validation helps maintain hardware compatibility for benchmark circuits.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
.pre-commit-config.yaml(1 hunks)CHANGELOG.md(1 hunks)
🔇 Additional comments (2)
.pre-commit-config.yaml (1)
132-132: Routine pre-commit hook version update.The pre-commit hook for validating pyproject schemas is bumped to a recent October 2025 release, which is consistent with the
autoupdate_schedule: quarterlysetting.CHANGELOG.md (1)
16-18: CHANGELOG entry content is clear and well-formatted.The entry accurately describes the fix and follows the project's changelog conventions with appropriate emoji, issue reference, and contributor attribution.
I have added mentioned changes. |
Signed-off-by: burgholzer <[email protected]>
Signed-off-by: burgholzer <[email protected]>
Signed-off-by: burgholzer <[email protected]>
Signed-off-by: burgholzer <[email protected]>
burgholzer
changed the title
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Just pushed a couple of fixes and clean ups.
This should be good to go now! 🚀
Many thanks for the contribution! Much appreciated 🙏🏼
burgholzer
merged commit b31cac4
into
munich-quantum-toolkit:main
2 participants
Description
Fixes #654
Checklist:
Summary by CodeRabbit
Bug Fixes
New Features