Add MPO operator-entanglement entropy and Schmidt-spectrum APIs#364
Open
yiranwang-phys wants to merge 14 commits into
Open
Add MPO operator-entanglement entropy and Schmidt-spectrum APIs#364yiranwang-phys wants to merge 14 commits into
yiranwang-phys wants to merge 14 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughAdded operator-Schmidt and operator-entanglement diagnostics to MPO (Schmidt values, entropy, spectrum utilities, dense-channel ↔ MPO conversion, dense fused-site diagnostics, robust SVD fallbacks and numeric validation) and added a new dense-unitary OTOC module with helpers to compute pairwise and cross-cut Pauli OTOCs; comprehensive tests added. (≈50 words) ChangesMPO Schmidt & diagnostics
Dense-unitary OTOC utilities
Sequence Diagram(s)sequenceDiagram
participant User
participant MPO
participant DenseMatrix
participant SVDEngine
participant Diagnostics
User->>DenseMatrix: provide dense channel
DenseMatrix->>MPO: MPO.from_dense_channel(matrix)
User->>MPO: request dense_channel_diagnostics(cut, target?)
MPO->>SVDEngine: request SVD (NumPy)
alt NumPy fails
SVDEngine->>SVDEngine: fallback to SciPy SVD
end
SVDEngine-->>MPO: singular values (Schmidt)
MPO->>Diagnostics: compute probabilities, entropy, spectrum distance
alt target provided
Diagnostics->>DenseMatrix: compare reconstructed dense channel to target
end
Diagnostics-->>User: aggregated diagnostics (entropies, spectra, diffs)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a11597d-a604-4f71-affe-25540b788001
📒 Files selected for processing (2)
src/mqt/yaqs/core/data_structures/networks.pytests/core/data_structures/test_networks.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/networks.py`:
- Line 1853: The call to cls._validate_numeric_array with long arguments
(reconstructed, stage="dense_channel_diagnostics reconstructed", ndim=2,
dtype=np.complex128) is too long; split the call across multiple lines for
readability by putting each keyword argument on its own line (or wrap arguments
in parentheses) so the line length is under style limits—locate the invocation
of cls._validate_numeric_array where it validates reconstructed and reformat the
arguments (stage, ndim, dtype) onto separate lines while preserving the same
argument order and values.
- Around line 1797-1799: The boundary branch currently returns a hardcoded [1.0]
after resolving cut_index with cls._resolve_cut_index, which is inconsistent
with _full_schmidt_values_for_cut that returns the operator's Frobenius norm;
change the boundary handling to compute and return the same Frobenius norm used
by _full_schmidt_values_for_cut (either by calling that method for the boundary
cut or by reusing its norm computation) so the returned Schmidt values match the
dense reference at the edges.
- Line 26: Remove the unused import of scipy.sparse at the top of the file;
locate the import statement "import scipy.sparse" in
src/mqt/yaqs/core/data_structures/networks.py and delete it, ensuring any
remaining SciPy usages still reference the used symbol (e.g., scipy_linalg) so
functionality around functions that call scipy_linalg remains unchanged.
- Around line 1335-1338: The current boundary-case branch calls self.to_matrix()
(in the cut_index check) which densifies the full MPO and is exponential; change
this to avoid implicit full densification by replacing the direct to_matrix()
call with a clear guard: detect when cut_index in (0, len(self.tensors)) and if
the system is large either (a) raise a descriptive NotImplementedError or
ValueError (mentioning cut_index, to_matrix, and _array_norm) or (b) emit a
conspicuous warning and document in the method/class docstring that computing
operator_norm at boundary cuts requires full densification and is exponential,
so users must call to_matrix() on small systems only; ensure the message
references the symbols cut_index, to_matrix(), _array_norm, and tensors so
callers know why and where the cost arises.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 23864867-f91b-490a-b77b-a8fd2bb2f987
📒 Files selected for processing (1)
src/mqt/yaqs/core/data_structures/networks.py
|
|
||
| import numpy as np | ||
| import opt_einsum as oe | ||
| import scipy.sparse |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if scipy.sparse is actually used anywhere in this file
rg -n 'scipy\.sparse|scipy_sparse' src/mqt/yaqs/core/data_structures/networks.pyRepository: munich-quantum-toolkit/yaqs
Length of output: 93
🏁 Script executed:
# Search for all scipy imports and any sparse-related usage
rg -n 'scipy|sparse' src/mqt/yaqs/core/data_structures/networks.py | head -30Repository: munich-quantum-toolkit/yaqs
Length of output: 257
🏁 Script executed:
# Check if scipy.sparse might be imported differently or aliased
rg -n 'from scipy|import scipy' src/mqt/yaqs/core/data_structures/networks.pyRepository: munich-quantum-toolkit/yaqs
Length of output: 137
🏁 Script executed:
# Look at lines around the import to understand context
sed -n '20,35p' src/mqt/yaqs/core/data_structures/networks.pyRepository: munich-quantum-toolkit/yaqs
Length of output: 516
Remove unused import: scipy.sparse is not used in this file.
The scipy.sparse module is imported at line 26 but has no usages anywhere in the codebase. Only scipy_linalg from the scipy package is actively used (lines 1507, 1541). Removing this import will keep the imports clean.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mqt/yaqs/core/data_structures/networks.py` at line 26, Remove the unused
import of scipy.sparse at the top of the file; locate the import statement
"import scipy.sparse" in src/mqt/yaqs/core/data_structures/networks.py and
delete it, ensuring any remaining SciPy usages still reference the used symbol
(e.g., scipy_linalg) so functionality around functions that call scipy_linalg
remains unchanged.
Comment on lines
+1335
to
+1338
| if cut_index in (0, len(self.tensors)): | ||
| operator_norm = self._array_norm(np.asarray(self.to_matrix(), dtype=np.complex128)) | ||
| return np.array([operator_norm], dtype=np.float64) | ||
|
|
Contributor
There was a problem hiding this comment.
Performance concern: to_matrix() at boundary cuts is O(d^L) exponential.
At boundary cuts (0 or length), this calls self.to_matrix() which densifies the entire MPO. For large systems this will be prohibitively slow and may cause memory issues. Consider documenting this limitation or implementing a more efficient norm computation that avoids full densification.
💡 Suggested documentation addition
def _full_schmidt_values_for_cut(
self,
cut: str | int = "center",
decomposition: str = "QR",
) -> NDArray[np.float64]:
- """Return the complete operator Schmidt values across a resolved cut."""
+ """Return the complete operator Schmidt values across a resolved cut.
+
+ Warning:
+ For boundary cuts (0 or length), this method densifies the entire MPO
+ to compute the operator norm, which is O(d^L) in both time and memory.
+ """📝 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
| if cut_index in (0, len(self.tensors)): | |
| operator_norm = self._array_norm(np.asarray(self.to_matrix(), dtype=np.complex128)) | |
| return np.array([operator_norm], dtype=np.float64) | |
| def _full_schmidt_values_for_cut( | |
| self, | |
| cut: str | int = "center", | |
| decomposition: str = "QR", | |
| ) -> NDArray[np.float64]: | |
| """Return the complete operator Schmidt values across a resolved cut. | |
| Warning: | |
| For boundary cuts (0 or length), this method densifies the entire MPO | |
| to compute the operator norm, which is O(d^L) in both time and memory. | |
| """ | |
| cut_index = self._resolve_cut_index(cut=cut, length=len(self.tensors)) | |
| if cut_index in (0, len(self.tensors)): | |
| operator_norm = self._array_norm(np.asarray(self.to_matrix(), dtype=np.complex128)) | |
| return np.array([operator_norm], dtype=np.float64) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mqt/yaqs/core/data_structures/networks.py` around lines 1335 - 1338, The
current boundary-case branch calls self.to_matrix() (in the cut_index check)
which densifies the full MPO and is exponential; change this to avoid implicit
full densification by replacing the direct to_matrix() call with a clear guard:
detect when cut_index in (0, len(self.tensors)) and if the system is large
either (a) raise a descriptive NotImplementedError or ValueError (mentioning
cut_index, to_matrix, and _array_norm) or (b) emit a conspicuous warning and
document in the method/class docstring that computing operator_norm at boundary
cuts requires full densification and is exponential, so users must call
to_matrix() on small systems only; ensure the message references the symbols
cut_index, to_matrix(), _array_norm, and tensors so callers know why and where
the cost arises.
yiranwang-phys
force-pushed
the
feat-mpo-operator-entanglement-v1
branch
from
a57de77 to
c096b80
Compare
…e networks.py conflicts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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/networks.py`:
- Around line 2312-2314: Document that when cls._resolve_cut_index(cut=...,
length=int(n_sites)) yields a boundary cut (cut_index == 0 or cut_index ==
int(n_sites)), the method deliberately returns a trivial normalized Schmidt
spectrum np.array([1.0], dtype=np.float64); add a brief docstring note on the
method explaining this choice and clarifying that this behavior differs from the
internal representation produced by _full_schmidt_values_for_bond so callers
understand the normalization/representation difference at boundaries.
- Around line 2091-2107: The code currently does in-place normalization with
"probs /= normalization" on an array created via "probs =
np.asarray(probabilities, dtype=np.float64).reshape(-1)", which can mutate the
caller's array if np.asarray returned a view; change this to avoid in-place
modification by ensuring "probs" is a writable copy (e.g., use np.array(...) or
call probs = probs.copy() / probs = probs.astype(np.float64, copy=True) after
the np.asarray step) or replace the in-place operator with an out-of-place
assignment "probs = probs / normalization"; update the normalization step around
the variable "probs" so callers' inputs are not modified.
- Around line 2298-2300: The code instantiates mpo = cls() and calls a
non-existent mpo.init_custom(...); replace that call with the existing
mpo.custom(...) method (passing the same arguments: list of np.asarray(...,
dtype=np.complex128).copy() and transpose=False) so the MPO class's defined API
is used, or alternatively add an init_custom wrapper on the MPO class that
delegates to custom; update the call site to use mpo.custom(...) referencing the
MPO class and the custom(...) method.
In `@tests/core/data_structures/test_networks.py`:
- Around line 538-539: The test calls a non-existent method mpo.init_identity
which will raise AttributeError; update the test to call the existing
MPO.identity method instead (replace mpo.init_identity(length=4,
physical_dimension=2) with mpo.identity(length=4, physical_dimension=2)) or
alternatively add a small alias method init_identity on the MPO class that
forwards to identity; reference the MPO class and the init_identity/identity
method names when making the change.
- Around line 577-578: The test incorrectly calls the instance initializer
mpo.init_ising(...) instead of the class factory; replace the instance call with
the MPO classmethod by creating the MPO via MPO.ising(length=4, J=1.0, g=0.7)
(e.g., set mpo = MPO.ising(...)) so the test uses the intended MPO.ising
classmethod rather than init_ising.
- Around line 562-563: The test calls a non-existent instance method
mpo.init_heisenberg which triggers AttributeError; change the test to use the
existing classmethod MPO.heisenberg (e.g. call MPO.heisenberg(...) or assign mpo
= MPO.heisenberg(length=6, Jx=1.0, Jy=0.8, Jz=0.6, h=0.3)) so it constructs the
MPO correctly instead of calling init_heisenberg; update the test to reference
the MPO.heisenberg symbol accordingly.
- Around line 598-599: The test calls mpo.init_identity(length=4,
physical_dimension=2) which is incorrect; replace this invocation with the
correct method mpo.identity(length=4, physical_dimension=2) so the MPO identity
is created via the public identity() method (search for usages of init_identity
and update this test to call identity on the MPO instance).
- Around line 551-552: The test calls a non-existent instance method init_ising
on MPO; replace that usage with the provided classmethod by calling
MPO.ising(...) to construct the MPO instead of mpo.init_ising(...). Locate the
test where mpo = MPO(); mpo.init_ising(length=4, J=1.0, g=0.7) appears and
change it to create mpo via MPO.ising(length=4, J=1.0, g=0.7) (or otherwise call
the classmethod MPO.ising) so the test uses the correct MPO.ising API.
- Around line 608-609: The test incorrectly constructs the MPO by calling the
instance method mpo.init_ising; instead call the classmethod to build the
operator and assign it to mpo via MPO.ising(...). Replace the two lines that
create an MPO and call mpo.init_ising(length=4, J=1.0, g=0.5) with a single call
that assigns mpo = MPO.ising(length=4, J=1.0, g=0.5), ensuring you reference the
MPO class and its ising classmethod rather than init_ising.
- Around line 536-613: Tests call non-existent MPO methods (init_identity,
init_ising, init_heisenberg); replace those with the correct API: call
mpo.identity(length=..., physical_dimension=...) instead of
mpo.init_identity(...), and use the classmethods to create instances: mpo =
MPO.ising(length=..., J=..., g=...) instead of mpo.init_ising(...) and mpo =
MPO.heisenberg(length=..., Jx=..., Jy=..., Jz=..., h=...) instead of
mpo.init_heisenberg(...); ensure places that previously reused an instance now
assign the return value from the classmethod to mpo.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0b2321f2-55b5-42b7-b2df-e1a1f873289d
📒 Files selected for processing (2)
src/mqt/yaqs/core/data_structures/networks.pytests/core/data_structures/test_networks.py
Comment on lines
+2091
to
+2107
| probs = np.asarray(probabilities, dtype=np.float64).reshape(-1) | ||
| if probs.size == 0: | ||
| return 0.0 | ||
| if not np.all(np.isfinite(probs)): | ||
| msg = f"Non-finite probabilities encountered while computing entropy: shape={probs.shape}" | ||
| raise ValueError(msg) | ||
| if np.any(probs < 0.0): | ||
| min_probability = float(np.min(probs)) | ||
| msg = f"Negative probabilities encountered while computing entropy: min={min_probability!r}" | ||
| raise ValueError(msg) | ||
|
|
||
| normalization = float(np.sum(probs, dtype=np.float64)) | ||
| if not np.isfinite(normalization) or normalization <= 0.0: | ||
| msg = f"Invalid probability normalization while computing entropy: sum={normalization!r}" | ||
| raise RuntimeError(msg) | ||
|
|
||
| probs /= normalization |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
In-place modification of probability array may cause unexpected side effects.
At line 2107, probs /= normalization modifies the array in-place. Since probs was created from np.asarray(probabilities, ...), if the input probabilities is already a float64 array, np.asarray may return a view rather than a copy, causing the caller's array to be modified.
🛡️ Suggested fix to avoid in-place modification
- probs /= normalization
+ probs = probs / normalization📝 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
| probs = np.asarray(probabilities, dtype=np.float64).reshape(-1) | |
| if probs.size == 0: | |
| return 0.0 | |
| if not np.all(np.isfinite(probs)): | |
| msg = f"Non-finite probabilities encountered while computing entropy: shape={probs.shape}" | |
| raise ValueError(msg) | |
| if np.any(probs < 0.0): | |
| min_probability = float(np.min(probs)) | |
| msg = f"Negative probabilities encountered while computing entropy: min={min_probability!r}" | |
| raise ValueError(msg) | |
| normalization = float(np.sum(probs, dtype=np.float64)) | |
| if not np.isfinite(normalization) or normalization <= 0.0: | |
| msg = f"Invalid probability normalization while computing entropy: sum={normalization!r}" | |
| raise RuntimeError(msg) | |
| probs /= normalization | |
| probs = np.asarray(probabilities, dtype=np.float64).reshape(-1) | |
| if probs.size == 0: | |
| return 0.0 | |
| if not np.all(np.isfinite(probs)): | |
| msg = f"Non-finite probabilities encountered while computing entropy: shape={probs.shape}" | |
| raise ValueError(msg) | |
| if np.any(probs < 0.0): | |
| min_probability = float(np.min(probs)) | |
| msg = f"Negative probabilities encountered while computing entropy: min={min_probability!r}" | |
| raise ValueError(msg) | |
| normalization = float(np.sum(probs, dtype=np.float64)) | |
| if not np.isfinite(normalization) or normalization <= 0.0: | |
| msg = f"Invalid probability normalization while computing entropy: sum={normalization!r}" | |
| raise RuntimeError(msg) | |
| probs = probs / normalization |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mqt/yaqs/core/data_structures/networks.py` around lines 2091 - 2107, The
code currently does in-place normalization with "probs /= normalization" on an
array created via "probs = np.asarray(probabilities,
dtype=np.float64).reshape(-1)", which can mutate the caller's array if
np.asarray returned a view; change this to avoid in-place modification by
ensuring "probs" is a writable copy (e.g., use np.array(...) or call probs =
probs.copy() / probs = probs.astype(np.float64, copy=True) after the np.asarray
step) or replace the in-place operator with an out-of-place assignment "probs =
probs / normalization"; update the normalization step around the variable
"probs" so callers' inputs are not modified.
Comment on lines
+2312
to
+2314
| cut_index = cls._resolve_cut_index(cut=cut, length=int(n_sites)) | ||
| if cut_index in {0, int(n_sites)}: | ||
| return np.array([1.0], dtype=np.float64) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting the boundary cut behavior.
At boundary cuts (0 or n_sites), this method returns [1.0] which represents a trivial cut with no entanglement. This is a reasonable choice for a normalized Schmidt spectrum reference, but it differs from what _full_schmidt_values_for_bond computes internally. Consider adding a brief docstring note explaining this design choice to prevent confusion when comparing results.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mqt/yaqs/core/data_structures/networks.py` around lines 2312 - 2314,
Document that when cls._resolve_cut_index(cut=..., length=int(n_sites)) yields a
boundary cut (cut_index == 0 or cut_index == int(n_sites)), the method
deliberately returns a trivial normalized Schmidt spectrum np.array([1.0],
dtype=np.float64); add a brief docstring note on the method explaining this
choice and clarifying that this behavior differs from the internal
representation produced by _full_schmidt_values_for_bond so callers understand
the normalization/representation difference at boundaries.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/mqt/yaqs/core/data_structures/networks.py (3)
2466-2478:⚠️ Potential issue | 🟠 MajorBoundary cuts in
dense_center_cut_schmidt_values()return the wrong spectrum.The early
[1.0]return makes the Frobenius-norm branch below unreachable and skips dense-channel validation entirely. Forcut=0orcut=n_sites, this helper should matchMPO.schmidt_values()by returning[||channel_dense||_F], not a hard-coded normalized value.🐛 Suggested fix
cut_index = cls._resolve_cut_index(cut=cut, length=int(n_sites)) - if cut_index in {0, int(n_sites)}: - return np.array([1.0], dtype=np.float64) - channel = cls._validated_dense_channel_matrix( channel_dense, n_sites=n_sites, local_dim=local_dim, stage="dense_center_cut_schmidt_values input", ) if cut_index in {0, int(n_sites)}: fro_norm = float(np.linalg.norm(channel, ord="fro")) return np.array([fro_norm], dtype=np.float64)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mqt/yaqs/core/data_structures/networks.py` around lines 2466 - 2478, The function dense_center_cut_schmidt_values currently returns a hard-coded [1.0] for boundary cuts before validating the dense channel and computing the Frobenius norm; remove the early return (the branch that returns np.array([1.0])) and instead resolve the cut with _resolve_cut_index, call _validated_dense_channel_matrix(channel_dense, n_sites, local_dim, stage=...), then for cut_index in {0, int(n_sites)} compute fro_norm = float(np.linalg.norm(channel, ord="fro")) and return np.array([fro_norm], dtype=np.float64) so boundary behavior matches MPO.schmidt_values().
2179-2195:⚠️ Potential issue | 🟡 MinorDon't normalize the caller's probability buffer in place.
np.asarray()can alias a writable input array, soprobs /= normalizationmutates the caller'sprobabilities. It also raises on read-only views.🧼 Suggested fix
- probs = np.asarray(probabilities, dtype=np.float64).reshape(-1) + probs = np.array(probabilities, dtype=np.float64, copy=True).reshape(-1) if probs.size == 0: return 0.0 @@ - probs /= normalization + probs = probs / normalization🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mqt/yaqs/core/data_structures/networks.py` around lines 2179 - 2195, The code currently does probs = np.asarray(probabilities, dtype=np.float64).reshape(-1) and then mutates that buffer in-place with probs /= normalization, which can modify the caller's array or fail on read-only views; change the allocation so you operate on a private writable copy (e.g. create probs as a copy via np.asarray(...).reshape(-1).copy() or np.array(..., dtype=np.float64, copy=True).reshape(-1)) and then perform normalization without in-place mutation (e.g. probs = probs / normalization or keep the copied array and use /= safely), updating the block around probs and probs /= normalization accordingly to avoid mutating the caller's probabilities.
1893-1896:⚠️ Potential issue | 🟠 MajorAvoid
to_matrix()for trivial cuts.This branch densifies the full MPO just to get its Frobenius norm. That makes
schmidt_values(cut=0|length)andoperator_entanglement_entropy()exponential in memory/time on large systems, even though the same value is available from the fused-site MPS norm.⚡ Suggested fix
cut_index = self._resolve_cut_index(cut=cut, length=len(self.tensors)) if cut_index in {0, len(self.tensors)}: - fro_norm = float(np.linalg.norm(np.asarray(self.to_matrix(), dtype=np.complex128), ord="fro")) + fro_norm_sq = max(0.0, float(self.to_mps().norm())) + fro_norm = math.sqrt(fro_norm_sq) return np.array([fro_norm], dtype=np.float64)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mqt/yaqs/core/data_structures/networks.py` around lines 1893 - 1896, The branch handling trivial cuts currently calls to_matrix(), densifying the full MPO; instead compute the Frobenius norm without forming the full matrix by contracting the MPO tensors into the fused-site vector and taking its 2-norm. Modify the code in schmidt_values (and thus operator_entanglement_entropy) where _resolve_cut_index(...) returns 0 or len(self.tensors) to avoid to_matrix(), and compute fro_norm by contracting self.tensors into a single fused-site representation (or using an existing fused-site/MPS-norm helper) and then calling np.linalg.norm on that fused vector; keep the return as np.array([fro_norm], dtype=np.float64). Ensure you reference and update logic around _resolve_cut_index, to_matrix, and the tensors field so the expensive densification is removed.
🤖 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/networks.py`:
- Around line 1853-1865: In _full_schmidt_values_for_bond, validate the
decomposition parameter (allowed values "QR" and "SVD") before calling
mps.set_canonical_form: if decomposition is not one of those, raise a ValueError
with a clear message; this ensures _full_schmidt_values_for_bond (which calls
MPS.set_canonical_form with orthogonality_center=j and
decomposition=decomposition) fails fast instead of silently using an unsupported
decomposition.
In `@tests/core/data_structures/test_networks.py`:
- Around line 468-474: The test test_from_dense_channel_roundtrip uses an
identity matrix which won't catch site-ordering or transpose bugs; replace or
augment the input to MPO.from_dense_channel (and dense_channel_to_tensors) with
a non-diagonal, deterministic matrix (for example a fixed-seed random unitary or
a permutation/Off-diagonal matrix generated via np.random.default_rng(0) or a
known permutation/unitary) so the round-trip mpo.to_matrix() comparison fails on
interleaving bugs; update the test to construct such a non-diagonal 16x16
channel, pass it to MPO.from_dense_channel, and keep the
np.testing.assert_allclose(mpo.to_matrix(), channel, atol=1e-12) assertion.
---
Duplicate comments:
In `@src/mqt/yaqs/core/data_structures/networks.py`:
- Around line 2466-2478: The function dense_center_cut_schmidt_values currently
returns a hard-coded [1.0] for boundary cuts before validating the dense channel
and computing the Frobenius norm; remove the early return (the branch that
returns np.array([1.0])) and instead resolve the cut with _resolve_cut_index,
call _validated_dense_channel_matrix(channel_dense, n_sites, local_dim,
stage=...), then for cut_index in {0, int(n_sites)} compute fro_norm =
float(np.linalg.norm(channel, ord="fro")) and return np.array([fro_norm],
dtype=np.float64) so boundary behavior matches MPO.schmidt_values().
- Around line 2179-2195: The code currently does probs =
np.asarray(probabilities, dtype=np.float64).reshape(-1) and then mutates that
buffer in-place with probs /= normalization, which can modify the caller's array
or fail on read-only views; change the allocation so you operate on a private
writable copy (e.g. create probs as a copy via
np.asarray(...).reshape(-1).copy() or np.array(..., dtype=np.float64,
copy=True).reshape(-1)) and then perform normalization without in-place mutation
(e.g. probs = probs / normalization or keep the copied array and use /= safely),
updating the block around probs and probs /= normalization accordingly to avoid
mutating the caller's probabilities.
- Around line 1893-1896: The branch handling trivial cuts currently calls
to_matrix(), densifying the full MPO; instead compute the Frobenius norm without
forming the full matrix by contracting the MPO tensors into the fused-site
vector and taking its 2-norm. Modify the code in schmidt_values (and thus
operator_entanglement_entropy) where _resolve_cut_index(...) returns 0 or
len(self.tensors) to avoid to_matrix(), and compute fro_norm by contracting
self.tensors into a single fused-site representation (or using an existing
fused-site/MPS-norm helper) and then calling np.linalg.norm on that fused
vector; keep the return as np.array([fro_norm], dtype=np.float64). Ensure you
reference and update logic around _resolve_cut_index, to_matrix, and the tensors
field so the expensive densification is removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a85e0a98-0f38-4ff7-9cac-4ddef50ed2e4
📒 Files selected for processing (2)
src/mqt/yaqs/core/data_structures/networks.pytests/core/data_structures/test_networks.py
Comment on lines
+1853
to
+1865
| def _full_schmidt_values_for_bond( | ||
| self, | ||
| sites: list[int], | ||
| decomposition: str = "QR", | ||
| ) -> NDArray[np.float64]: | ||
| """Return the complete operator Schmidt values across a nearest-neighbor bond.""" | ||
| assert len(sites) == 2, "Schmidt spectrum is defined on a bond (two adjacent sites)." | ||
| i, j = sites | ||
| assert i + 1 == j, "Schmidt spectrum is only defined for nearest-neighbor cut." | ||
|
|
||
| mps = self.to_mps() | ||
| mps.set_canonical_form(orthogonality_center=j, decomposition=decomposition) | ||
|
|
Contributor
There was a problem hiding this comment.
Reject unsupported decomposition values up front.
Anything other than "QR" or "SVD" currently flows into MPS.set_canonical_form() and mostly no-ops there, so this method can return Schmidt values from a non-canonical gauge instead of failing fast.
🛡️ Suggested fix
def _full_schmidt_values_for_bond(
self,
sites: list[int],
decomposition: str = "QR",
) -> NDArray[np.float64]:
"""Return the complete operator Schmidt values across a nearest-neighbor bond."""
+ if decomposition not in {"QR", "SVD"}:
+ msg = "decomposition must be 'QR' or 'SVD'."
+ raise ValueError(msg)
assert len(sites) == 2, "Schmidt spectrum is defined on a bond (two adjacent sites)."
i, j = sites
assert i + 1 == j, "Schmidt spectrum is only defined for nearest-neighbor cut."
mps = self.to_mps()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mqt/yaqs/core/data_structures/networks.py` around lines 1853 - 1865, In
_full_schmidt_values_for_bond, validate the decomposition parameter (allowed
values "QR" and "SVD") before calling mps.set_canonical_form: if decomposition
is not one of those, raise a ValueError with a clear message; this ensures
_full_schmidt_values_for_bond (which calls MPS.set_canonical_form with
orthogonality_center=j and decomposition=decomposition) fails fast instead of
silently using an unsupported decomposition.
Comment on lines
+468
to
+474
| def test_from_dense_channel_roundtrip() -> None: | ||
| """Test dense channel factory reconstructs the original dense channel.""" | ||
| channel = np.eye(16, dtype=np.complex128) | ||
|
|
||
| mpo = MPO.from_dense_channel(channel, n_sites=2, local_dim=4) | ||
|
|
||
| np.testing.assert_allclose(mpo.to_matrix(), channel, atol=1e-12) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Strengthen this round-trip with a non-diagonal channel.
np.eye(16) will survive any site-ordering or transpose bug, so this test does not really validate the new interleaving logic in from_dense_channel() / dense_channel_to_tensors(). Add at least one dense channel with off-diagonal structure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/data_structures/test_networks.py` around lines 468 - 474, The test
test_from_dense_channel_roundtrip uses an identity matrix which won't catch
site-ordering or transpose bugs; replace or augment the input to
MPO.from_dense_channel (and dense_channel_to_tensors) with a non-diagonal,
deterministic matrix (for example a fixed-seed random unitary or a
permutation/Off-diagonal matrix generated via np.random.default_rng(0) or a
known permutation/unitary) so the round-trip mpo.to_matrix() comparison fails on
interleaving bugs; update the test to construct such a non-diagonal 16x16
channel, pass it to MPO.from_dense_channel, and keep the
np.testing.assert_allclose(mpo.to_matrix(), channel, atol=1e-12) assertion.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/core/data_structures/test_networks.py (1)
468-474: 🧹 Nitpick | 🔵 TrivialAdd at least one off-diagonal dense channel fixture here.
These cases only exercise
np.eye(...)/np.diag(...), which survive many site-ordering and transpose bugs. The dense-channel roundtrip and diagnostics paths need a deterministic off-diagonal input, otherwise the new interleaving logic can be wrong and still pass.Also applies to: 479-483, 509-523
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/data_structures/test_networks.py` around lines 468 - 474, The test only uses diagonal identity channels and must include at least one deterministic off-diagonal dense-channel fixture to catch site-ordering/interleaving bugs; update test_from_dense_channel_roundtrip (and the similar tests around lines noted) to create an off-diagonal channel (e.g., a fixed permutation or a small complex matrix with a hard-coded pattern or a RNG with a fixed seed), call MPO.from_dense_channel(channel, n_sites=..., local_dim=...), and assert mpo.to_matrix() matches the original channel with np.testing.assert_allclose; reference MPO.from_dense_channel and mpo.to_matrix when adding the new fixture so the round-trip covers non-diagonal structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/core/data_structures/test_networks.py`:
- Around line 251-268: The dense_operator_schmidt_values oracle currently uses
mpo.to_mps() (in dense_operator_schmidt_values) which masks fused-leg ordering
bugs; instead call mpo.to_matrix() to get the full dense operator, then
reshape/transpose that dense matrix explicitly into a 2‑party bipartition at the
requested cut: compute the product of physical dimensions left_of_cut and
right_of_cut to form (left_dim, right_dim), reshape/transpose the full matrix so
that the degrees of freedom left of the cut are contiguous in the first axis and
those right of the cut are contiguous in the second axis, then run np.linalg.svd
on that reshaped matrix and return the singular values as np.float64. Ensure you
reference dense_operator_schmidt_values and replace usage of mpo.to_mps() with
mpo.to_matrix() plus explicit reshape/transpose based on site dims to make the
oracle independent of MPO.to_mps() ordering.
- Around line 769-773: The function dense_center_cut_schmidt_values contains an
early-return that unconditionally returns [1.0] for trivial cuts, violating the
public contract in _full_schmidt_values_for_cut which requires returning the
operator Frobenius norm; remove that early return in
dense_center_cut_schmidt_values so execution reaches the existing normalization
logic (the block that computes and scales by the Frobenius norm) and ensure the
test is updated to assert the actual Frobenius norm of the test channel instead
of the hardcoded [1.0].
---
Duplicate comments:
In `@tests/core/data_structures/test_networks.py`:
- Around line 468-474: The test only uses diagonal identity channels and must
include at least one deterministic off-diagonal dense-channel fixture to catch
site-ordering/interleaving bugs; update test_from_dense_channel_roundtrip (and
the similar tests around lines noted) to create an off-diagonal channel (e.g., a
fixed permutation or a small complex matrix with a hard-coded pattern or a RNG
with a fixed seed), call MPO.from_dense_channel(channel, n_sites=...,
local_dim=...), and assert mpo.to_matrix() matches the original channel with
np.testing.assert_allclose; reference MPO.from_dense_channel and mpo.to_matrix
when adding the new fixture so the round-trip covers non-diagonal structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: de88950a-43c2-4861-ad40-8538e1e92615
📒 Files selected for processing (1)
tests/core/data_structures/test_networks.py
Comment on lines
+251
to
+268
| def dense_operator_schmidt_values(mpo: MPO, cut: int) -> NDArray[np.float64]: | ||
| """Compute Schmidt values from dense contraction for a given MPO cut. | ||
|
|
||
| Returns: | ||
| Dense Schmidt singular values for the requested cut. | ||
| """ | ||
| mps = mpo.to_mps() | ||
|
|
||
| state = mps.tensors[0][:, 0, :] | ||
| for tensor in mps.tensors[1:]: | ||
| state = np.tensordot(state, tensor, axes=([-1], [1])) | ||
|
|
||
| state = np.squeeze(state, axis=-1) | ||
| left_dim = int(np.prod(state.shape[:cut], dtype=np.int64)) | ||
| right_dim = int(np.prod(state.shape[cut:], dtype=np.int64)) | ||
| matrix = np.reshape(state, (left_dim, right_dim)) | ||
| singular_values = np.linalg.svd(matrix, compute_uv=False, full_matrices=False) | ||
| return np.asarray(singular_values, dtype=np.float64) |
Contributor
There was a problem hiding this comment.
Make the dense Schmidt oracle independent of MPO.to_mps().
Line 257 builds the “dense reference” from mpo.to_mps(), so test_mpo_schmidt_values_and_entropy_match_dense_reference() can still pass if the MPO→MPS fused-leg ordering is wrong on both sides. Compute the oracle from mpo.to_matrix() with an explicit dense reshape/transpose instead.
🛠️ Proposed change
def dense_operator_schmidt_values(mpo: MPO, cut: int) -> NDArray[np.float64]:
"""Compute Schmidt values from dense contraction for a given MPO cut.
Returns:
Dense Schmidt singular values for the requested cut.
"""
- mps = mpo.to_mps()
-
- state = mps.tensors[0][:, 0, :]
- for tensor in mps.tensors[1:]:
- state = np.tensordot(state, tensor, axes=([-1], [1]))
-
- state = np.squeeze(state, axis=-1)
- left_dim = int(np.prod(state.shape[:cut], dtype=np.int64))
- right_dim = int(np.prod(state.shape[cut:], dtype=np.int64))
- matrix = np.reshape(state, (left_dim, right_dim))
+ dense = np.asarray(mpo.to_matrix(), dtype=np.complex128)
+ d = mpo.physical_dimension
+ length = mpo.length
+
+ tensor = dense.reshape((d,) * (2 * length))
+ fused_axes = [axis for site in range(length) for axis in (site, site + length)]
+ fused = np.transpose(tensor, fused_axes)
+ matrix = fused.reshape(d ** (2 * cut), d ** (2 * (length - cut)))
singular_values = np.linalg.svd(matrix, compute_uv=False, full_matrices=False)
return np.asarray(singular_values, dtype=np.float64)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/data_structures/test_networks.py` around lines 251 - 268, The
dense_operator_schmidt_values oracle currently uses mpo.to_mps() (in
dense_operator_schmidt_values) which masks fused-leg ordering bugs; instead call
mpo.to_matrix() to get the full dense operator, then reshape/transpose that
dense matrix explicitly into a 2‑party bipartition at the requested cut: compute
the product of physical dimensions left_of_cut and right_of_cut to form
(left_dim, right_dim), reshape/transpose the full matrix so that the degrees of
freedom left of the cut are contiguous in the first axis and those right of the
cut are contiguous in the second axis, then run np.linalg.svd on that reshaped
matrix and return the singular values as np.float64. Ensure you reference
dense_operator_schmidt_values and replace usage of mpo.to_mps() with
mpo.to_matrix() plus explicit reshape/transpose based on site dims to make the
oracle independent of MPO.to_mps() ordering.
Comment on lines
+769
to
+773
| np.testing.assert_allclose( | ||
| MPO.dense_center_cut_schmidt_values(channel, n_sites=2, cut=0, local_dim=4), | ||
| np.array([1.0]), | ||
| atol=1e-12, | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Public trivial-cut contract ==="
sed -n '1877,1902p' src/mqt/yaqs/core/data_structures/networks.py
echo
echo "=== Dense helper implementation ==="
rg -n -A40 -B5 'def dense_center_cut_schmidt_values' src/mqt/yaqs/core/data_structures/networks.pyRepository: munich-quantum-toolkit/yaqs
Length of output: 3426
Fix the trivial-cut normalization in dense_center_cut_schmidt_values().
The early return at line 2468 hardcodes [1.0] for trivial cuts, but the public contract (_full_schmidt_values_for_cut) specifies that trivial cuts must return the operator Frobenius norm. The subsequent check at lines 2476-2478 (unreachable due to the early return) contains the correct logic. Remove the early return and rely on the subsequent normalization to match the public contract. The test assertion should then verify the actual Frobenius norm of the test channel, not a hardcoded [1.0].
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/data_structures/test_networks.py` around lines 769 - 773, The
function dense_center_cut_schmidt_values contains an early-return that
unconditionally returns [1.0] for trivial cuts, violating the public contract in
_full_schmidt_values_for_cut which requires returning the operator Frobenius
norm; remove that early return in dense_center_cut_schmidt_values so execution
reaches the existing normalization logic (the block that computes and scales by
the Frobenius norm) and ensure the test is updated to assert the actual
Frobenius norm of the test channel instead of the hardcoded [1.0].
Assisted-by: GPT-5 via Codex
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/yaqs/core/methods/otoc.py`:
- Around line 280-355: Add Google-style docstrings to each private helper in
this diff: _average_directed_cross_cut, _validate_num_qubits, _validate_site,
_validate_sites, _validate_paulis, _validate_normalization,
_validate_unitary_shape, and _real_float. For each function include a short
one-line summary, an Args section describing parameter names and types, a
Returns section with the return type (or None), and a Raises section listing the
ValueError conditions (e.g., invalid num_qubits, out-of-range site, empty
sites/paulis, wrong unitary shape, non-real OTOC imaginary residual). Ensure
docstrings follow Google style and match the existing parameter names and
behavior of the functions.
- Around line 216-218: The current validation calls to _validate_sites for
left_sites and right_sites allow overlaps and duplicates which breaks across-cut
partition semantics; update _validate_sites usage (and any other call sites
around lines 308-315) to enforce that each site list contains only unique qubit
indices and that left_sites and right_sites are disjoint: check
len(set(left_sites)) == len(left_sites) and len(set(right_sites)) ==
len(right_sites), validate indices are in range 0..num_qubits-1 (reuse existing
_validate_sites checks), and assert set(left_sites).isdisjoint(set(right_sites))
(raise a clear ValueError mentioning "left_sites" and "right_sites"); keep pauli
validation via _validate_paulis as-is. Ensure the same enforcement is applied to
all cross-cut API calls in this module.
- Around line 302-305: The _validate_site function currently allows non-integer
numerics (e.g., 0.5) to pass the bounds check and later cause a TypeError;
update _validate_site to first verify the type by checking isinstance(site,
numbers.Integral) (or numbers.Integral/Integral import) and raise a TypeError
with a clear message if not integral, then proceed with the existing 0 <= site <
num_qubits bounds check; place this type check at the top of _validate_site so
_validate_site is the point that rejects non-integer indices.
In `@tests/core/methods/test_otoc.py`:
- Line 120: Remove the "# type: ignore[arg-type]" suppression and instead pass
an explicitly typed invalid value using typing.Any so the test remains
intentionally invalid but typesafe; import Any from typing at the top of the
test, create a variable like invalid_norm: Any = "scaled" (or use
typing.cast(Any, "scaled")) and call pair_pauli_otoc(unitary, 0, 1, "X", "Z",
num_qubits=2, normalization=invalid_norm) to preserve the test intent without a
type-ignore comment.
🪄 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: 90af78ed-79b8-4ba2-bbff-09fccc1bc7c4
📒 Files selected for processing (2)
src/mqt/yaqs/core/methods/otoc.pytests/core/methods/test_otoc.py
Comment on lines
+216
to
+218
| left = _validate_sites(left_sites, num_qubits, "left_sites") | ||
| right = _validate_sites(right_sites, num_qubits, "right_sites") | ||
| pauli_labels = _validate_paulis(paulis) |
Contributor
There was a problem hiding this comment.
Enforce disjoint, unique site partitions for cross-cut APIs.
Lines 216-218 currently allow overlaps and duplicates in left_sites/right_sites, which biases averages and breaks partition semantics for “across-cut” metrics.
Suggested fix
def cross_cut_otoc(
...
) -> dict[str, float]:
...
left = _validate_sites(left_sites, num_qubits, "left_sites")
right = _validate_sites(right_sites, num_qubits, "right_sites")
+ overlap = set(left) & set(right)
+ if overlap:
+ msg = f"left_sites and right_sites must be disjoint, but overlap at {sorted(overlap)}."
+ raise ValueError(msg) def _validate_sites(sites: Sequence[int], num_qubits: int, name: str) -> list[int]:
site_list = list(sites)
if not site_list:
msg = f"{name} must be non-empty."
raise ValueError(msg)
+ if len(set(site_list)) != len(site_list):
+ msg = f"{name} must not contain duplicates."
+ raise ValueError(msg)
for site in site_list:
_validate_site(site, num_qubits)
return site_listAlso applies to: 308-315
🤖 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/methods/otoc.py` around lines 216 - 218, The current
validation calls to _validate_sites for left_sites and right_sites allow
overlaps and duplicates which breaks across-cut partition semantics; update
_validate_sites usage (and any other call sites around lines 308-315) to enforce
that each site list contains only unique qubit indices and that left_sites and
right_sites are disjoint: check len(set(left_sites)) == len(left_sites) and
len(set(right_sites)) == len(right_sites), validate indices are in range
0..num_qubits-1 (reuse existing _validate_sites checks), and assert
set(left_sites).isdisjoint(set(right_sites)) (raise a clear ValueError
mentioning "left_sites" and "right_sites"); keep pauli validation via
_validate_paulis as-is. Ensure the same enforcement is applied to all cross-cut
API calls in this module.
Comment on lines
+280
to
+355
| def _average_directed_cross_cut( | ||
| unitary: np.ndarray, | ||
| source_sites: Sequence[int], | ||
| target_sites: Sequence[int], | ||
| num_qubits: int, | ||
| paulis: Sequence[str], | ||
| normalization: Normalization, | ||
| ) -> float: | ||
| values = [ | ||
| averaged_pair_otoc(unitary, source_site, target_site, num_qubits, paulis, normalization) | ||
| for source_site in source_sites | ||
| for target_site in target_sites | ||
| ] | ||
| return float(np.mean(values)) | ||
|
|
||
|
|
||
| def _validate_num_qubits(num_qubits: int) -> None: | ||
| if num_qubits <= 0: | ||
| msg = f"num_qubits must be positive, but is {num_qubits}." | ||
| raise ValueError(msg) | ||
|
|
||
|
|
||
| def _validate_site(site: int, num_qubits: int) -> None: | ||
| if site < 0 or site >= num_qubits: | ||
| msg = f"site must satisfy 0 <= site < num_qubits, but got site={site} and num_qubits={num_qubits}." | ||
| raise ValueError(msg) | ||
|
|
||
|
|
||
| def _validate_sites(sites: Sequence[int], num_qubits: int, name: str) -> list[int]: | ||
| site_list = list(sites) | ||
| if not site_list: | ||
| msg = f"{name} must be non-empty." | ||
| raise ValueError(msg) | ||
| for site in site_list: | ||
| _validate_site(site, num_qubits) | ||
| return site_list | ||
|
|
||
|
|
||
| def _validate_paulis(paulis: Sequence[str]) -> tuple[str, ...]: | ||
| pauli_labels = tuple(paulis) | ||
| if not pauli_labels: | ||
| msg = "paulis must be non-empty." | ||
| raise ValueError(msg) | ||
| for label in pauli_labels: | ||
| pauli_matrix(label) | ||
| return pauli_labels | ||
|
|
||
|
|
||
| def _validate_normalization(normalization: str) -> None: | ||
| if normalization not in {"raw", "bounded"}: | ||
| msg = f"normalization must be 'raw' or 'bounded', but is {normalization!r}." | ||
| raise ValueError(msg) | ||
|
|
||
|
|
||
| def _validate_unitary_shape(unitary: np.ndarray, num_qubits: int) -> NDArray[np.complex128]: | ||
| _validate_num_qubits(num_qubits) | ||
| unitary_array = np.asarray(unitary, dtype=np.complex128) | ||
| dimension = 2**num_qubits | ||
| expected_shape = (dimension, dimension) | ||
| if unitary_array.shape != expected_shape: | ||
| msg = ( | ||
| f"unitary must have shape {expected_shape} for num_qubits={num_qubits}, " | ||
| f"but has shape {unitary_array.shape}." | ||
| ) | ||
| raise ValueError(msg) | ||
| return unitary_array | ||
|
|
||
|
|
||
| def _real_float(value: complex) -> float: | ||
| real_value = float(np.real(value)) | ||
| imaginary_value = float(np.imag(value)) | ||
| tolerance = 1e-10 * max(1.0, abs(real_value)) | ||
| if abs(imaginary_value) > tolerance: | ||
| msg = f"Expected a real OTOC value, but the imaginary residual is {imaginary_value}." | ||
| raise ValueError(msg) | ||
| return real_value |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add Google-style docstrings to private helper functions.
Functions in Lines 280-355 (_average_directed_cross_cut, _validate_*, _real_float) are missing docstrings, which violates repository Python docstring standards.
As per coding guidelines, **/*.py: Use Google-style docstrings in Python code.
🤖 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/methods/otoc.py` around lines 280 - 355, Add Google-style
docstrings to each private helper in this diff: _average_directed_cross_cut,
_validate_num_qubits, _validate_site, _validate_sites, _validate_paulis,
_validate_normalization, _validate_unitary_shape, and _real_float. For each
function include a short one-line summary, an Args section describing parameter
names and types, a Returns section with the return type (or None), and a Raises
section listing the ValueError conditions (e.g., invalid num_qubits,
out-of-range site, empty sites/paulis, wrong unitary shape, non-real OTOC
imaginary residual). Ensure docstrings follow Google style and match the
existing parameter names and behavior of the functions.
Comment on lines
+302
to
+305
| def _validate_site(site: int, num_qubits: int) -> None: | ||
| if site < 0 or site >= num_qubits: | ||
| msg = f"site must satisfy 0 <= site < num_qubits, but got site={site} and num_qubits={num_qubits}." | ||
| raise ValueError(msg) |
Contributor
There was a problem hiding this comment.
Validate site type before bounds checks.
Line 303 accepts non-integer numerics (e.g., 0.5), which bypasses validation and fails later with a non-API TypeError during indexing. Reject non-integer site indices here.
Suggested fix
+from numbers import Integral
...
def _validate_site(site: int, num_qubits: int) -> None:
+ if not isinstance(site, Integral) or isinstance(site, bool):
+ msg = f"site must be an integer index, but got {site!r}."
+ raise ValueError(msg)
if site < 0 or site >= num_qubits:
msg = f"site must satisfy 0 <= site < num_qubits, but got site={site} and num_qubits={num_qubits}."
raise ValueError(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/yaqs/core/methods/otoc.py` around lines 302 - 305, The _validate_site
function currently allows non-integer numerics (e.g., 0.5) to pass the bounds
check and later cause a TypeError; update _validate_site to first verify the
type by checking isinstance(site, numbers.Integral) (or
numbers.Integral/Integral import) and raise a TypeError with a clear message if
not integral, then proceed with the existing 0 <= site < num_qubits bounds
check; place this type check at the top of _validate_site so _validate_site is
the point that rejects non-integer indices.
| pair_pauli_otoc(unitary, 0, 1, "A", "Z", num_qubits=2) | ||
|
|
||
| with pytest.raises(ValueError, match="normalization must be"): | ||
| pair_pauli_otoc(unitary, 0, 1, "X", "Z", num_qubits=2, normalization="scaled") # type: ignore[arg-type] |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Avoid suppression for intentional invalid-typing test input.
Line 120 uses # type: ignore[arg-type]; prefer passing an explicitly-typed invalid value (e.g., Any) so the test intent is preserved without suppression.
Suggested fix
+from typing import Any
...
+ invalid_normalization: Any = "scaled"
with pytest.raises(ValueError, match="normalization must be"):
- pair_pauli_otoc(unitary, 0, 1, "X", "Z", num_qubits=2, normalization="scaled") # type: ignore[arg-type]
+ pair_pauli_otoc(unitary, 0, 1, "X", "Z", num_qubits=2, normalization=invalid_normalization)As per coding guidelines, PREFER fixing typing issues reported by ty before adding suppression comments (# ty: ignore[code]); suppressions are sometimes necessary for incompletely typed libraries (e.g., Qiskit).
📝 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
| pair_pauli_otoc(unitary, 0, 1, "X", "Z", num_qubits=2, normalization="scaled") # type: ignore[arg-type] | |
| invalid_normalization: Any = "scaled" | |
| with pytest.raises(ValueError, match="normalization must be"): | |
| pair_pauli_otoc(unitary, 0, 1, "X", "Z", num_qubits=2, normalization=invalid_normalization) |
🤖 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 `@tests/core/methods/test_otoc.py` at line 120, Remove the "# type:
ignore[arg-type]" suppression and instead pass an explicitly typed invalid value
using typing.Any so the test remains intentionally invalid but typesafe; import
Any from typing at the top of the test, create a variable like invalid_norm: Any
= "scaled" (or use typing.cast(Any, "scaled")) and call pair_pauli_otoc(unitary,
0, 1, "X", "Z", num_qubits=2, normalization=invalid_norm) to preserve the test
intent without a type-ignore comment.
Assisted-by: GPT-5 via Codex
This reverts commit 46624cb.
yiranwang-phys
changed the title
Author
|
…ntanglement-v1 # Conflicts: # src/mqt/yaqs/core/data_structures/mps.py # tests/core/data_structures/test_mps.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
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.
Summary
This PR adds MPO-side operator-entanglement diagnostics for YAQS.
The main tools are:
The entropy convention is:
[
p_i = \frac{s_i^2}{\sum_j s_j^2}, \qquad
S = -\sum_i p_i \log(p_i),
]
where (s_i) are the operator-Schmidt singular values of the MPO across the selected cut.
Validation
The implementation is covered by focused tests in:
tests/core/data_structures/test_networks.pyThe tests cover dense-reference comparisons, cut handling, trivial cuts, invalid inputs, helper-level behavior, and numerical edge cases.