-
-
Notifications
You must be signed in to change notification settings - Fork 30
✨ Implement Shor's 9 Qubit Code #803
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
✨ Implement Shor's 9 Qubit Code #803
Conversation
81b22da to
f8b3db8
Compare
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a new Shor's 9-qubit-code benchmark (with mid-circuit measurements and conditional corrections), test coverage for structure and multi-logical-qubit scaling, and increases the minimum Qiskit dependency to 2.0.0. Error handling enforces total qubits are multiples of 17. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User"
participant Registry as "Benchmark Registry"
participant QC as "QuantumCircuit"
participant Backend as "Backend/QPU"
participant ClReg as "Classical Register"
rect rgba(135,206,250,0.5)
User->>Registry: request "Shor's 9 Qubit Code" circuit (num_qubits)
Registry->>QC: create_circuit(num_qubits)
QC->>Backend: submit circuit (includes mid-circuit measurements)
Backend->>ClReg: write mid-circuit measurement outcomes
ClReg-->>Backend: evaluate conditionals (IfElse)
Backend->>QC: apply conditional corrections (X/Z) to data qubits
Backend->>QC: final measurement -> ClReg
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/mqt/bench/benchmarks/shors_nine_qubit_code.py`:
- Around line 260-263: Fix the typo in the encoding docstring by replacing the
incorrect ket string "|--->⟩" with the correct "|---⟩" in the Encoding section;
locate the docstring that describes "Phase-flip encoding" (the text containing
"|0⟩ → |+++⟩ and |1⟩ → |--->⟩") and edit that string to remove the extra ">" so
it reads "|0⟩ → |+++⟩ and |1⟩ → |---⟩".
- Around line 84-110: The phase-flip syndrome circuit in
_get_nine_qubit_shors_code_phase_flip_syndrome_extraction_circuit uses
ancilla-controlled CNOTs but lacks an explicit note of which X‑parity
stabilizers are being measured; update the function docstring or add a brief
inline comment near the uses of phase_flip_syndrome and logical_qubit to state
that the circuit measures X⊗X⊗X parity on each 3‑qubit block (qubits 0–2, 3–5,
6–8) using ancillas phase_flip_syndrome[0] and phase_flip_syndrome[1], and
clarify the ancilla measurement mapping (01 → block 1, 10 → block 2, 11 → block
3, 00 → no error) so the CX patterns are immediately understandable.
- Around line 113-144: Update the docstring of
_apply_nine_qubit_shors_code_bit_flip_correction to explicitly state the assumed
Qiskit little-endian bit order (ClassicalRegister index 0 = least-significant
bit) and explain how the integer syndrome patterns (e.g., 0b000001, 0b000010,
etc.) map to the three-bit syndrome values for each 3-qubit block so future
maintainers understand the mapping and conditional X corrections.
In `@src/mqt/bench/custom_solovay_kitaev.py`:
- Around line 62-63: Fix the awkward comment in
src/mqt/bench/custom_solovay_kitaev.py by rephrasing the sentence that currently
reads "as we know it comes from a Qiskit gate, as this we know it will generate"
to a single clear clause; for example, change the comment inside the
input-matrix check block to: "we do not check the input matrix because it comes
from a Qiskit gate and therefore will be a valid SU(2) matrix" so it reads
grammatically correct and unambiguous.
- Around line 52-54: The docstring in src/mqt/bench/custom_solovay_kitaev.py
incorrectly states that TranspilerError is raised when a gate lacks to_matrix,
but the implementation silently continues for such gates; update the docstring
(or remove the Raises section) to reflect the actual behavior by removing the
TranspilerError claim and explaining that gates without a to_matrix method are
skipped, referencing the to_matrix check and TranspilerError symbol so reviewers
can locate the change.
- Around line 68-72: The code directly accesses the private attribute self._sk
and calls self._sk.run(...) then uses dag.substitute_node_with_dag(...); add a
clear FIXME comment above this usage that explains why accessing self._sk is
necessary (e.g., to call the Solovay–Kitaev implementation for approximation)
and include a removal trigger note "Remove once qiskit minimum version is bumped
to 2.0.0" so future maintainers know when to refactor; keep the comment adjacent
to the lines invoking self._sk, reference self._sk, _sk.run, recursion_degree
and dag.substitute_node_with_dag in the comment, and don’t change behavior—this
is purely documentation for the fragile private-attribute dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/mqt/bench/benchmarks/shors_nine_qubit_code.py`:
- Around line 306-313: The current validation only checks divisibility by 17 and
allows invalid values like 0 or negative multiples; update the guard around
num_qubits so it requires num_qubits to be a positive multiple of 17 (e.g.,
num_qubits >= 17 and num_qubits % 17 == 0) and raise a clear ValueError
otherwise; adjust the existing validation block that computes num_logical_qubits
and calls _create_single_logical_qubit_circuit to rely on this stricter check.
cab9321 to
f2392b9
Compare
…ious sizes of Shor's 9 Qubit Code circuits
…est_quantumcircuit_levels
bcfa75f to
bdf81ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 20: Fix the double-hyphen typo at the start of the CHANGELOG entry for
"⬆️ Increase minimum Qiskit version to 2.0.0" by removing the extra leading
hyphen so the line begins with a single list marker (e.g., change "- - ⬆️
Increase minimum Qiskit version to 2.0.0..." to "- ⬆️ Increase minimum Qiskit
version to 2.0.0...").
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
flowerthrower
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done @adnathanail, really enjoyed the clear separation of circuit components and the well-documented construction of the final circuit. The only minor thing apart from the comments, is the measure_all at the end of the circuit, since one technically doesn't need the information of the ancillary qubits any more. However, I am aware that we are not doing this consistently throughout the other benchmarks either. So perhaps that is sth we should scrutinize for all benchmarks after discussing internally what the best practice here would be. After one final look by @burgholzer (could be fast) and this will be ready to merge.
|
@flowerthrower Thanks very much! I've combined those tests, and removed the |
|
@flowerthrower not 100% why CI is failing - it's just Python 3.13 on Ubuntu arm, and it seems to be the parallel test runner collecting different tests? Do you have the button to rerun a specific test? It's not showing on mine |
Description
Adds Shor's 9 Qubit Code as a benchmark
Fixes #790
Checklist: