Noise Swap#396
Open
aleramos119 wants to merge 21 commits into
Open
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
NoiseModel.proccess["name"] accepts directly the BaseGate class and also a string with the name
- Restructure NoiseModel.__init__ process loop: handle 1-site first, then adjacent/non-adjacent 2-site with Crosstalk branching - Move BaseGate to runtime import; get_operator short-circuits on BaseGate instances - Add longrange_crosstalk_* aliases to GateLibrary and _PAULI_STRINGS - Extend is_pauli to handle BaseGate instances via isinstance check - Fix pre-commit issues: split compound asserts, add __init__ docstrings to Crosstalk subclasses, update test docstring - Add .claude/ to .gitignore Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add local_dim parameter to BaseGate.__init__ (default 2 preserves qubit behaviour); interaction is now computed as log(n)/log(local_dim) and validated to be an integer - Remove power-of-2 restriction from Destroy and Create - Crosstalk inherits local_dim from constituent gates and raises ValueError if they differ Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Forward local_dim in all BaseGate methods that return a new gate (dag, conj, trans, __add__, __sub__, __mul__, __matmul__) - Replace PauliX/Y/Z.matrix with X()/Y()/Z().matrix in tests, removing the noise_library import Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughNoise/operator sourcing moved from NoiseLibrary to GateLibrary: BaseGate gained local_dim, new Zero and Crosstalk gates and factories were added, NoiseModel now resolves and consumes BaseGate instances (extracting matrices and special-crosstalk handling), dissipation detection and tests updated to match the new gate semantics. ChangesConfig
Gate Library
Noise Model
Dissipation Logic
Tests — Noise / Lindblad
Tests — Gate Library
Sequence Diagram(s)(Skipped — changes modify library APIs and normalization but do not introduce a new multi-actor sequential runtime flow requiring a diagram.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mqt/yaqs/core/data_structures/noise_model.py`:
- Around line 202-217: get_operator currently calls operator_class() without
arguments which will raise a confusing TypeError for parameterized GateLibrary
entries (e.g., rx, ry, rz, u, u2, cp, p, pvm, destroy/create); update
get_operator to either catch TypeError when instantiating operator_class and
raise a clear ValueError describing that the named gate requires parameters
(include the gate name) or validate against a whitelist of parameter-free gate
names before instantiation; also fix the docstring return description to state
it returns a BaseGate (not a matrix) and adjust the docstring summary
accordingly.
- Around line 102-123: The code only adjusts ordering for Crosstalk but leaves
non-Crosstalk 2-site operators misaligned: when you detect swapped =
sorted(sites) != sites you must also reorder the operator data to match the new
site order. For adjacent non-Crosstalk cases (where proc["matrix"] is set),
either replace proc["matrix"] with the matrix conjugated by a SWAP on the two
sites (i.e. apply the index-swap transform) or explicitly raise if the provided
BaseGate is asymmetric and cannot be reordered; reference the variables proc,
name_op, swapped and where proc["matrix"] is assigned. For long-range cases
where proc["factors"] is provided, when swapped is True reorder the factors
(e.g. proc["factors"] = tuple(reversed(proc["factors"])) ) so factor↔site
alignment is preserved. Add a unit test that constructs a non-Crosstalk
asymmetric 2-site gate (e.g. CX-like) with unsorted sites and asserts the stored
proc["sites"] and proc operator data remain correctly aligned after
normalization.
In `@src/mqt/yaqs/core/libraries/gate_library.py`:
- Around line 141-166: The constructor BaseGate.__init__ currently computes
interaction = np.log(n) / np.log(local_dim) which misbehaves for local_dim <= 1;
add an explicit guard near the start of __init__ to validate local_dim >= 2 and
raise a ValueError with a clear message (e.g. "local_dim must be >= 2") before
computing logs; then proceed with the existing square-check and interaction
computation, leaving variables mat, n, interaction, self.local_dim, self.matrix,
self.tensor, and self.interaction unchanged.
- Around line 1534-1559: The constructor of Crosstalk must validate that both
operands are single-site gates: in Crosstalk.__init__ check gate1.interaction ==
1 and gate2.interaction == 1 (or equivalently that their interaction attribute
equals 1) before computing matrix1/matrix2/swapped_matrix and calling
super().__init__; if either operand is not single-site, raise a ValueError with
a clear message mentioning gate1.name/gate2.name and their interaction values so
callers can't pass multi-site gates like XX or CX that would produce invalid
factors for NoiseModel.
In `@src/mqt/yaqs/core/methods/dissipation.py`:
- Around line 61-103: PAULI fast-path misses common aliases and the Crosstalk
base class; update _PAULI_STRINGS and _PAULI_CLASSES and is_pauli to derive
aliases and classes programmatically: query GateLibrary for aliases that map to
Pauli operators (e.g. "x","y","z","dephasing") and add those names into
_PAULI_STRINGS, and include the Crosstalk base class (Crosstalk) in
_PAULI_CLASSES (or build the tuple from the Pauli classes plus Crosstalk) so
is_pauli(proc) (which inspects proc["name"]) recognizes both alias strings and
composite Crosstalk instances via isinstance(..., _PAULI_CLASSES).
In `@tests/analog/test_lindblad.py`:
- Around line 140-143: The test defines noise_processes with name="dephasing"
which is not in _PAULI_STRINGS so dissipation.is_pauli(proc) returns False and
the Pauli fast path is not exercised; update the test to either use a
Pauli-designated name (e.g., change the process name to one present in
_PAULI_STRINGS) or add an explicit assertion that dissipation.is_pauli(proc) is
True for the created proc (or add a sibling test) so the fast-path in
dissipation.py (is_pauli, _PAULI_STRINGS) is actually executed for the sigma_z
operator referenced in noise_processes.
In `@tests/core/data_structures/test_noise_model.py`:
- Around line 171-178: The test
test_longrange_unknown_label_without_factors_raises should stop catching broad
exception types and instead assert the specific error and message from failing
to resolve a long-range label via GateLibrary/get_operator; change the
pytest.raises call to expect a specific exception (e.g., ValueError or KeyError
raised by NoiseModel.__init__/GateLibrary.get_operator) and include a match
argument that checks for the stable substring like "unknown gate" or "could not
resolve 'foo_bar'" so the test only passes when the name-resolution failure
occurs rather than any unrelated AttributeError.
🪄 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: b279b9cd-ac62-4c69-90dd-39415d16f219
📒 Files selected for processing (6)
.gitignoresrc/mqt/yaqs/core/data_structures/noise_model.pysrc/mqt/yaqs/core/libraries/gate_library.pysrc/mqt/yaqs/core/methods/dissipation.pytests/analog/test_lindblad.pytests/core/data_structures/test_noise_model.py
Comment on lines
+102
to
+123
| else: # Two-site: normalize site ordering | ||
| sorted_sites = sorted(sites) | ||
| if sorted_sites != sites: | ||
| swapped = sorted_sites != sites | ||
| if swapped: | ||
| proc["sites"] = sorted_sites | ||
| i, j = proc["sites"] | ||
| is_adjacent = abs(j - i) == 1 | ||
|
|
||
| # Adjacent two-site: use full matrix | ||
| if is_adjacent: | ||
| if str(name).startswith("crosstalk_"): | ||
| # infer matrix from suffix ab | ||
| suffix = str(name).rsplit("_", 1)[-1] | ||
| assert len(suffix) == 2, "Invalid crosstalk label. Expected 'crosstalk_ab' with a,b in {x,y,z}." | ||
| assert all(c in "xyz" for c in suffix), ( | ||
| "Invalid crosstalk label. Expected 'crosstalk_ab' with a,b in {x,y,z}." | ||
| ) | ||
| a, b = suffix[0], suffix[1] | ||
| proc["matrix"] = np.kron(PAULI_MAP[a], PAULI_MAP[b]) | ||
| elif "matrix" not in proc: | ||
| proc["matrix"] = NoiseModel.get_operator(name) | ||
| filled_processes.append(proc) | ||
| continue | ||
|
|
||
| # Non-adjacent two-site: attach per-site factors for crosstalk labels | ||
| if str(name).startswith("crosstalk_"): | ||
| if "factors" not in proc: | ||
| suffix = str(name).rsplit("_", 1)[-1] | ||
| assert len(suffix) == 2, "Invalid crosstalk label. Expected 'crosstalk_ab' with a,b in {x,y,z}." | ||
| assert all(c in "xyz" for c in suffix), ( | ||
| "Invalid crosstalk label. Expected 'crosstalk_ab' with a,b in {x,y,z}." | ||
| ) | ||
| a, b = suffix[0], suffix[1] | ||
| proc["factors"] = (PAULI_MAP[a], PAULI_MAP[b]) | ||
| filled_processes.append(proc) | ||
| continue | ||
|
|
||
| # Long-range two-site with canonical legacy label | ||
| if str(name).startswith(CROSSTALK_PREFIX): | ||
| if "factors" not in proc: | ||
| suffix = str(name).rsplit("_", 1)[-1] | ||
| assert len(suffix) == 2, ( | ||
| f"Invalid crosstalk label. Expected '{CROSSTALK_PREFIX}ab' with a,b in {{x,y,z}}." | ||
| ) | ||
| assert all(c in "xyz" for c in suffix), ( | ||
| f"Invalid crosstalk label. Expected '{CROSSTALK_PREFIX}ab' with a,b in {{x,y,z}}." | ||
| ) | ||
| a, b = suffix[0], suffix[1] | ||
| proc["factors"] = (PAULI_MAP[a], PAULI_MAP[b]) | ||
| filled_processes.append(proc) | ||
| continue | ||
|
|
||
| # Other long-range two-site: require explicit factors | ||
| assert "factors" in proc, ( | ||
| "Non-adjacent 2-site processes must specify 'factors' unless named 'crosstalk_{ab}'." | ||
| ) | ||
| filled_processes.append(proc) | ||
| continue | ||
|
|
||
| # One-site: ensure matrix | ||
| if "matrix" not in proc: | ||
| proc["matrix"] = NoiseModel.get_operator(name) | ||
|
|
||
| if abs(j - i) == 1: # Adjacent: store full matrix | ||
| if isinstance(name_op, Crosstalk): | ||
| proc["matrix"] = name_op.swapped_matrix if swapped else name_op.matrix | ||
| else: | ||
| proc["matrix"] = name_op.matrix | ||
|
|
||
| elif isinstance(name_op, Crosstalk): | ||
| proc["factors"] = ( | ||
| (name_op.matrix2, name_op.matrix1) if swapped else (name_op.matrix1, name_op.matrix2) | ||
| ) | ||
| else: | ||
| assert "factors" in proc, ( | ||
| "Non-adjacent 2-site processes must specify 'factors' unless a Crosstalk gate is provided." | ||
| ) | ||
|
|
Contributor
There was a problem hiding this comment.
Swap fix only covers Crosstalk — non-Crosstalk 2-site gates still hit the original bug.
The PR description states the constructor "reordered indices into ascending order but did not reorder the corresponding operators accordingly." The new code fixes this only when name_op is a Crosstalk instance:
- Line 113 (adjacent, non-
Crosstalk):proc["matrix"] = name_op.matrixis stored without reordering when sites were swapped. For any asymmetric 2-siteBaseGatepassed by name or as aBaseGateinstance (e.g., a custom gate, orCX), this reproduces exactly the bug this PR claims to fix. - Lines 119–122 (long-range, non-
Crosstalk): when the user supplies their own"factors"with unsortedsites, the factors are not swapped either —proc["sites"]becomes ascending butproc["factors"]keeps the original order, so factor ↔ site alignment is broken.
At minimum the long-range explicit-factors case must reorder factors when swapped; the adjacent non-Crosstalk case should either swap via SWAP @ M @ SWAP or reject asymmetric 2-site operators here.
🛡️ Proposed fix for the explicit-factors case
elif isinstance(name_op, Crosstalk):
proc["factors"] = (
(name_op.matrix2, name_op.matrix1) if swapped else (name_op.matrix1, name_op.matrix2)
)
else:
assert "factors" in proc, (
"Non-adjacent 2-site processes must specify 'factors' unless a Crosstalk gate is provided."
)
+ if swapped:
+ f0, f1 = proc["factors"]
+ proc["factors"] = (f1, f0)Please also add a test covering a non-Crosstalk, asymmetric adjacent 2-site operator with unsorted sites to lock this in.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mqt/yaqs/core/data_structures/noise_model.py` around lines 102 - 123, The
code only adjusts ordering for Crosstalk but leaves non-Crosstalk 2-site
operators misaligned: when you detect swapped = sorted(sites) != sites you must
also reorder the operator data to match the new site order. For adjacent
non-Crosstalk cases (where proc["matrix"] is set), either replace proc["matrix"]
with the matrix conjugated by a SWAP on the two sites (i.e. apply the index-swap
transform) or explicitly raise if the provided BaseGate is asymmetric and cannot
be reordered; reference the variables proc, name_op, swapped and where
proc["matrix"] is assigned. For long-range cases where proc["factors"] is
provided, when swapped is True reorder the factors (e.g. proc["factors"] =
tuple(reversed(proc["factors"])) ) so factor↔site alignment is preserved. Add a
unit test that constructs a non-Crosstalk asymmetric 2-site gate (e.g. CX-like)
with unsorted sites and asserts the stored proc["sites"] and proc operator data
remain correctly aligned after normalization.
Comment on lines
202
to
+217
| @staticmethod | ||
| def get_operator(name: str) -> NDArray[np.complex128]: | ||
| """Retrieve the operator from NoiseLibrary, possibly as a tensor product if needed. | ||
| def get_operator(name: str | BaseGate) -> BaseGate: | ||
| """Retrieve the operator from GateLibrary, possibly as a tensor product if needed. | ||
|
|
||
| Args: | ||
| name: Name of the noise process (e.g., 'xx', 'zz'). | ||
|
|
||
| Returns: | ||
| np.ndarray: The matrix representation of the operator. | ||
| BaseGate: The matrix representation of the operator. | ||
| """ | ||
| if name in PAULI_MAP: | ||
| return PAULI_MAP[name] | ||
| operator_class = getattr(NoiseLibrary, name) | ||
| if isinstance(name, BaseGate): | ||
| return name | ||
|
|
||
| operator_class = getattr(GateLibrary, name) | ||
|
|
||
| operator: BaseGate = operator_class() | ||
| return operator.matrix | ||
| return operator_class() |
Contributor
There was a problem hiding this comment.
get_operator will fail for parameterized / arg-taking gate classes.
operator_class() unconditionally instantiates with no arguments. GateLibrary contains classes that require arguments (e.g., rx, ry, rz, u, u2, cp, p, pvm) as well as destroy/create which have optional d. If a user ever passes one of those names (or a future alias like raising_three) they will get a confusing TypeError rather than a clear diagnostic. Consider either catching the TypeError and raising a more descriptive error, or restricting get_operator to names known to be parameter-free.
Also, the docstring’s Returns: BaseGate: The matrix representation of the operator. is stale — it now returns a BaseGate, not a matrix. As per coding guidelines (Google-style docstrings in Python code), please correct the description.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mqt/yaqs/core/data_structures/noise_model.py` around lines 202 - 217,
get_operator currently calls operator_class() without arguments which will raise
a confusing TypeError for parameterized GateLibrary entries (e.g., rx, ry, rz,
u, u2, cp, p, pvm, destroy/create); update get_operator to either catch
TypeError when instantiating operator_class and raise a clear ValueError
describing that the named gate requires parameters (include the gate name) or
validate against a whitelist of parameter-free gate names before instantiation;
also fix the docstring return description to state it returns a BaseGate (not a
matrix) and adjust the docstring summary accordingly.
Comment on lines
+141
to
+166
| def __init__(self, mat: NDArray[np.complex128], local_dim: int = 2) -> None: | ||
| """Initializes a BaseGate instance with the given matrix. | ||
|
|
||
| Args: | ||
| mat: The matrix representation of the gate. | ||
| local_dim: Local Hilbert space dimension per site (e.g. 2 for qubits, 3 for qutrits). | ||
|
|
||
| Raises: | ||
| ValueError: If the matrix is not square. | ||
| ValueError: If the matrix size is not a power of 2. | ||
| ValueError: If the matrix size is not a power of local_dim. | ||
| """ | ||
| if mat.shape[0] != mat.shape[1]: | ||
| msg = "Matrix must be square" | ||
| raise ValueError(msg) | ||
|
|
||
| log = np.log2(mat.shape[0]) | ||
| n = mat.shape[0] | ||
|
|
||
| interaction = np.log(n) / np.log(local_dim) | ||
| if not np.isclose(interaction, round(interaction)): | ||
| msg = f"Matrix size {n} is not a power of {local_dim}" | ||
| raise ValueError(msg) | ||
|
|
||
| self.local_dim = local_dim | ||
| self.matrix = mat | ||
| self.tensor = mat | ||
| self.interaction = int(log) | ||
| self.interaction = round(interaction) |
Contributor
There was a problem hiding this comment.
Validate local_dim >= 2 to avoid NaN/division-by-zero.
np.log(local_dim) is 0 when local_dim == 1 and undefined for local_dim <= 0, so the ratio becomes NaN/inf, bypasses the np.isclose check incorrectly or yields the confusing message "Matrix size N is not a power of 1" for any matrix. Add an explicit guard so the error is clear and deterministic.
🛡️ Proposed fix
if mat.shape[0] != mat.shape[1]:
msg = "Matrix must be square"
raise ValueError(msg)
+ if local_dim < 2:
+ msg = f"local_dim must be >= 2, got {local_dim}"
+ raise ValueError(msg)
+
n = mat.shape[0]
interaction = np.log(n) / np.log(local_dim)Note: Destroy/Create assert d > 0 (lines 758, 786) then pass local_dim=d, so d=1 currently produces a 1×1 zero matrix and a misleading error inside BaseGate. Tightening to d >= 2 (or converting the assertion to a ValueError, since asserts are stripped under python -O) is recommended.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mqt/yaqs/core/libraries/gate_library.py` around lines 141 - 166, The
constructor BaseGate.__init__ currently computes interaction = np.log(n) /
np.log(local_dim) which misbehaves for local_dim <= 1; add an explicit guard
near the start of __init__ to validate local_dim >= 2 and raise a ValueError
with a clear message (e.g. "local_dim must be >= 2") before computing logs; then
proceed with the existing square-check and interaction computation, leaving
variables mat, n, interaction, self.local_dim, self.matrix, self.tensor, and
self.interaction unchanged.
Comment on lines
+1534
to
+1559
| class Crosstalk(BaseGate): | ||
| """Class representing the Kronecker product of two single-site gates.""" | ||
|
|
||
| name = "crosstalk" | ||
|
|
||
| def __init__(self, gate1: BaseGate, gate2: BaseGate) -> None: | ||
| """Initializes the Kronecker product gate. | ||
|
|
||
| Args: | ||
| gate1: Left operand gate. | ||
| gate2: Right operand gate. | ||
|
|
||
| Raises: | ||
| ValueError: If the gates have different local dimensions. | ||
| """ | ||
| if gate1.local_dim != gate2.local_dim: | ||
| msg = f"Crosstalk gates must have the same local dimension, got {gate1.local_dim} and {gate2.local_dim}" | ||
| raise ValueError(msg) | ||
|
|
||
| self.name = f"crosstalk_{gate1.name}_{gate2.name}" | ||
| self.matrix1 = gate1.matrix | ||
| self.matrix2 = gate2.matrix | ||
| self.swapped_matrix = np.kron(gate2.matrix, gate1.matrix).astype(np.complex128) | ||
|
|
||
| mat = np.kron(gate1.matrix, gate2.matrix).astype(np.complex128) | ||
| super().__init__(mat, local_dim=gate1.local_dim) |
Contributor
There was a problem hiding this comment.
Validate inputs to Crosstalk.
Crosstalk is meant for Kronecker products of single-site gates, but there is no check that gate1.interaction == 1 and gate2.interaction == 1. Passing a 2-site gate (e.g., XX, CX) silently stores a 4×4 matrix in matrix1/matrix2, producing invalid "factors" when NoiseModel later destructures them for long-range processes, and a 16×16 matrix that will still pass the local_dim power check.
🛡️ Proposed validation
if gate1.local_dim != gate2.local_dim:
msg = f"Crosstalk gates must have the same local dimension, got {gate1.local_dim} and {gate2.local_dim}"
raise ValueError(msg)
+ if gate1.interaction != 1 or gate2.interaction != 1:
+ msg = (
+ f"Crosstalk expects two single-site gates (interaction=1), "
+ f"got interaction={gate1.interaction} and {gate2.interaction}"
+ )
+ raise ValueError(msg)
+
self.name = f"crosstalk_{gate1.name}_{gate2.name}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mqt/yaqs/core/libraries/gate_library.py` around lines 1534 - 1559, The
constructor of Crosstalk must validate that both operands are single-site gates:
in Crosstalk.__init__ check gate1.interaction == 1 and gate2.interaction == 1
(or equivalently that their interaction attribute equals 1) before computing
matrix1/matrix2/swapped_matrix and calling super().__init__; if either operand
is not single-site, raise a ValueError with a clear message mentioning
gate1.name/gate2.name and their interaction values so callers can't pass
multi-site gates like XX or CX that would produce invalid factors for
NoiseModel.
Comment on lines
+61
to
+103
| _PAULI_STRINGS = { | ||
| "pauli_x", | ||
| "pauli_y", | ||
| "pauli_z", | ||
| "crosstalk_xx", | ||
| "crosstalk_yy", | ||
| "crosstalk_zz", | ||
| "crosstalk_xy", | ||
| "crosstalk_yx", | ||
| "crosstalk_zy", | ||
| "crosstalk_zx", | ||
| "crosstalk_yz", | ||
| "crosstalk_xz", | ||
| "longrange_crosstalk_xx", | ||
| "longrange_crosstalk_yy", | ||
| "longrange_crosstalk_zz", | ||
| "longrange_crosstalk_xy", | ||
| "longrange_crosstalk_yx", | ||
| "longrange_crosstalk_zy", | ||
| "longrange_crosstalk_zx", | ||
| "longrange_crosstalk_yz", | ||
| "longrange_crosstalk_xz", | ||
| } | ||
| _PAULI_CLASSES = ( | ||
| X, | ||
| Y, | ||
| Z, | ||
| CrosstalkXX, | ||
| CrosstalkYY, | ||
| CrosstalkZZ, | ||
| CrosstalkXY, | ||
| CrosstalkYX, | ||
| CrosstalkZY, | ||
| CrosstalkZX, | ||
| CrosstalkYZ, | ||
| CrosstalkXZ, | ||
| ) | ||
|
|
||
|
|
||
| def is_pauli(proc: dict[str, Any]) -> bool: | ||
| """Return True if the process is a Pauli process.""" | ||
| return bool( | ||
| proc["name"] | ||
| in { | ||
| "pauli_x", | ||
| "pauli_y", | ||
| "pauli_z", | ||
| "crosstalk_xx", | ||
| "crosstalk_yy", | ||
| "crosstalk_zz", | ||
| "crosstalk_xy", | ||
| "crosstalk_yx", | ||
| "crosstalk_zy", | ||
| "crosstalk_zx", | ||
| "crosstalk_yz", | ||
| "crosstalk_xz", | ||
| } | ||
| ) | ||
| name = proc["name"] | ||
| return name in _PAULI_STRINGS or isinstance(name, _PAULI_CLASSES) |
Contributor
There was a problem hiding this comment.
_PAULI_STRINGS and _PAULI_CLASSES are incomplete — Pauli fast path silently bypassed for common aliases.
GateLibrary exposes additional aliases that resolve to Pauli operators: x, y, z, and dephasing (= Z). None of these are in _PAULI_STRINGS, so is_pauli(proc) returns False for them and the dissipator falls through to the generic expm(-0.5 * dt * γ * A†A) branch. Mathematically this still happens to produce the correct result (because σ†σ = I for Pauli), but:
test_lindblad_dephasing_both_qubitsintests/analog/test_lindblad.pyuses"name": "dephasing"— the intended fast path is not exercised at all.- The behavior is inconsistent:
"pauli_x"uses the fast path but"x"does not, even though both resolve to the same gate class. _PAULI_CLASSESalso omits theCrosstalkbase class, so a user-constructedCrosstalk(X(), Y())instance passed asproc["name"](now supported byNoiseModel.get_operator) won’t be recognized even though its concrete subclasses would be.
Consider deriving the set programmatically from GateLibrary aliases and including Crosstalk in the class tuple.
🛠️ Suggested consolidation
+from ..libraries.gate_library import Crosstalk
from ..libraries.gate_library import (
CrosstalkXX,
...
X, Y, Z,
)
...
_PAULI_STRINGS = {
- "pauli_x", "pauli_y", "pauli_z",
+ "pauli_x", "pauli_y", "pauli_z",
+ "x", "y", "z", "dephasing",
"crosstalk_xx", ...
}
_PAULI_CLASSES = (
- X, Y, Z,
+ X, Y, Z, Crosstalk,
CrosstalkXX, ...
)(Adding Crosstalk makes the concrete-subclass entries redundant but harmless.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mqt/yaqs/core/methods/dissipation.py` around lines 61 - 103, PAULI
fast-path misses common aliases and the Crosstalk base class; update
_PAULI_STRINGS and _PAULI_CLASSES and is_pauli to derive aliases and classes
programmatically: query GateLibrary for aliases that map to Pauli operators
(e.g. "x","y","z","dephasing") and add those names into _PAULI_STRINGS, and
include the Crosstalk base class (Crosstalk) in _PAULI_CLASSES (or build the
tuple from the Pauli classes plus Crosstalk) so is_pauli(proc) (which inspects
proc["name"]) recognizes both alias strings and composite Crosstalk instances
via isinstance(..., _PAULI_CLASSES).
Comment on lines
140
to
143
| noise_processes = [ | ||
| {"name": "dephasing_0", "sites": [0], "strength": gamma, "matrix": sigma_z}, | ||
| {"name": "dephasing_1", "sites": [1], "strength": gamma, "matrix": sigma_z}, | ||
| {"name": "dephasing", "sites": [0], "strength": gamma, "matrix": sigma_z}, | ||
| {"name": "dephasing", "sites": [1], "strength": gamma, "matrix": sigma_z}, | ||
| ] |
Contributor
There was a problem hiding this comment.
Test does not exercise the Pauli fast path despite using a dephasing operator.
With name="dephasing", is_pauli(proc) returns False (see comment in dissipation.py — "dephasing" is not in _PAULI_STRINGS), so the dissipator goes through the generic expm branch. Numerically this still matches because Z²=I, but the test is not validating what its name implies. Once _PAULI_STRINGS is extended, this test will transparently start covering the fast path; until then, consider adding an assertion (or a sibling test) that pins which code path is exercised.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/analog/test_lindblad.py` around lines 140 - 143, The test defines
noise_processes with name="dephasing" which is not in _PAULI_STRINGS so
dissipation.is_pauli(proc) returns False and the Pauli fast path is not
exercised; update the test to either use a Pauli-designated name (e.g., change
the process name to one present in _PAULI_STRINGS) or add an explicit assertion
that dissipation.is_pauli(proc) is True for the created proc (or add a sibling
test) so the fast-path in dissipation.py (is_pauli, _PAULI_STRINGS) is actually
executed for the sigma_z operator referenced in noise_processes.
Comment on lines
171
to
178
| def test_longrange_unknown_label_without_factors_raises() -> None: | ||
| """Test that unknown long-range labels without 'factors' raise. | ||
|
|
||
| If the name is not 'longrange_crosstalk_{ab}' and no factors are provided, | ||
| If the name is not a recognized GateLibrary alias and no factors are provided, | ||
| initialization must fail to avoid guessing operators. | ||
|
|
||
| Raises: | ||
| AssertionError: If the model accepts an unknown long-range label without factors. | ||
| """ | ||
| try: | ||
| # Name is not a recognized non-adjacent 'crosstalk_{ab}' and no factors provided | ||
| with pytest.raises((AttributeError, AssertionError)): | ||
| _ = NoiseModel([{"name": "foo_bar", "sites": [0, 2], "strength": 0.1}]) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Broadened exception expectation can hide regressions.
pytest.raises((AttributeError, AssertionError)) will pass for any AttributeError raised anywhere in NoiseModel.__init__ — including an unrelated bug (e.g., a renamed attribute access). Since the intent is "unknown name cannot be resolved by GateLibrary", consider matching on a substring to lock the contract:
- with pytest.raises((AttributeError, AssertionError)):
+ with pytest.raises((AttributeError, AssertionError), match="foo_bar|factors"):
_ = NoiseModel([{"name": "foo_bar", "sites": [0, 2], "strength": 0.1}])Longer-term, having get_operator raise a dedicated KeyError/ValueError with a stable message would make this assertion more robust.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/data_structures/test_noise_model.py` around lines 171 - 178, The
test test_longrange_unknown_label_without_factors_raises should stop catching
broad exception types and instead assert the specific error and message from
failing to resolve a long-range label via GateLibrary/get_operator; change the
pytest.raises call to expect a specific exception (e.g., ValueError or KeyError
raised by NoiseModel.__init__/GateLibrary.get_operator) and include a match
argument that checks for the stable substring like "unknown gate" or "could not
resolve 'foo_bar'" so the test only passes when the name-resolution failure
occurs rather than any unrelated AttributeError.
- Correct Destroy/Create matrix size from 2x2 to d×d - Fix XX/YY/ZZ: replace stale `mpo` attribute with `mpo_tensors` note - Add full Attributes section to Crosstalk class - Correct CZ generator comment (I-X → I-Z) - Expand set_sites docstrings in two-qubit gates to describe full effect - Add missing aliases to GateLibrary Attributes section Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/mqt/yaqs/core/libraries/gate_library.py (2)
141-166:⚠️ Potential issue | 🟡 Minor
local_dim < 2still lets matrices pass with a confusing error.When
local_dim == 1,np.log(local_dim) == 0causes a divide-by-zero → the ratio becomesinf/NaN, sonp.isclose(interaction, round(interaction))evaluates unpredictably and the subsequent "Matrix size N is not a power of 1" message is misleading.local_dim <= 0is also not rejected. SinceDestroy/Createonlyassert d > 0(stripped underpython -O) and forwardlocal_dim=d, a caller usingd=1lands here instead of getting a clean error. A guardif local_dim < 2: raise ValueError(...)before the log is worth adding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mqt/yaqs/core/libraries/gate_library.py` around lines 141 - 166, Add a guard at the start of BaseGate.__init__ to reject invalid local_dim values: check if local_dim < 2 and raise ValueError with a clear message (e.g. "local_dim must be an integer >= 2"); this prevents divide-by-zero when computing interaction = np.log(n)/np.log(local_dim) and avoids misleading "power of" errors when callers pass 1 or non-positive values (the relevant symbols are BaseGate.__init__, local_dim, interaction).
1546-1582:⚠️ Potential issue | 🟡 MinorMissing
interaction == 1validation forCrosstalkoperands.
Crosstalkis documented as a Kronecker product of two single-site gates, but onlylocal_dimequality is checked. Passing a 2-site gate (e.g.,XX,CX) silently stores 4×4 factors inmatrix1/matrix2and produces a 16×16matrixthat still passes thelocal_dim-power check inBaseGate— which downstreamNoiseModelcode would then mis-destructure into "factors". Add a guard that both operands haveinteraction == 1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mqt/yaqs/core/libraries/gate_library.py` around lines 1546 - 1582, Crosstalk currently only validates local_dim but must also ensure both operands are single-site; in the Crosstalk.__init__ add a guard that gate1.interaction == 1 and gate2.interaction == 1 and raise a ValueError with a clear message if not; reference the Crosstalk class and its attributes matrix1/matrix2 as the place to validate, so we never accept multi-site gates (e.g., XX, CX) that would produce mis-sized Kronecker products downstream.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mqt/yaqs/core/libraries/gate_library.py`:
- Around line 1657-1686: The Crosstalk ladder-operator aliases hardcode local
dimension d=2; update CrosstalkDestroyDestroy, CrosstalkCreateCreate,
CrosstalkDestroyCreate, and CrosstalkCreateDestroy to accept a local-dimension
parameter (e.g., def __init__(self, d: int = 2)) and pass that d into the
underlying Destroy(d) / Create(d) constructors instead of using the default, so
these GateLibrary aliases work for higher local dimensions; keep the existing
default of 2 to preserve backwards compatibility and update the constructor
docstrings to note the d parameter.
- Line 741: Replace the Unicode multiplication sign (×) used in the docstrings
with an ASCII "x" to satisfy ruff RUF002: update the docstring for the Destroy,
Create, and Crosstalk docstrings so "d×d" becomes "d x d" (and "d²×d²" becomes
"d^2 x d^2" or "d² x d²" using an ASCII "x"), modifying the docstring text in
the Destroy, Create, and Crosstalk definitions to use the ASCII x character
consistently.
- Around line 814-834: The Zero gate's matrix in Zero.__init__ is created as an
integer ndarray (mat = np.array([[0,0],[0,0]])) which is inconsistent with other
complex-typed placeholder/measurement gates; change the mat creation to
explicitly use a complex dtype (e.g., use np.zeros((2,2), dtype=complex) or
np.array([...], dtype=complex)) before calling super().__init__(mat) so the
gate's matrix dtype matches other gates and avoids subtle promotion issues.
---
Duplicate comments:
In `@src/mqt/yaqs/core/libraries/gate_library.py`:
- Around line 141-166: Add a guard at the start of BaseGate.__init__ to reject
invalid local_dim values: check if local_dim < 2 and raise ValueError with a
clear message (e.g. "local_dim must be an integer >= 2"); this prevents
divide-by-zero when computing interaction = np.log(n)/np.log(local_dim) and
avoids misleading "power of" errors when callers pass 1 or non-positive values
(the relevant symbols are BaseGate.__init__, local_dim, interaction).
- Around line 1546-1582: Crosstalk currently only validates local_dim but must
also ensure both operands are single-site; in the Crosstalk.__init__ add a guard
that gate1.interaction == 1 and gate2.interaction == 1 and raise a ValueError
with a clear message if not; reference the Crosstalk class and its attributes
matrix1/matrix2 as the place to validate, so we never accept multi-site gates
(e.g., XX, CX) that would produce mis-sized Kronecker products downstream.
🪄 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: 6e951ac0-3c8c-4fe1-a646-859fcd9f0016
📒 Files selected for processing (1)
src/mqt/yaqs/core/libraries/gate_library.py
Comment on lines
+814
to
+834
| class Zero(BaseGate): | ||
| """Class representing the Zero gate. | ||
|
|
||
| Attributes: | ||
| name: The name of the gate ("zero"). | ||
| matrix: The 2x2 zero matrix. | ||
| interaction: The interaction level (1 for single-qubit gates). | ||
| tensor: The tensor representation of the gate (same as the matrix). | ||
|
|
||
| Methods: | ||
| set_sites(*sites: int) -> None: | ||
| Sets the site(s) where the gate is applied. | ||
| """ | ||
|
|
||
| name = "zero" | ||
|
|
||
| def __init__(self) -> None: | ||
| """Initializes the Zero gate.""" | ||
| mat = np.array([[0, 0], [0, 0]]) | ||
| super().__init__(mat) | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Zero matrix has integer dtype.
np.array([[0, 0], [0, 0]]) produces an int64 matrix, unlike most other 2×2 gates here that are fine with int but unlike the complex-typed placeholders (P0, P1, RuntimeCost, ...). If Zero is used as a noise operator and later combined with complex matrices (e.g., in np.kron, dissipator terms, or Crosstalk), NumPy will promote correctly, but dtype inconsistency can cause subtle surprises (e.g., when callers set elements in-place, or when comparing dtypes). Consider dtype=complex for consistency with the measurement/placeholder gates.
♻️ Proposed fix
- mat = np.array([[0, 0], [0, 0]])
+ mat = np.array([[0, 0], [0, 0]], dtype=complex)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mqt/yaqs/core/libraries/gate_library.py` around lines 814 - 834, The Zero
gate's matrix in Zero.__init__ is created as an integer ndarray (mat =
np.array([[0,0],[0,0]])) which is inconsistent with other complex-typed
placeholder/measurement gates; change the mat creation to explicitly use a
complex dtype (e.g., use np.zeros((2,2), dtype=complex) or np.array([...],
dtype=complex)) before calling super().__init__(mat) so the gate's matrix dtype
matches other gates and avoids subtle promotion issues.
Comment on lines
+1657
to
+1686
| class CrosstalkDestroyDestroy(Crosstalk): | ||
| """Crosstalk Destroy ⊗ Destroy.""" | ||
|
|
||
| def __init__(self) -> None: | ||
| """Initializes Destroy ⊗ Destroy crosstalk.""" | ||
| super().__init__(Destroy(), Destroy()) | ||
|
|
||
|
|
||
| class CrosstalkCreateCreate(Crosstalk): | ||
| """Crosstalk Create ⊗ Create.""" | ||
|
|
||
| def __init__(self) -> None: | ||
| """Initializes Create ⊗ Create crosstalk.""" | ||
| super().__init__(Create(), Create()) | ||
|
|
||
|
|
||
| class CrosstalkDestroyCreate(Crosstalk): | ||
| """Crosstalk Destroy ⊗ Create.""" | ||
|
|
||
| def __init__(self) -> None: | ||
| """Initializes Destroy ⊗ Create crosstalk.""" | ||
| super().__init__(Destroy(), Create()) | ||
|
|
||
|
|
||
| class CrosstalkCreateDestroy(Crosstalk): | ||
| """Crosstalk Create ⊗ Destroy.""" | ||
|
|
||
| def __init__(self) -> None: | ||
| """Initializes Create ⊗ Destroy crosstalk.""" | ||
| super().__init__(Create(), Destroy()) |
Contributor
There was a problem hiding this comment.
Ladder-operator Crosstalk aliases hardcode d=2.
CrosstalkDestroyDestroy, CrosstalkCreateCreate, CrosstalkDestroyCreate, and CrosstalkCreateDestroy instantiate Destroy()/Create() with the default d=2, so the raising_two/lowering_two GateLibrary aliases cannot be reused for higher local dimensions (qutrits, bosonic modes). Since the rest of the changes make local_dim first-class throughout BaseGate, consider parametrizing these subclasses by d (e.g., def __init__(self, d: int = 2): super().__init__(Destroy(d), Destroy(d))), or document the limitation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mqt/yaqs/core/libraries/gate_library.py` around lines 1657 - 1686, The
Crosstalk ladder-operator aliases hardcode local dimension d=2; update
CrosstalkDestroyDestroy, CrosstalkCreateCreate, CrosstalkDestroyCreate, and
CrosstalkCreateDestroy to accept a local-dimension parameter (e.g., def
__init__(self, d: int = 2)) and pass that d into the underlying Destroy(d) /
Create(d) constructors instead of using the default, so these GateLibrary
aliases work for higher local dimensions; keep the existing default of 2 to
preserve backwards compatibility and update the constructor docstrings to note
the d parameter.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Ruff RUF002 flags U+00D7 (×) as ambiguous. Replace d×d and d²×d² with dxd and (d^2)x(d^2) in Destroy, Create, and Crosstalk docstrings. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
New tests cover: - BaseGate: non-power-of-local_dim ValueError, list-input set_sites, @ operator, factory classmethods (destroy, zero, crosstalk, p0, p1, pvm) - mpo_tensors property AttributeError before set_sites is called - Zero, XX, YY, ZZ, P0, P1, PVM gate constructors - Crosstalk: matrix/swapped_matrix, mismatched-local_dim ValueError, all 13 named subclasses - Two-qubit gates (cx, cz, swap, rxx, ryy, rzz, cp): list-input set_sites, wrong-site-count ValueError, CX reversed-order tensor transpose - Entropy/SchmidtSpectrum set_sites list-input branch Remaining 3% (lines 99-114) is dead code in extend_gate's elif len(tensors) == 3 branch, which split_tensor never triggers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…00%) Three new tests target the three previously uncovered branches: - Adjacent two-site non-Crosstalk gate uses name_op.matrix directly (line 113) - Non-adjacent non-Crosstalk process with pre-supplied factors passes assert (line 120) - get_operator returns a BaseGate instance unchanged when passed directly (line 213) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mqt/yaqs/core/libraries/gate_library.py (1)
192-266:⚠️ Potential issue | 🟠 MajorApply the same compatibility checks to
@as to gate multiplication.Line 266 allows any shape-compatible matrices, so a 4x4 single-site qudit gate (
local_dim=4,interaction=1) can be@-multiplied with a 4x4 two-qubit gate (local_dim=2,interaction=2) and silently returned as a two-qubit gate. Check bothinteractionandlocal_dimbefore all binary gate operations.🛡️ Proposed compatibility helper
+ def _ensure_compatible_gate(self, other: BaseGate, operation: str) -> None: + if self.interaction != other.interaction or self.local_dim != other.local_dim: + msg = ( + f"Cannot {operation} gates with different interaction/local_dim " + f"({self.interaction}, {self.local_dim}) and ({other.interaction}, {other.local_dim})" + ) + raise ValueError(msg) + def __add__(self, other: BaseGate) -> BaseGate: @@ - if self.interaction != other.interaction: - msg = "Cannot add gates with different interaction" - raise ValueError(msg) + self._ensure_compatible_gate(other, "add") return BaseGate(self.matrix + other.matrix, local_dim=self.local_dim) @@ - if self.interaction != other.interaction: - msg = "Cannot subtract gates with different interaction" - raise ValueError(msg) + self._ensure_compatible_gate(other, "subtract") return BaseGate(self.matrix - other.matrix, local_dim=self.local_dim) @@ if isinstance(other, BaseGate): - if self.interaction != other.interaction: - msg = "Cannot multiply gates with different interaction" - raise ValueError(msg) + self._ensure_compatible_gate(other, "multiply") return BaseGate(self.matrix @ other.matrix, local_dim=self.local_dim) @@ def __matmul__(self, other: BaseGate) -> BaseGate: @@ - return BaseGate(self.matrix @ other.matrix, local_dim=self.local_dim) + self._ensure_compatible_gate(other, "matrix-multiply") + return BaseGate(self.matrix @ other.matrix, local_dim=self.local_dim)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mqt/yaqs/core/libraries/gate_library.py` around lines 192 - 266, The __matmul__ implementation currently does no compatibility checks and can combine gates with mismatched interaction or local_dim (e.g., BaseGate objects with differing interaction or local_dim), so update __matmul__ to mirror the checks used in __mul__/__add__/__sub__: verify self.interaction == other.interaction and self.local_dim == other.local_dim before performing self.matrix @ other.matrix, and raise a ValueError with a clear message (similar to "Cannot multiply gates with different interaction" / "Cannot multiply gates with different local_dim") if they don't match; do this in BaseGate.__matmul__ (and optionally factor the checks into a small helper used by __add__, __sub__, __mul__, and __matmul__ to avoid duplication).
♻️ Duplicate comments (3)
src/mqt/yaqs/core/libraries/gate_library.py (2)
141-166:⚠️ Potential issue | 🟡 MinorValidate
local_dimbefore derivinginteraction.
local_dim <= 1reaches the logarithm-based power check and produces unclear failures; this also makesDestroy(1)/Create(1)fail indirectly instead of with the documented constructor validation.🛡️ Proposed guard
def __init__(self, mat: NDArray[np.complex128], local_dim: int = 2) -> None: """Initializes a BaseGate instance with the given matrix. @@ if mat.shape[0] != mat.shape[1]: msg = "Matrix must be square" raise ValueError(msg) + if local_dim < 2: + msg = f"local_dim must be >= 2, got {local_dim}" + raise ValueError(msg) + n = mat.shape[0] interaction = np.log(n) / np.log(local_dim)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mqt/yaqs/core/libraries/gate_library.py` around lines 141 - 166, The constructor BaseGate.__init__ computes interaction using log(local_dim) without first validating local_dim; add a guard at the start of __init__ to validate local_dim is an integer greater than 1 (e.g., local_dim >= 2) and raise a clear ValueError if not, before computing interaction, so subsequent np.log-based checks (and uses in Destroy/Create) fail with a descriptive constructor-level error instead of unclear math errors.
1546-1582:⚠️ Potential issue | 🟠 MajorReject multi-site operands in
Crosstalk.The class contract says
gate1andgate2are single-site gates, but Line 1576 accepts multi-site gates likeXX(). That makesmatrix1/matrix2invalid per-site factors and can corrupt long-range NoiseModel factor construction.🛡️ Proposed validation
if gate1.local_dim != gate2.local_dim: msg = f"Crosstalk gates must have the same local dimension, got {gate1.local_dim} and {gate2.local_dim}" raise ValueError(msg) + if gate1.interaction != 1 or gate2.interaction != 1: + msg = ( + "Crosstalk expects two single-site gates, " + f"got {gate1.name} with interaction {gate1.interaction} and " + f"{gate2.name} with interaction {gate2.interaction}" + ) + raise ValueError(msg) + self.name = f"crosstalk_{gate1.name}_{gate2.name}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mqt/yaqs/core/libraries/gate_library.py` around lines 1546 - 1582, The Crosstalk constructor currently accepts multi-site gates; add a validation that both operands are single-site by checking gate1.interaction == 1 and gate2.interaction == 1 (or equivalent single-site property) and raise a ValueError if not, before assigning matrix1/matrix2 and computing kron products; update the error message to mention the requirement for single-site gates and include gate1.name/gate2.name for clarity so matrix1/matrix2 remain valid per-site factors used elsewhere.tests/core/data_structures/test_noise_model.py (1)
171-178:⚠️ Potential issue | 🟡 MinorTighten this exception assertion to the intended name-resolution failure.
Line 177 will pass for unrelated
AttributeError/AssertionErrorraised anywhere inNoiseModel.__init__. Match the stable failure message so this test only covers the unknown-label path.🧪 Proposed test tightening
- with pytest.raises((AttributeError, AssertionError)): + with pytest.raises((AttributeError, AssertionError), match="foo_bar|unknown|resolve|factors"): _ = NoiseModel([{"name": "foo_bar", "sites": [0, 2], "strength": 0.1}])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/data_structures/test_noise_model.py` around lines 171 - 178, Tighten the test in test_longrange_unknown_label_without_factors_raises so it only catches the specific name-resolution failure from NoiseModel.__init__: replace the broad pytest.raises((AttributeError, AssertionError)) with pytest.raises(ValueError, match="unrecognized long-range label") (or the exact stable message emitted by NoiseModel.__init__ for unknown labels) so the test targets the unknown-label path in NoiseModel rather than any AttributeError/AssertionError raised elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/mqt/yaqs/core/libraries/gate_library.py`:
- Around line 192-266: The __matmul__ implementation currently does no
compatibility checks and can combine gates with mismatched interaction or
local_dim (e.g., BaseGate objects with differing interaction or local_dim), so
update __matmul__ to mirror the checks used in __mul__/__add__/__sub__: verify
self.interaction == other.interaction and self.local_dim == other.local_dim
before performing self.matrix @ other.matrix, and raise a ValueError with a
clear message (similar to "Cannot multiply gates with different interaction" /
"Cannot multiply gates with different local_dim") if they don't match; do this
in BaseGate.__matmul__ (and optionally factor the checks into a small helper
used by __add__, __sub__, __mul__, and __matmul__ to avoid duplication).
---
Duplicate comments:
In `@src/mqt/yaqs/core/libraries/gate_library.py`:
- Around line 141-166: The constructor BaseGate.__init__ computes interaction
using log(local_dim) without first validating local_dim; add a guard at the
start of __init__ to validate local_dim is an integer greater than 1 (e.g.,
local_dim >= 2) and raise a clear ValueError if not, before computing
interaction, so subsequent np.log-based checks (and uses in Destroy/Create) fail
with a descriptive constructor-level error instead of unclear math errors.
- Around line 1546-1582: The Crosstalk constructor currently accepts multi-site
gates; add a validation that both operands are single-site by checking
gate1.interaction == 1 and gate2.interaction == 1 (or equivalent single-site
property) and raise a ValueError if not, before assigning matrix1/matrix2 and
computing kron products; update the error message to mention the requirement for
single-site gates and include gate1.name/gate2.name for clarity so
matrix1/matrix2 remain valid per-site factors used elsewhere.
In `@tests/core/data_structures/test_noise_model.py`:
- Around line 171-178: Tighten the test in
test_longrange_unknown_label_without_factors_raises so it only catches the
specific name-resolution failure from NoiseModel.__init__: replace the broad
pytest.raises((AttributeError, AssertionError)) with pytest.raises(ValueError,
match="unrecognized long-range label") (or the exact stable message emitted by
NoiseModel.__init__ for unknown labels) so the test targets the unknown-label
path in NoiseModel rather than any AttributeError/AssertionError raised
elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 048ca944-e44b-46b5-a234-903da185ac3d
📒 Files selected for processing (3)
src/mqt/yaqs/core/libraries/gate_library.pytests/core/data_structures/test_noise_model.pytests/core/libraries/test_gate_library.py
Covers project overview, source layout, key abstractions (BaseGate, NoiseModel, MPS/MPO), development conventions, and useful commands. Complements AGENTS.md without duplicating it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace bare Any with Callable[..., BaseGate] and dict[str, list[float]] to satisfy ruff ANN401 (no bare Any) and ty call-non-callable checks - Move Callable import under TYPE_CHECKING per ruff TC003 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Annotations were dropped by a previous hook run. Reapply Callable[..., BaseGate] and dict[str, list[float]] for gate_factory and kwargs, with Callable imported under TYPE_CHECKING. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Member
|
@CodeRabbit full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mqt/yaqs/core/libraries/gate_library.py (1)
257-267:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
__matmul__bypasses interaction compatibility checks.
__mul__rejects mixed interaction levels, but__matmul__does not. This allows composing gates that are shape-compatible but physically incompatible (e.g., different interaction semantics), producing misleading results.Suggested fix
def __matmul__(self, other: BaseGate) -> BaseGate: """Matrix multiplication using @ operator. @@ - return BaseGate(self.matrix @ other.matrix, local_dim=self.local_dim) + if self.interaction != other.interaction: + msg = "Cannot multiply gates with different interaction" + raise ValueError(msg) + return BaseGate(self.matrix @ other.matrix, local_dim=self.local_dim)🤖 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/yaqs/core/libraries/gate_library.py` around lines 257 - 267, The __matmul__ method currently constructs a new BaseGate from self.matrix @ other.matrix without enforcing the same interaction-level compatibility checks that __mul__ performs, allowing physically incompatible gates to be combined; update __matmul__ (the BaseGate.__matmul__ method) to perform the same interaction compatibility validation as BaseGate.__mul__ (or call into the same helper used by __mul__), and raise the same error (e.g., ValueError) when interaction semantics differ, then only return BaseGate(self.matrix @ other.matrix, local_dim=self.local_dim) when the compatibility check passes.
🤖 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.
Outside diff comments:
In `@src/mqt/yaqs/core/libraries/gate_library.py`:
- Around line 257-267: The __matmul__ method currently constructs a new BaseGate
from self.matrix @ other.matrix without enforcing the same interaction-level
compatibility checks that __mul__ performs, allowing physically incompatible
gates to be combined; update __matmul__ (the BaseGate.__matmul__ method) to
perform the same interaction compatibility validation as BaseGate.__mul__ (or
call into the same helper used by __mul__), and raise the same error (e.g.,
ValueError) when interaction semantics differ, then only return
BaseGate(self.matrix @ other.matrix, local_dim=self.local_dim) when the
compatibility check passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d72f1e3-8ca5-4a40-aa4a-e71b0a2206d5
📒 Files selected for processing (7)
.gitignoresrc/mqt/yaqs/core/data_structures/noise_model.pysrc/mqt/yaqs/core/libraries/gate_library.pysrc/mqt/yaqs/core/methods/dissipation.pytests/analog/test_lindblad.pytests/core/data_structures/test_noise_model.pytests/core/libraries/test_gate_library.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When constructing a NoiseModel class, internally the constructor swapped the indexes if they were not provided in ascending order, however didn't swapped the corresponding operators.
The construction of the NoiseModel accepts directly a BaseGate class in the "name" keywords instead of only a string with the name.
Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.