✨ Add error correction module with Shor and Steane logical circuit transpilers#930
Conversation
small formatting changes
…de' into feature/ErrorCorrection_Verification
+ Added logging functionality
Added Input sanitation for enoding keywords
…nic0909/MQT_Bench_ErrorCorrectionCodes into feature/ErrorCorrection_MergePrep
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
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)
540-575: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject unsupported
encodingusage instead of silently dropping it.
get_benchmarkacceptsencoding, but only theBenchmarkLevel.ALGbranch forwards it. Calls likeget_benchmark(..., level=BenchmarkLevel.INDEP, encoding="shor")return an unencoded circuit without warning.Proposed fix
Returns: Qiskit::QuantumCircuit object representing the benchmark with the selected options """ + if encoding and level is not BenchmarkLevel.ALG: + msg = "`encoding` is only supported for algorithm-level benchmarks (level=BenchmarkLevel.ALG)." + raise ValueError(msg) + if level is BenchmarkLevel.ALG: return get_benchmark_alg(Also applies to: 579-617
🤖 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/mqt/bench/benchmark_generation.py` around lines 540 - 575, get_benchmark currently accepts encoding but only passes it through in the BenchmarkLevel.ALG path, so unsupported levels silently ignore it. Update get_benchmark to validate encoding against the selected level (and/or the downstream helpers like get_benchmark_alg and the other level-specific branches) and raise a clear error when encoding is provided where it is not supported, instead of returning an unencoded circuit.
169-176: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
encodingto allget_benchmark_algoverloads. The implementation acceptsencoding, but the overloads atsrc/mqt/bench/benchmark_generation.py:169-198omit it, so type checkers and IDEs can reject valid calls likeget_benchmark_alg(..., encoding="steane").🤖 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/mqt/bench/benchmark_generation.py` around lines 169 - 176, Add the missing encoding parameter to every get_benchmark_alg overload in benchmark_generation.py so the signatures match the implementation. Update the overloads around get_benchmark_alg and any related configuration typing to include encoding as an accepted keyword argument, ensuring valid calls like get_benchmark_alg(..., encoding="steane") are recognized by type checkers and IDEs.
🤖 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 `@src/mqt/bench/components/shor_circuit_components.py`:
- Around line 131-135: The docstring sections for the Shor circuit component
functions currently use plain “Arguments:” blocks instead of Google-style
“Args:” headers. Update both affected docstrings in shor_circuit_components.py,
including the one describing qc, logical_qubit, bit_flip_syndrome, and
bit_flip_syndrome_measurement, to use the standard Google-style Args: section
formatting consistently with the project’s Python docstring guidelines.
In `@src/mqt/bench/components/steane_circuit_components.py`:
- Around line 121-127: The docstring parameter section in the component function
currently uses a non-standard header, so update the relevant docstring in
steane_circuit_components.py to follow Google-style formatting by renaming the
parameter section from “Arguments:” to “Args:” and keeping the existing
parameter descriptions aligned under that section. Make sure the affected
function docstring remains consistent with the project’s Python docstring
convention for the symbols describing qc, logical_qubit, bit_flip_syndrome,
phase_flip_syndrome, bit_flip_syndrome_measurement, and
phase_flip_syndrome_measurement.
In `@src/mqt/bench/error_correction/shor_transpiler.py`:
- Around line 246-260: The _logical_measure() path currently decodes the logical
block and leaves it decoded, so later operations on the same logical qubit can
be applied to unencoded physical qubits. Update _logical_measure() and the
surrounding Shor transpiler flow to either reject measurements unless they are
terminal on that logical qubit, or re-encode the block immediately after
measurement if further gates are allowed. Use the existing
_apply_shor_decoding(), logical_qubits, and transpiled_qc measurement handling
to locate and enforce this behavior.
- Around line 99-100: The transpilation flow in `ShorTranspiler.transpile()` is
carrying state across runs because `encode_qubits()` appends to
`self.logical_qubits` and `self.physical_data_registers` without resetting them
first. Before calling `encode_qubits()` and `replace_gates()`, clear all per-run
collections and any handler state tied to the previous `self.transpiled_qc` so a
second `transpile()` on the same instance builds only fresh registers for the
new circuit. Make sure the reset is applied in the main `transpile()` path and
any related setup code referenced by the same block.
In `@src/mqt/bench/error_correction/steane_transpiler.py`:
- Around line 67-68: Reset the transpiler’s per-run state before starting a new
`transpile()` pass: `SteaneTranspiler.encode_qubits()` appends to persistent
register lists, so make sure `transpile()` clears or reinitializes the stateful
register collections and any related cached circuit data before calling
`encode_qubits()` and `replace_gates()`. Update the `SteaneTranspiler` flow so
each invocation works only with the freshly built `self.transpiled_qc`, and
ensure later handlers do not read stale registers left over from a previous run.
- Around line 184-207: The _handle_measure() logic in SteaneTranspiler currently
decodes a logical block in place, so later operations on the same logical qubit
will act on a decoded state. Update the measurement handling to either reject
non-terminal measurements up front or restore the encoded block before any
subsequent logical gate is processed, and make sure the fix is applied
consistently within SteaneTranspiler._handle_measure and the related
gate-handling flow that assumes physical_data_registers stay encoded.
In `@tests/test_error_correction.py`:
- Around line 37-44: The new Python test helpers and functions still use
narrative-only docstrings, so update the remaining docstrings to Google style
with explicit Args/Returns sections where applicable. Focus on the affected test
functions in test_error_correction.py, including
test_errorcorrection_transpiler_gate_equivalence and the other indicated
helpers, and rewrite their docstrings to match the repository’s Python docstring
convention.
---
Outside diff comments:
In `@src/mqt/bench/benchmark_generation.py`:
- Around line 540-575: get_benchmark currently accepts encoding but only passes
it through in the BenchmarkLevel.ALG path, so unsupported levels silently ignore
it. Update get_benchmark to validate encoding against the selected level (and/or
the downstream helpers like get_benchmark_alg and the other level-specific
branches) and raise a clear error when encoding is provided where it is not
supported, instead of returning an unencoded circuit.
- Around line 169-176: Add the missing encoding parameter to every
get_benchmark_alg overload in benchmark_generation.py so the signatures match
the implementation. Update the overloads around get_benchmark_alg and any
related configuration typing to include encoding as an accepted keyword
argument, ensuring valid calls like get_benchmark_alg(..., encoding="steane")
are recognized by type checkers and IDEs.
🪄 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: a5919708-1d38-4fb8-9149-f8df31a0109a
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.gitignorepyproject.tomlsrc/mqt/bench/benchmark_generation.pysrc/mqt/bench/benchmarks/seven_qubit_steane_code.pysrc/mqt/bench/benchmarks/shors_nine_qubit_code.pysrc/mqt/bench/components/shor_circuit_components.pysrc/mqt/bench/components/steane_circuit_components.pysrc/mqt/bench/error_correction/shor_transpiler.pysrc/mqt/bench/error_correction/steane_transpiler.pytests/gate_counts.jsontests/test_error_correction.py
…nic0909/MQT_Bench_ErrorCorrectionCodes into feature/ErrorCorrection_MergePrep
…m/emilkanic0909/MQT_Bench_ErrorCorrectionCodes into feature/ErrorCorrection_MergePrep # Conflicts: # src/mqt/bench/error_correction/steane_transpiler.py
…nic0909/MQT_Bench_ErrorCorrectionCodes into feature/ErrorCorrection_MergePrep
flowerthrower
left a comment
flowerthrower
left a comment
There was a problem hiding this comment.
Dear all, thank you for this PR. We are already on a good path. I left a few comments in the Steane transpiler, which you might want to focus on first. Once this is properly implemented we can apply the same structure to the Shor transpiler. Overall comment, your current implementation is a bit overkill in some places, lets make this as lean as possible.
| def _handle_measure(self, instruction: CircuitInstruction) -> None: | ||
| """Handle measure instruction. | ||
|
|
||
| measure_all() is unsupported operation |
There was a problem hiding this comment.
This is not a gate in the first place.
There was a problem hiding this comment.
But to execute it, we need to decode and other logic to move between logical measurement to physical, since measurement is also a part of error correction code. Therefore we have though that we need to transpile everything, so that we should not make additionally case distinctions and everything is consistent.
Also it builts itself upon existing code very good and similar to gates. Therefore we think we could maybe not call it as "replace gate" but say e.g. "replace instruction", since gates are also CircuitInstruction, same as barrier or measurement.
There was a problem hiding this comment.
Not sure if I understand your answer correctly, but my comment was about line 187, i.e., the measure_all() comment.
| ) | ||
| self.transpiled_qc.barrier(label="Encoding") | ||
|
|
||
| def decode_qubits(self) -> None: |
There was a problem hiding this comment.
why do we need this?
There was a problem hiding this comment.
In first place, we rely on it to traverse circuit for adding noise for testing. Also it is used, so that we could see by mpl drawings, whether it looks correct and could differentiate between different components. We thought it made sense to have, because without barriers, gates are often dragged to the left, and then it is hard to differentiate parts. We could rename it, or remove, but we would need to rethink tests then
There was a problem hiding this comment.
The comment is on l.111. I guess we don't need the decode_qubits after having changed the tests (#930 (comment))
There was a problem hiding this comment.
Some of the generated algorithms inlcude measurements (e.g. the Bernstein-Vazirani circuit, I believe). Without the decoding step the resulting circuit would no longer faithfully match the original circuit.
Should we remove the decoding regardless?
There was a problem hiding this comment.
@Felix-Gundlach he means our function that decodes all qubits for tests. We decode it in also by logical measurement. We have discussed it, that it was not very architecturally clean to have a function that are used only for tests. Thefore we could delete it when we delete corresponding tests
| for instruction in self.original_qc.data: | ||
| gate_name = instruction.operation.name | ||
|
|
||
| if gate_name == "qft": |
There was a problem hiding this comment.
Ideally this initial decomposition into Clifford+T should be done for any circuit, not only "qft".
There was a problem hiding this comment.
I already implemented it on shor code it's global first then transpiles clifford and T afterwards for all
There was a problem hiding this comment.
@flowerthrower We will try to implement it, so we are able then not to differentiate between "clifford circuits" and QFTGate(), also we will try to use same transpilation logic as you told in next comment, when you confirm our proposals.
Shor and Steane, will be both fixed, since they both apply old transpilation scheme .
| tmp = QuantumCircuit(len(instruction.qubits)) | ||
| tmp.append(instruction.operation, range(len(instruction.qubits))) | ||
|
|
||
| tmp = transpile( |
There was a problem hiding this comment.
instead of fullblown tranpsile() on individual gates let's go with a simple circuit wide clifford+t synthesis
you may have a look at how we compile cirucits for "clifford+t" targets already in this repository
There was a problem hiding this comment.
I already implemented it on shor code
There was a problem hiding this comment.
@flowerthrower We have seen similar code in "get_benchmark_native_gates", there was also a part with transpiling into clifford + t + rotations. And PassManager removes rotations then, when we understood correctly?
But we have already implemented our transpilation coroutine and found your implementation too late, therefore we stayed on it so that we would not mess up. And wanted to hear your feedback.
Hence, we could use then your transpilation logic. That was also one of the questions for you, whether we should then make this transpilation from "native gate benchmarks" using transpiler and SolovayKitaev passmanager as a reusable component as it was done by Shors and Steane components?
@SalehAlsherif It is another transpiler (you have moved old one level up), not what currently in Shor or Steane, I think it would be better to wait until clarification regarding transpilation logic and then we will fix it
There was a problem hiding this comment.
the overall replace sequence would look sth like this (your Shor implementation is already close)
- transpile global circuit to Clifford+T:
- Steane: transpile to
["id", "x", "y", "z", "h", "s", "sdg", "cx", "sx", "sxdg", "cy", "cz", "swap", "dcx", "t", "tdg"] - Shor: transpile to
["id", "x", "y", "z", "cx", "swap", "dcx", "t", "tdg"]
- iterate over all gates and
- Steane: replace transversal gates
["id", "x", "y", "z", "h", "s", "sdg", "cx"]directly, and derived transversal gates["sx", "sxdg", "cy", "cz", "swap", "dcx"] - Shor: replace transversal gates
["id", "x", "y", "z", "cx"]directly, and derived transversal gates["swap", "dcx"]
- when encountering a
tgate, substitutetgates with an opaque ideal logical gadget andtdggates with a dagger version of it
| if self.add_syndromes: | ||
| self.insert_syndromes(logical_qubit_index) | ||
|
|
||
| def _handle_t(self, instruction: CircuitInstruction) -> None: |
There was a problem hiding this comment.
you may substitute T gates with an opaque ideal logical gadget (and a dagger version of it), like
Instruction(
name="steane_logical_t_magic_state_injection",
num_qubits=7,
num_clbits=0,
params=[],
)
There was a problem hiding this comment.
@flowerthrower But in a project description it was stated how to prepare state for T using magical state preparations, therefore we have made it in this way, and also tested its functionality.
But now we should simple replace it with this DummyGate? I mean this "Instruction()" has no math info in it, and therefore not executable or simulatable, when I understood it correctly. It sounds unnicely for me to use it and also by taking into account your comment about "enable compiler engineers to get an understanding of what such circuits might look like in the future. For this purpose, circuit structure checks are enough.", so possibly it could be usefull to stay by "cheat" from project description with T gates, since it is very illustrative use case of the most difficult part of error correction and correspondingly to give an overview for future compiler engineers, how this thing are worked around. We could maybe simplify Shor's realisation, but Steane looks not very messy.
There was a problem hiding this comment.
good argument, let's keep it for the Stean since the correction is traversal
|
|
||
| self.transpiled_qc.barrier(label=f"Measurement {logical_qubit_index}") | ||
|
|
||
| def _handle_h(self, instruction: CircuitInstruction) -> None: |
There was a problem hiding this comment.
there is a lot of redundancy in these methods that could be reduced
There was a problem hiding this comment.
@flowerthrower You mean this barrier labelling? We could remove it. Only I wanted to ask whether it is so unuseful, since maybe for compiler engineers, it would be very nice to have this labels to see how everything works together
There was a problem hiding this comment.
no its about all single qubit gates are handeled individually though they share 90 of the same code
There was a problem hiding this comment.
@flowerthrower good point, we will adapt it
|
|
||
| self.transpiled_qc.barrier(label=f"H {logical_qubit_index}") | ||
| if self.add_syndromes: | ||
| self.insert_syndromes(logical_qubit_index) |
There was a problem hiding this comment.
the syndrome cycle scheme should be documented in the docstrings
| self.insert_syndromes(target_logical_qubit_index) | ||
|
|
||
| # it could use the hadamards with cnots | ||
| def _handle_cz(self, instruction: CircuitInstruction) -> None: |
There was a problem hiding this comment.
also redundant two-qubit handlers
There was a problem hiding this comment.
@flowerthrower But CZ has its own transversal implementation and realizing it by H CX possibly would add more unnessesary complications, e.g. when one would like to use CZ gate, it will become H CX and will increase circuit depth unnecessarily also by adding all syndrome measurement stuff, when a direct and easier implementation exist. But when we will not add syndromes in between, then this circuit could be optimized again to CZ gates, when compiler touches it, what could make things messy. For example, one compiler engineer want to see what happens to CZ gate and sees H CX, and probably it will think that direct CZ implementation isnt supported by these correction codes, what is wrong. Please correct me.
There was a problem hiding this comment.
the comment is about redundant two qubit halndlers that have repeating code for cx and cz
There was a problem hiding this comment.
@flowerthrower Oh, sorry for misunderstanding
Changed circuit structure tests to use hardcoded gate data, only for 3 qubits
structural tests now manually transpile circuits
…ilation of gates without modifying the error correction tests
Removed other tests and test_error_correction.py
…nic0909/MQT_Bench_ErrorCorrectionCodes into feature/ErrorCorrection_MergePrep
4 participants
Summary
This PR introduces a dedicated Error Correction Module for MQT Bench. It adds two high-level transpilers that encode standard benchmark circuits into fault-tolerant logical circuits using the 7-qubit Steane code and the 9-qubit Shor code, respectively. The module is designed to integrate cleanly with the existing
benchmark_generationAPI.Changes
New files
src/mqt/bench/error_correction/steane_transpiler.py—SteaneTranspilerclass: encodes logical qubits into 7-qubit Steane blocks, applies transversal Clifford gates (H, X, Z, S, CX, CZ), and handles T-gates via magic state injection with ancilla teleportation.src/mqt/bench/error_correction/shor_transpiler.py—ShorTranspilerclass: encodes logical qubits into 9-qubit Shor blocks, implements the full logical Clifford gate set (including the non-transversal H via bit-swap), and supports S- and T-gates via magic state teleportation gadgets.tests/test_error_correction.py— pytest suite covering gate-level equivalence (via MQT QCEC), correctness under injected bit- and phase-flip errors (via Hellinger fidelity with Aer), and circuit structure validation against pre-computed gate counts for GHZ, Bernstein–Vazirani, graph state, and QFT benchmarks.How it works
Both transpilers follow the same three-phase pipeline:
QFTGate) are first decomposed into the supported basis set{H, X, Z, S, T, CX, CZ}.if_testfeature.T-gates, which are non-transversal, are handled via a teleportation gadget: a magic state ancilla block is prepared, a logical CNOT is applied from the data qubit to the ancilla, the ancilla is decoded and measured, and a conditional logical S correction is applied on outcome 1.
Testing
mqt.qcecfor all single- and two-qubit Clifford gates.gate_counts.jsonfor qubit counts ranging from 3 to 9 across all supported algorithms.Notes
add_syndromes=Falsefor lightweight structural or equivalence checks.Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.