Conversation
Use new elimination procedure for state preparation as well.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mqt/qecc/codes/stabilizer_code.py (1)
237-245:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the logical pairing matrix in both directions.
The current check only enforces “each Z logical anti-commutes with exactly one X logical” via row sums. A matrix where two Zs both anti-commute with the same X still passes that test, even though one X then has no partner. This should also require exactly one anti-commuting partner per column.
Suggested fix
commutations = (self.z_logicals.tableau @ self.x_logicals.tableau).matrix - if not np.all(np.sum(commutations, axis=1) == 1): + if not ( + np.all(np.sum(commutations, axis=1) == 1) + and np.all(np.sum(commutations, axis=0) == 1) + ): msg = "Every logical X-operator must anti-commute with exactly one logical Z-operator." raise InvalidStabilizerCodeError(msg)🤖 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/qecc/codes/stabilizer_code.py` around lines 237 - 245, The current validation only ensures each row of commutations (computed from self.z_logicals.tableau @ self.x_logicals.tableau) sums to 1; update the check to require both row sums and column sums equal 1 so every Z has exactly one X partner and every X has exactly one Z partner. Concretely, replace the single-row assertion on commutations with a dual check using np.sum(..., axis=1) == 1 and np.sum(..., axis=0) == 1 and raise InvalidStabilizerCodeError with the same message if either fails; leave the stabilizer_commutations (self.generators.tableau @ self.generators.tableau) check 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 `@src/mqt/qecc/circuit_synthesis/circuit_utils.py`:
- Around line 195-217: Update the docstrings for the three new public functions
to use Google-style format like the rest of the module: for depth(circ: Circuit)
add a docstring with a short description, an Args section documenting circ
(Circuit) and a Returns section indicating it returns an int (depth); for
remove_single_qubit_gates(circ: Circuit) and remove_swap_gates(circ: Circuit)
add Google-style docstrings with Args documenting circ (Circuit) and Returns
indicating they return a Circuit (the filtered circuit). Keep descriptions
concise and consistent with existing docstrings in the file.
In `@src/mqt/qecc/circuit_synthesis/circuits.py`:
- Around line 542-545: num_qubits() on CNOTCircuit returns 1 for an empty
circuit because max(..., default=0) + 1 yields 1; change the logic in the
num_qubits method to return 0 when there are no CNOTs, initializations, or
inputs by using max(..., default=-1) + 1 (or an explicit empty check) so the
result is 0 for an empty circuit; update the code referencing attributes
self.cnots, self._initializations, and self._inputs in the num_qubits method to
implement this change.
- Around line 530-533: from_cnot_list currently appends values from the inputs
parameter into cnot_circuit._inputs without validating them, allowing negative
or invalid logical indices to corrupt inputs(), get_logicals_css(), and
num_qubits(); update the loop in from_cnot_list so that for each q in inputs you
(1) validate q is an int and q >= 0 (raise ValueError on invalid), (2) ensure q
is not already present in cnot_circuit._inputs and (optionally) not in
cnot_circuit._initializations before appending, and (3) append only validated
values; reference the symbols cnot_circuit._inputs,
cnot_circuit._initializations, and the from_cnot_list call site when making the
change.
- Around line 744-764: The explicit-wiring branch currently validates
keys/values and uniqueness but misses the case where an enc2 input wired to an
enc1 output is later reset in enc2.to_stim_circuit(), which would clobber
incoming state; update the compose() path to inspect enc2.to_stim_circuit() (or
its stim.Circuit representation) for any reset/initialize operations on the
qubit indices referenced by wiring.values() and raise a ValueError if any wired
enc2 input is initialized/reset (matching the check done in
compose_cnot_circuits()); keep using the same error semantics (i.e. clear
message) and perform this check before calling
compose_circuits(enc1.to_stim_circuit(), enc2.to_stim_circuit(), wiring) and
CliffordIsometry.from_stim_circuit(composed).
In `@src/mqt/qecc/circuit_synthesis/rollout.py`:
- Around line 272-284: The recursive rollout path in _create_rollout_strategy
currently wraps the child RolloutCandidateGenerator into a new
EliminationStrategy but omits the caller’s non-default settings
(selection_strategy, filters, callback, post_process_fn), causing recursion to
use defaults; update _create_rollout_strategy to construct EliminationStrategy
with the same selection_strategy, filters, callback, and post_process_fn values
used in _create_fresh_rollout_strategy (i.e., pass
selection_strategy=self.selection_strategy, filters=self.filters,
callback=self.callback, post_process_fn=self.post_process_fn) while still using
the RolloutCandidateGenerator and existing termination_criterion so child
rollouts preserve the configured behavior.
In `@src/mqt/qecc/codes/stabilizer_code.py`:
- Around line 536-553: The parser currently builds rows from row_strings and
then calls matrix = np.array(rows, dtype=np.int8), which will raise a generic
NumPy ValueError for ragged rows; update the code after populating rows to
validate that all rows have the same length (e.g., expected_len = len(rows[0])
and verify every len(r) == expected_len) and if any mismatch is found raise
InvalidStabilizerCodeError with a clear message about ragged binary matrix rows;
perform this check before calling np.array so the domain-specific error is
raised instead of NumPy's.
- Around line 94-99: The method that computes equality of stabilizer groups
should first check that the codes act on the same number of qubits and return
False if not; modify the function containing the shown snippet (the
equal_stabilizer_group/is_equivalent path that uses self.generators.as_matrix()
and other.generators.as_matrix()) to guard with if self.n != other.n: return
False before computing ranks or calling np.vstack, so stacking is only attempted
for matching qubit counts and the caller (e.g. is_equivalent) won’t raise on
simple mismatches.
In `@tests/circuit_synthesis/test_utils.py`:
- Around line 684-697: The test parametrization only verifies
num_two_qubit_gates(circuit) for the default count_swaps=False path and misses
the SWAP handling branch; update test_num_two_qubit_gates to include at least
one case exercising SWAP by adding a parameter or separate assertion that calls
num_two_qubit_gates(circuit, count_swaps=True) (for example a circuit containing
"SWAP 0 1") and asserts the expected count (1), so the SWAP branch in
num_two_qubit_gates is covered and will fail if regressions occur.
---
Outside diff comments:
In `@src/mqt/qecc/codes/stabilizer_code.py`:
- Around line 237-245: The current validation only ensures each row of
commutations (computed from self.z_logicals.tableau @ self.x_logicals.tableau)
sums to 1; update the check to require both row sums and column sums equal 1 so
every Z has exactly one X partner and every X has exactly one Z partner.
Concretely, replace the single-row assertion on commutations with a dual check
using np.sum(..., axis=1) == 1 and np.sum(..., axis=0) == 1 and raise
InvalidStabilizerCodeError with the same message if either fails; leave the
stabilizer_commutations (self.generators.tableau @ self.generators.tableau)
check 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: 25c7068e-66c5-4468-8f90-902e585793be
📒 Files selected for processing (7)
src/mqt/qecc/circuit_synthesis/circuit_utils.pysrc/mqt/qecc/circuit_synthesis/circuits.pysrc/mqt/qecc/circuit_synthesis/rollout.pysrc/mqt/qecc/circuit_synthesis/synthesis_utils.pysrc/mqt/qecc/codes/stabilizer_code.pytests/circuit_synthesis/test_encoder_synthesis.pytests/circuit_synthesis/test_utils.py
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/mqt/qecc/codes/stabilizer_code.py (2)
240-248:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSymplectic check on logical Z/X is incomplete.
np.all(np.sum(commutations, axis=1) == 1)only verifies that every logical Z anti‑commutes with exactly one logical X. A row‑stochastic-like pattern (e.g. all logical Zs anti‑commuting with the same logical X) would still pass this check while leaving other Xs unmatched. Verify the column sums as well so the pairing is truly one‑to‑one.🛡️ Proposed fix
commutations = (self.z_logicals.tableau @ self.x_logicals.tableau).matrix - if not np.all(np.sum(commutations, axis=1) == 1): + if not ( + np.all(np.sum(commutations, axis=1) == 1) + and np.all(np.sum(commutations, axis=0) == 1) + ): msg = "Every logical X-operator must anti-commute with exactly one logical Z-operator." raise InvalidStabilizerCodeError(msg)🤖 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/qecc/codes/stabilizer_code.py` around lines 240 - 248, The symplectic pairing check is incomplete: the current check using commutations = (self.z_logicals.tableau @ self.x_logicals.tableau).matrix and np.all(np.sum(commutations, axis=1) == 1) only ensures each Z anti‑commutes with one X but not that Xs are uniquely paired; update the validation to also require the column sums to equal 1 (e.g. np.sum(commutations, axis=0) == 1) so the commutations matrix is a permutation/one‑to‑one mapping, and raise InvalidStabilizerCodeError with the existing message if either row or column check fails.
64-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
compute_logical_ops()runs unconditionally and can break user‑supplied logicals.
__init__always callsself.compute_logical_ops()first, then optionally overridesz_logicals/x_logicalswith the caller-provided ones. Two problems:
compute_logical_ops()raisesInvalidStabilizerCodeError(line 390‑392) if it cannot deriveksymplectic pairs from the given generators. A user who passes validz_logicals/x_logicalsto bypass this derivation can still be blocked by a failure that should not have been attempted.- If only one of
z_logicals/x_logicalsis provided, the code silently mixes a user‑supplied basis with the auto‑computed counterpart, which can violate the symplectic pairing assumed by_check_code_correct()and downstream APIs (get_logical_mapping,is_z_logical/is_x_logical).Skip auto-computation when the caller already provides the logicals, and require both or neither.
🛡️ Proposed fix
- self.distance = 1 if distance is None else distance # default distance is 1 - self.compute_logical_ops() - if z_logicals is not None: - self.z_logicals = self.get_generators(z_logicals) - if x_logicals is not None: - self.x_logicals = self.get_generators(x_logicals) + self.distance = 1 if distance is None else distance # default distance is 1 + if (z_logicals is None) != (x_logicals is None): + msg = "z_logicals and x_logicals must both be provided or both be None." + raise InvalidStabilizerCodeError(msg) + if z_logicals is not None and x_logicals is not None: + self.z_logicals = self.get_generators(z_logicals) + self.x_logicals = self.get_generators(x_logicals) + else: + self.compute_logical_ops()🤖 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/qecc/codes/stabilizer_code.py` around lines 64 - 68, Currently __init__ calls self.compute_logical_ops() unconditionally which can raise InvalidStabilizerCodeError or produce half-computed logicals that are then partially overwritten; change the logic so compute_logical_ops() is called only when both z_logicals and x_logicals are None, and if the caller supplies logicals require both z_logicals and x_logicals to be provided (reject or raise if only one is given); ensure when user-supplied logicals are used you still call get_generators on both z_logicals and x_logicals and skip compute_logical_ops(), and update any validation via _check_code_correct()/get_logical_mapping()/is_z_logical/is_x_logical to assume symplectic pairs come from either both user-supplied generators or the auto-computed ones.src/mqt/qecc/circuit_synthesis/circuits.py (1)
394-406: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDrop redundant
to_qiskit_circuitanddrawoverrides inCNOTCircuit.Both methods are byte-identical to the implementations on
CliffordIsometry(lines 201–213 and 192–199). BecauseCNOTCircuitalready overridesto_stim_circuit, polymorphism ensures the inherited base implementations route through the subclass method automatically — the explicit overrides only duplicate code and risk drift between the two paths.♻️ Proposed fix
- def to_qiskit_circuit(self, remove_resets: bool = True) -> QuantumCircuit: - """Convert the CNOT circuit to a qiskit.QuantumCircuit. - - Args: - remove_resets: If set to `True`, removes resets in the |0> state from the circuit. - - Returns: - A qiskit.QuantumCircuit representation of the CNOT circuit. - """ - circ = QuantumCircuit.from_qasm_str(self.to_stim_circuit().to_qasm(open_qasm_version=2)) - if remove_resets: - return RemoveResetInZeroState()(circ) - return circ - `@classmethod`- def draw(self, *args, **kwargs): # noqa: ANN003, ANN002, ANN201 - """Draw the circuit using Qiskit visualization tools. - - Args: - *args: Positional arguments for the Qiskit draw method. - **kwargs: Keyword arguments for the Qiskit draw method. - """ - return self.to_qiskit_circuit().draw(*args, **kwargs) - def _propagate_paulis(self, xs: list[int], zs: list[int]) -> tuple[npt.NDArray[np.int8], npt.NDArray[np.int8]]:Also applies to: 566-573
🤖 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/qecc/circuit_synthesis/circuits.py` around lines 394 - 406, The CNOTCircuit class contains redundant overrides of to_qiskit_circuit and draw that are byte-for-byte identical to the implementations on CliffordIsometry; remove the to_qiskit_circuit and draw methods from CNOTCircuit so it inherits the base behavior (CliffordIsometry.to_qiskit_circuit and .draw) which will correctly dispatch via CNOTCircuit.to_stim_circuit; ensure both duplicate occurrences (the override around the to_qiskit_circuit shown and the other duplicate at the later block) are deleted and run tests to confirm no behavioral change.
🤖 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/qecc/circuit_synthesis/circuits.py`:
- Line 164: Make the boolean flags keyword-only by adding a bare * in the method
signatures that accept booleans: change the signatures of to_stim_circuit (on
the base Circuit/CliffordIsometry implementations) and the overloads that accept
with_resets/remove_resets so callers must pass those booleans by keyword; do the
same for CNOTCircuit.to_qiskit_circuit so its signature matches the base class.
Update the function signatures for to_stim_circuit, any other to_stim_circuit
overloads that take remove_resets/with_resets, and CNOTCircuit.to_qiskit_circuit
to include a leading * before the boolean parameters, leaving all internal call
sites (which already use keyword args) unchanged.
- Around line 332-336: Remove the redundant re-initialization of
self._initializations in the CNOT circuit __init__; since super().__init__() (in
the base class used by the class defining __init__) already sets
self._initializations: dict[int, str] = {}, delete the line rebinding
self._initializations in the __init__ method so only the cnots list is
initialized there (refer to the __init__ method of the CNOT circuit class and
the attribute name _initializations to locate the code).
In `@src/mqt/qecc/codes/stabilizer_code.py`:
- Around line 460-491: The heuristic in _is_binary_matrix_format is brittle
because it only checks tokens[:5] on the first non-empty line and can
false-positive; update the function to validate all tokens of the first
non-empty line (not just the first five) and ensure every token is "0" or "1"
(for either comma- or whitespace-separated parsing), optionally also verifying
the second non-empty line has the same column count to reduce false positives;
this makes misclassification unlikely and avoids parse errors later in
_load_from_binary_matrix by failing early with a clear format rejection.
- Around line 138-160: get_logical_mapping currently can reuse the same index in
other for multiple logicals; change it to enforce a bijection by tracking
available target indices (e.g., a set of indices built from
other.z_logicals/other.x_logicals) and removing an index once it's matched; in
the loop over self.z_logicals/self.x_logicals only consider candidate idx values
that remain in the available set, append and remove the matched idx, and return
None if no unused match is found for any logical; keep existing checks using
self.k and equal_logical_basis and use stabilizer_equivalent for matching as
before.
- Around line 305-308: When ns.size == 0 the code silently sets self.z_logicals
and self.x_logicals to empty StabilizerTableau.empty(n) even when self.k > 0,
which creates a code with nonzero k but no logicals; change this branch to fail
fast: if ns.size == 0 and self.k > 0 raise a ValueError (or appropriate
exception) with a clear message instead of returning, referencing ns.size,
self.k, self.z_logicals and self.x_logicals so the check occurs before
_check_code_correct(); alternatively, remove the early return so the
canonical-pairing logic can run and the existing length checks will raise —
implement the former (explicit raise) in the block that currently assigns
StabilizerTableau.empty.
---
Outside diff comments:
In `@src/mqt/qecc/circuit_synthesis/circuits.py`:
- Around line 394-406: The CNOTCircuit class contains redundant overrides of
to_qiskit_circuit and draw that are byte-for-byte identical to the
implementations on CliffordIsometry; remove the to_qiskit_circuit and draw
methods from CNOTCircuit so it inherits the base behavior
(CliffordIsometry.to_qiskit_circuit and .draw) which will correctly dispatch via
CNOTCircuit.to_stim_circuit; ensure both duplicate occurrences (the override
around the to_qiskit_circuit shown and the other duplicate at the later block)
are deleted and run tests to confirm no behavioral change.
In `@src/mqt/qecc/codes/stabilizer_code.py`:
- Around line 240-248: The symplectic pairing check is incomplete: the current
check using commutations = (self.z_logicals.tableau @
self.x_logicals.tableau).matrix and np.all(np.sum(commutations, axis=1) == 1)
only ensures each Z anti‑commutes with one X but not that Xs are uniquely
paired; update the validation to also require the column sums to equal 1 (e.g.
np.sum(commutations, axis=0) == 1) so the commutations matrix is a
permutation/one‑to‑one mapping, and raise InvalidStabilizerCodeError with the
existing message if either row or column check fails.
- Around line 64-68: Currently __init__ calls self.compute_logical_ops()
unconditionally which can raise InvalidStabilizerCodeError or produce
half-computed logicals that are then partially overwritten; change the logic so
compute_logical_ops() is called only when both z_logicals and x_logicals are
None, and if the caller supplies logicals require both z_logicals and x_logicals
to be provided (reject or raise if only one is given); ensure when user-supplied
logicals are used you still call get_generators on both z_logicals and
x_logicals and skip compute_logical_ops(), and update any validation via
_check_code_correct()/get_logical_mapping()/is_z_logical/is_x_logical to assume
symplectic pairs come from either both user-supplied generators or the
auto-computed ones.
🪄 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: fe3666a1-ac4e-400e-ad21-1819447a71a8
📒 Files selected for processing (4)
src/mqt/qecc/circuit_synthesis/circuit_utils.pysrc/mqt/qecc/circuit_synthesis/circuits.pysrc/mqt/qecc/codes/stabilizer_code.pytests/circuit_synthesis/test_utils.py
|
|
||
| return iso | ||
|
|
||
| def to_stim_circuit(self, with_resets: bool = True) -> stim.Circuit: |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Make with_resets/remove_resets keyword-only to resolve FBT001/FBT002.
Ruff flags boolean positional arguments at lines 164, 201, and 374. Since CliffordIsometry and the new CNOTCircuit.to_stim_circuit overload are introduced in this PR and every internal call site already passes these flags by keyword (e.g. lines 173/199/210/773 and the test suites), making them keyword-only is non-breaking and consistent with the same fix already applied to count_swaps on two_qubit_gate_depth/num_two_qubit_gates.
As per coding guidelines, "PREFER fixing reported warnings over suppressing them in Python code".
♻️ Proposed fix
- def to_stim_circuit(self, with_resets: bool = True) -> stim.Circuit:
+ def to_stim_circuit(self, *, with_resets: bool = True) -> stim.Circuit:- def to_qiskit_circuit(self, remove_resets: bool = True) -> QuantumCircuit:
+ def to_qiskit_circuit(self, *, remove_resets: bool = True) -> QuantumCircuit:- def to_stim_circuit(self, with_resets: bool = True) -> stim.Circuit:
+ def to_stim_circuit(self, *, with_resets: bool = True) -> stim.Circuit:For consistency, also apply * to CNOTCircuit.to_qiskit_circuit (line 394) so the signature matches the base class.
Also applies to: 201-201, 374-374
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 164-164: Boolean-typed positional argument in function definition
(FBT001)
[warning] 164-164: Boolean default positional argument in function definition
(FBT002)
🤖 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/qecc/circuit_synthesis/circuits.py` at line 164, Make the boolean
flags keyword-only by adding a bare * in the method signatures that accept
booleans: change the signatures of to_stim_circuit (on the base
Circuit/CliffordIsometry implementations) and the overloads that accept
with_resets/remove_resets so callers must pass those booleans by keyword; do the
same for CNOTCircuit.to_qiskit_circuit so its signature matches the base class.
Update the function signatures for to_stim_circuit, any other to_stim_circuit
overloads that take remove_resets/with_resets, and CNOTCircuit.to_qiskit_circuit
to include a leading * before the boolean parameters, leaving all internal call
sites (which already use keyword args) unchanged.
Comment on lines
+332
to
+336
| def __init__(self) -> None: | ||
| """Initialize an empty CNOT circuit.""" | ||
| super().__init__() | ||
| self.cnots: list[tuple[int, int]] = [] | ||
| self._initializations: dict[int, str] = {} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Redundant re-initialization of _initializations.
super().__init__() already creates self._initializations: dict[int, str] = {}. Re-binding it on line 336 has no effect on behavior and just adds noise.
♻️ Proposed fix
def __init__(self) -> None:
"""Initialize an empty CNOT circuit."""
super().__init__()
self.cnots: list[tuple[int, int]] = []
- self._initializations: dict[int, str] = {}🤖 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/qecc/circuit_synthesis/circuits.py` around lines 332 - 336, Remove
the redundant re-initialization of self._initializations in the CNOT circuit
__init__; since super().__init__() (in the base class used by the class defining
__init__) already sets self._initializations: dict[int, str] = {}, delete the
line rebinding self._initializations in the __init__ method so only the cnots
list is initialized there (refer to the __init__ method of the CNOT circuit
class and the attribute name _initializations to locate the code).
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 (1)
src/mqt/qecc/codes/stabilizer_code.py (1)
64-70:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
compute_logical_ops()runs even when both logical bases are provided.
compute_logical_ops()is invoked unconditionally before the user-suppliedz_logicals/x_logicalsoverrides. Beyond the wasted work, this is now a behavioral regression risk: if the auto-computation can fail (e.g.compute_logical_opsraisesInvalidStabilizerCodeErrorfrom line 391 when canonical pairing fails for the auto-derived basis), construction fails even when the caller passed perfectly valid logicals. Skip the computation when both overrides are provided.🛠️ Proposed fix
- self.compute_logical_ops() - if z_logicals is not None: - self.z_logicals = self.get_generators(z_logicals) - if x_logicals is not None: - self.x_logicals = self.get_generators(x_logicals) + if z_logicals is None or x_logicals is None: + self.compute_logical_ops() + if z_logicals is not None: + self.z_logicals = self.get_generators(z_logicals) + if x_logicals is not None: + self.x_logicals = self.get_generators(x_logicals)🤖 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/qecc/codes/stabilizer_code.py` around lines 64 - 70, Only call compute_logical_ops() when at least one logical basis is not supplied by the caller: check if z_logicals is None or x_logicals is None before invoking compute_logical_ops(); otherwise skip it and directly set self.z_logicals = self.get_generators(z_logicals) and/or self.x_logicals = self.get_generators(x_logicals) as present, then run self._check_code_correct(). Ensure you reference compute_logical_ops, get_generators, z_logicals, x_logicals, and _check_code_correct so the change is applied in that constructor/init block.
🤖 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 `@docs/Encoders.md`:
- Around line 49-51: The examples call depth_optimal_encoding_circuit(...,
max_timeout=2) and then immediately use depth_opt.get_uninitialized(), which can
fail if the function returned None on timeout; update the examples to guard
against None by checking if depth_opt is None before accessing get_uninitialized
(e.g., if depth_opt is None: handle/report timeout and skip further calls else
proceed to call depth_opt.get_uninitialized()), and apply the same None-check
pattern to the other similar snippets that use depth_optimal_encoding_circuit
and its return value.
In `@src/mqt/qecc/codes/constructions.py`:
- Line 23: The one-line docstring for the function that returns the [[2^r-1,
2^r-2r-1, 3]] quantum Hamming code must be rewritten to Google-style: add a
short summary line, a blank line, an Args section documenting the parameter r
(type and meaning), and a Returns section describing the returned quantum code
(type and shape), and include any Exceptions in a Raises section if applicable;
update the triple-quoted string in the function that returns the [[2^r-1,
2^r-2r-1, 3]] quantum Hamming code accordingly so it follows the Google
docstring structure.
In `@src/mqt/qecc/codes/pauli.py`:
- Around line 231-237: Remove the dead expression "matrix.shape[0] // 2" in the
classmethod that constructs a SymplecticMatrix: the standalone integer division
is unused and should be deleted (or, if the half-size is needed later, assign it
to a variable like n = matrix.shape[0] // 2). Update the block around the matrix
shape check and SymplecticMatrix(matrix) creation so only meaningful expressions
remain (refer to the variables matrix, SymplecticMatrix, and cls to locate the
code).
- Around line 707-709: The __hash__ implementation on CheckMatrix uses
self.matrix.tobytes() and self.type but omits the matrix shape, causing hash
collisions for differently-shaped arrays with identical byte layouts; update the
__hash__ method to include the matrix shape (e.g., incorporate
self.matrix.shape) along with the byte contents and self.type so the hash is
unique for different shapes while remaining consistent with __eq__.
- Around line 666-681: Replace the use of assert in CheckMatrix.__init__ with an
explicit input validation that raises a proper exception (ValueError) when
pauli_type is not "X" or "Z"; update CheckMatrix.__init__ to validate pauli_type
and raise ValueError("Check matrix type must be either 'X' or 'Z'.") so
downstream code (is_x_type, is_z_type, from_check_matrix) never receives an
invalid type.
In `@src/mqt/qecc/codes/stabilizer_code.py`:
- Around line 310-311: The helper mod2_rank is doing an unnecessary modulo
operation each call; remove the redundant mat % 2 and call the GF(2) rank
function directly. Update mod2_rank to pass mat (ensuring its dtype/values are
still {0,1} if needed) into ldpc.mod2.mod2_numpy.rank and return int(rank(mat))
so the expensive per-call modulo is eliminated; reference the mod2_rank wrapper
and the underlying rank function used in the rank-growth loop.
- Line 296: Remove the dead expression int(rank(mat)) which is computed and
discarded; locate the stray call in stabilizer_code.py inside the method where
`mat` is used (it was a leftover before switching to `self.k`) and delete that
line so the code relies on `self.k` instead of the unused `int(rank(mat))`.
---
Outside diff comments:
In `@src/mqt/qecc/codes/stabilizer_code.py`:
- Around line 64-70: Only call compute_logical_ops() when at least one logical
basis is not supplied by the caller: check if z_logicals is None or x_logicals
is None before invoking compute_logical_ops(); otherwise skip it and directly
set self.z_logicals = self.get_generators(z_logicals) and/or self.x_logicals =
self.get_generators(x_logicals) as present, then run self._check_code_correct().
Ensure you reference compute_logical_ops, get_generators, z_logicals,
x_logicals, and _check_code_correct so the change is applied in that
constructor/init block.
🪄 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: 49dd2f18-87d1-4c2c-afdd-b8e645cb7142
📒 Files selected for processing (4)
docs/Encoders.mdsrc/mqt/qecc/codes/constructions.pysrc/mqt/qecc/codes/pauli.pysrc/mqt/qecc/codes/stabilizer_code.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Tom Peham <pehamtom@gmx.at>
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/qecc/codes/stabilizer_code.py (1)
64-68: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winSkip
compute_logical_ops()when both logicals are user-supplied.
compute_logical_ops()always runs and can raise (ns.size == 0withself.k > 0, or canonical pairing failure). When the caller already supplies bothz_logicalsandx_logicals, the computed values are immediately overwritten — but if the computation raises first, the user-provided logicals never get a chance to take effect, blocking otherwise-valid construction.♻️ Proposed change
- self.compute_logical_ops() - if z_logicals is not None: - self.z_logicals = self.get_generators(z_logicals) - if x_logicals is not None: - self.x_logicals = self.get_generators(x_logicals) + if z_logicals is None or x_logicals is None: + self.compute_logical_ops() + if z_logicals is not None: + self.z_logicals = self.get_generators(z_logicals) + if x_logicals is not None: + self.x_logicals = self.get_generators(x_logicals)🤖 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/qecc/codes/stabilizer_code.py` around lines 64 - 68, The constructor currently always calls compute_logical_ops() which can raise even when the caller supplied both z_logicals and x_logicals; change the flow so compute_logical_ops() is invoked only if either z_logicals or x_logicals is None, and otherwise skip it and simply set self.z_logicals = self.get_generators(z_logicals) and self.x_logicals = self.get_generators(x_logicals) as before; refer to the compute_logical_ops, get_generators methods and the z_logicals/x_logicals attributes to locate and update the conditional.
♻️ Duplicate comments (2)
docs/Encoders.md (2)
49-51:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard timeout-returned
Nonebefore dereferencing encoder objects.At Line 50 and Line 61, the examples call
.get_uninitialized()immediately, but Line 70 already documents that timeout-limited synthesis may returnNone. Add aNoneguard in both snippets to prevent notebook-cell failures.Suggested fix
depth_opt = depth_optimal_encoding_circuit(steane_code, max_timeout=2) +if depth_opt is None: + raise RuntimeError("No depth-optimal encoder found within timeout.") q_enc = depth_opt.get_uninitialized()gate_opt = gate_optimal_encoding_circuit(steane_code, max_timeout=2) +if gate_opt is None: + raise RuntimeError("No gate-optimal encoder found within timeout.") q_enc = gate_opt.get_uninitialized()Also applies to: 60-62
🤖 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 `@docs/Encoders.md` around lines 49 - 51, The examples call depth_optimal_encoding_circuit(...) which may return None on timeout, but the snippets call depth_opt.get_uninitialized() unguarded; update both occurrences to check the returned value (the variable depth_opt from depth_optimal_encoding_circuit) for None before dereferencing and only call get_uninitialized() when depth_opt is not None, handling the None case (e.g., log/raise/skip) to avoid notebook failures.
246-250:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAccept only non-string results as successful non-CSS synthesis outputs.
At Line 248, the
elsebranch treats any non-"UNSAT"value as an encoder. If the API returns other error strings, downstream calls at Line 254+ will fail. Gate success on non-string results and treat all strings as non-success; also adjust Line 263 wording to avoid implying"UNSAT"is the only string outcome.Suggested fix
for d in range(3, 9): result = depth_optimal_encoding_circuit_non_css(five_qubit_code, max_depth=d, exact_two_qubit_count=True, max_two_qubit_gates=6) - if result == "UNSAT": - print(f"No solution for max_depth={d}") - else: + if isinstance(result, str): + print(f"No solution for max_depth={d}: {result}") + else: encoder = result breakAlso applies to: 252-258, 263-263
🤖 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 `@docs/Encoders.md` around lines 246 - 250, The code treats any non-"UNSAT" value as a successful encoder; instead, verify the type of result and accept only non-string (e.g., dict/object) values as success. Replace the else branch that sets encoder = result with a guard that checks if result is a string (treat as failure/log and continue) and only assign encoder when result is not a string; update the subsequent success/failure log wording (the message that currently mentions "UNSAT") to indicate a non-success string response rather than implying "UNSAT" is the only string outcome; refer to the variables result and encoder and the loop that breaks on success.
🤖 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/qecc/codes/pauli.py`:
- Around line 530-570: The code assumes non-stabilizer rows in other_rows form
X/Z pairs but computes k = len(other_rows) // 2 which silently accepts odd
lengths; add an explicit guard right after other_rows is built to validate
len(other_rows) is even and that len(other_rows) == 2 * k (or simply
len(other_rows) % 2 == 0), and if not raise a ValueError with a clear message;
reference the variables other_rows, k, logical_x_rows/logical_z_rows and the
destabilizer construction loop (the d_i logic) so the check prevents mismatched
pairing before the destabilizer adjustments run.
In `@src/mqt/qecc/codes/stabilizer_code.py`:
- Around line 300-308: Remove the duplicated assignments in the block where n,
mat, and identity are set: keep a single assignment for n = self.n, mat =
self.generators.tableau.matrix, and identity = np.eye(n, dtype=np.int8) (and
retain z0 = np.zeros((n, n), dtype=np.int8)), removing the repeated lines so the
function/stabilizer_code.py code around those symbols is not cluttered by
redundant reassignments.
---
Outside diff comments:
In `@src/mqt/qecc/codes/stabilizer_code.py`:
- Around line 64-68: The constructor currently always calls
compute_logical_ops() which can raise even when the caller supplied both
z_logicals and x_logicals; change the flow so compute_logical_ops() is invoked
only if either z_logicals or x_logicals is None, and otherwise skip it and
simply set self.z_logicals = self.get_generators(z_logicals) and self.x_logicals
= self.get_generators(x_logicals) as before; refer to the compute_logical_ops,
get_generators methods and the z_logicals/x_logicals attributes to locate and
update the conditional.
---
Duplicate comments:
In `@docs/Encoders.md`:
- Around line 49-51: The examples call depth_optimal_encoding_circuit(...) which
may return None on timeout, but the snippets call depth_opt.get_uninitialized()
unguarded; update both occurrences to check the returned value (the variable
depth_opt from depth_optimal_encoding_circuit) for None before dereferencing and
only call get_uninitialized() when depth_opt is not None, handling the None case
(e.g., log/raise/skip) to avoid notebook failures.
- Around line 246-250: The code treats any non-"UNSAT" value as a successful
encoder; instead, verify the type of result and accept only non-string (e.g.,
dict/object) values as success. Replace the else branch that sets encoder =
result with a guard that checks if result is a string (treat as failure/log and
continue) and only assign encoder when result is not a string; update the
subsequent success/failure log wording (the message that currently mentions
"UNSAT") to indicate a non-success string response rather than implying "UNSAT"
is the only string outcome; refer to the variables result and encoder and the
loop that breaks on success.
🪄 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: 8634ed67-b324-4496-92ec-bb7f27c7a5fa
📒 Files selected for processing (4)
docs/Encoders.mdsrc/mqt/qecc/codes/constructions.pysrc/mqt/qecc/codes/pauli.pysrc/mqt/qecc/codes/stabilizer_code.py
Comment on lines
+530
to
+570
| other_rows = [ | ||
| (i, stabilizers.tableau[i].copy(), stabilizers.phase[i]) for i in range(m_total) if i not in stab_row_set | ||
| ] | ||
|
|
||
| k = len(other_rows) // 2 | ||
| logical_x_rows = other_rows[:k] | ||
| logical_z_rows = other_rows[k:] | ||
|
|
||
| destabilizers: list[npt.NDArray[np.int8]] = [] | ||
| for idx, stab_row_idx in enumerate(stab_rows): | ||
| stab_i = SymplecticVector(stabilizers.tableau[stab_row_idx]) | ||
|
|
||
| remaining_stab_indices = [stab_rows[j] for j in range(idx + 1, m)] | ||
|
|
||
| d_i = _initialize_destabilizer_from_nullspace(stab_i, remaining_stab_indices, stabilizers) | ||
|
|
||
| if d_i is None: | ||
| msg = f"Could not find valid initial destabilizer for stabilizer at row {stab_row_idx}." | ||
| raise ValueError(msg) | ||
|
|
||
| for other_idx_pos in range(idx): | ||
| s_j = SymplecticVector(stabilizers.tableau[stab_rows[other_idx_pos]]) | ||
| if d_i @ s_j == 1: | ||
| d_j = SymplecticVector(destabilizers[other_idx_pos]) | ||
| d_i += d_j | ||
|
|
||
| for other_idx_pos in range(idx): | ||
| d_j = SymplecticVector(destabilizers[other_idx_pos]) | ||
| if d_i @ d_j == 1: | ||
| s_j = SymplecticVector(stabilizers.tableau[stab_rows[other_idx_pos]]) | ||
| d_i += s_j | ||
|
|
||
| for logical_idx, (_, logical_row, _) in enumerate(other_rows): | ||
| log = SymplecticVector(logical_row) | ||
| if d_i @ log == 1: | ||
| if logical_idx < k: | ||
| l_z = SymplecticVector(other_rows[logical_idx + k][1]) | ||
| d_i += l_z | ||
| else: | ||
| l_x = SymplecticVector(other_rows[logical_idx - k][1]) | ||
| d_i += l_x |
There was a problem hiding this comment.
Validate that non-stabilizer rows come in X/Z pairs.
k = len(other_rows) // 2 silently floor-divides, and the loop at Lines 562‑570 assumes a strict pairing where row i (i < k) is logical X and row i + k is its logical Z partner. If len(other_rows) is odd (e.g., a partial tableau missing one logical, or any input that doesn’t conform to the documented [X1..Xk, Z1..Zk] ordering), the destabilizer adjustments will mismatch pairs and produce an invalid tableau without raising.
Consider validating the precondition explicitly so violations surface as a clean ValueError rather than a corrupted tableau:
🛡️ Proposed guard
+ if len(other_rows) % 2 != 0:
+ msg = (
+ "complete_stabilizer_tableau_with_destabilizers requires non-stabilizer rows "
+ f"to come in matched logical X/Z pairs; got {len(other_rows)} rows."
+ )
+ raise ValueError(msg)
+
k = len(other_rows) // 2
logical_x_rows = other_rows[:k]
logical_z_rows = other_rows[k:]🤖 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/qecc/codes/pauli.py` around lines 530 - 570, The code assumes
non-stabilizer rows in other_rows form X/Z pairs but computes k =
len(other_rows) // 2 which silently accepts odd lengths; add an explicit guard
right after other_rows is built to validate len(other_rows) is even and that
len(other_rows) == 2 * k (or simply len(other_rows) % 2 == 0), and if not raise
a ValueError with a clear message; reference the variables other_rows, k,
logical_x_rows/logical_z_rows and the destabilizer construction loop (the d_i
logic) so the check prevents mismatched pairing before the destabilizer
adjustments run.
Comment on lines
+300
to
+308
| n = self.n | ||
| mat = self.generators.tableau.matrix | ||
| n = self.n | ||
| mat = self.generators.tableau.matrix | ||
|
|
||
| identity = np.eye(n, dtype=np.int8) | ||
|
|
||
| identity = np.eye(n, dtype=np.int8) | ||
| z0 = np.zeros((n, n), dtype=np.int8) |
There was a problem hiding this comment.
Remove duplicated assignments.
n, mat, and identity are each assigned/computed twice in this block. This looks like residue from a bad merge/rebase — it's harmless at runtime but is dead code that obscures intent.
🧹 Suggested cleanup
n = self.n
mat = self.generators.tableau.matrix
- n = self.n
- mat = self.generators.tableau.matrix
identity = np.eye(n, dtype=np.int8)
-
- identity = np.eye(n, dtype=np.int8)
z0 = np.zeros((n, n), dtype=np.int8)
lamb = np.block([[z0, identity], [identity, z0]])📝 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
| n = self.n | |
| mat = self.generators.tableau.matrix | |
| n = self.n | |
| mat = self.generators.tableau.matrix | |
| identity = np.eye(n, dtype=np.int8) | |
| identity = np.eye(n, dtype=np.int8) | |
| z0 = np.zeros((n, n), dtype=np.int8) | |
| n = self.n | |
| mat = self.generators.tableau.matrix | |
| identity = np.eye(n, dtype=np.int8) | |
| z0 = np.zeros((n, n), dtype=np.int8) |
🤖 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/qecc/codes/stabilizer_code.py` around lines 300 - 308, Remove the
duplicated assignments in the block where n, mat, and identity are set: keep a
single assignment for n = self.n, mat = self.generators.tableau.matrix, and
identity = np.eye(n, dtype=np.int8) (and retain z0 = np.zeros((n, n),
dtype=np.int8)), removing the repeated lines so the function/stabilizer_code.py
code around those symbols is not cluttered by redundant reassignments.
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
1 participant
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 introduces major changes to encoding circuit (and by extension state preparation circuit) synthesis. Main features are:
Checklist: