added MPO.from_matrix() method#331
Merged
aaronleesander merged 32 commits intomunich-quantum-toolkit:mainfrom Feb 17, 2026
Merged
added MPO.from_matrix() method#331aaronleesander merged 32 commits intomunich-quantum-toolkit:mainfrom
aaronleesander merged 32 commits intomunich-quantum-toolkit:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
…sehubbard I need to pull before commiting
![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: 4
🤖 Fix all issues with AI agents
In `@src/mqt/yaqs/core/data_structures/networks.py`:
- Line 1821: The call to np.asarray in the networks code sets order="F"
unnecessarily; remove the order argument so the line uses np.asarray(mat,
dtype=dtype) instead. Locate the assignment to M (M = np.asarray(mat,
dtype=dtype, order="F")) in the networks module and drop the order parameter to
avoid misleading Fortran-order implication while preserving semantics and
letting NumPy choose default memory layout.
- Around line 1784-1786: Replace the legacy typing names with modern built-in
generics and add a concrete dtype type: change the parameter annotation
"max_bond: Optional[int] = None" to "max_bond: int | None = None", keep "cutoff:
float = 0.0", and add a dtype annotation like "dtype: npt.DTypeLike =
np.complex128"; also change any "List[...]" usage at the later occurrence to
"list[...]" (the code around the parameter names max_bond/cutoff/dtype and the
list usage at the later site should be updated), and add "import numpy.typing as
npt" at the top of the module.
- Around line 1865-1872: The assignment mpo.physical_dimension currently sets a
tuple (d_in, d_out) but the class annotation and other constructors expect an
int; update the constructor that creates mpo (the cls() flow) to assign a single
integer to physical_dimension instead of a tuple—use d_in when d_in == d_out (or
use max(d_in, d_out) if you want a safe fallback for asymmetric inputs) and keep
the rest of the creation flow (tensors, length, mpo.check_if_valid_mpo())
unchanged; ensure references to physical_dimension elsewhere still treat it as
an int.
In `@tests/core/data_structures/test_networks.py`:
- Around line 389-407: The existing test only covers the happy path for
MPO.from_matrix with d_in==d_out and real random matrices; add three new cases:
(1) a test that calls MPO.from_matrix with an incompatible-shaped matrix (e.g.,
shape not equal to (d_in**L, d_out**L)) and asserts it raises ValueError
(exercise the error path in from_matrix), (2) a test that constructs a known
Hamiltonian (e.g., _ising_dense) and calls MPO.from_matrix with cutoff>0 to
verify lossy truncation still reconstructs within a tolerance using
mpo.to_matrix(), and (3) a test where d_in != d_out (non-square local operators)
to ensure from_matrix handles non-square local dimensions; also replace
np.random.rand usage with a complex random matrix (use a crandn equivalent or
combine RNG.standard_normal for real and imag) and use np.random.default_rng()
for reproducible randomness when generating random matrices.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/mqt/yaqs/core/data_structures/networks.py`:
- Around line 1786-1802: The docstring for the method that "Factorize a dense
matrix into an MPO" should be converted from NumPy-style to the project's
Google-style to match other methods like hamiltonian, bose_hubbard, and
compress: replace the "Parameters" block with an "Args:" block listing mat
(np.ndarray), d (int), max_bond (int | None) — change Optional[int] to int |
None — and cutoff (float); then add a "Returns:" section that clearly documents
the returned MPO type and shape/meaning. Ensure parameter names mat, d,
max_bond, and cutoff appear exactly as in the signature and give a concise
description of the return (e.g., MPO object and inferred number of sites n).
- Around line 1809-1815: Add explicit validation for the base d and the inferred
chain length: check d first and raise clear errors for invalid values (if d <= 0
raise ValueError; if d == 1 then allow only rows == 1 and set n = 1, otherwise
raise ValueError). After computing n_float and n (using np.log(rows)/np.log(d)
as before), ensure n >= 1 and raise a ValueError if n == 0 (to prevent the later
“last site” reshape crash). Use the existing locals d, rows, n_float, and n to
implement these guards and update error messages to be explicit about the cause.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/mqt/yaqs/core/data_structures/networks.py`:
- Line 1811: Replace the Unicode multiplication sign U+00D7 in the docstrings
flagged by Ruff (RUF001/RUF002): change the occurrences of "1 × 1" (appearing in
the string If ``d == 1`` but the matrix is not ``1 × 1`` and the similar
instance at the later occurrence) to use ASCII "x" or "by" (e.g., "1 x 1" or "1
by 1") so the linter warnings go away; locate the exact text in
src/mqt/yaqs/core/data_structures/networks.py and update both occurrences
consistently.
- Around line 1895-1897: The statement "return mpo" is incorrectly dedented to
class/module level causing a SyntaxError; move the "return mpo" line to the same
indentation as the preceding "assert mpo.check_if_valid_mpo()" so it is inside
the method that initializes and returns the mpo (the function containing the
assert and mpo creation), ensuring the return belongs to that method and not the
class/module scope.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/mqt/yaqs/core/data_structures/networks.py`:
- Around line 1853-1859: The current _truncate(s: np.ndarray) uses cutoff > 0.0
so a default cutoff==0.0 keeps all singular values (including exact zeros);
either explicitly document that cutoff==0.0 means "no truncation, keep zeros" in
the function/docstring for _truncate (mention cutoff, s and max_bond) or, if the
intended behavior is to drop exact-zero singular values by default, change the
guard to treat zero as a threshold (e.g., use cutoff >= 0.0 when computing r =
max(int(np.sum(s > cutoff)), 1)) so exact-zero singular values are not counted
as meaningful bond dimensions; update tests/docs accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/mqt/yaqs/core/data_structures/networks.py`:
- Around line 1800-1802: The docstring and implementation disagree: the code
uses the guard cutoff >= 0.0 and the filter s > cutoff, which with the default
cutoff==0.0 will discard exact-zero singular values (contrary to the docstring).
Fix by either (A) updating the docstring to state that cutoff=0.0 still drops
exact-zero singular values, or (B) change the guard logic so default behavior
keeps all values (replace cutoff >= 0.0 with cutoff > 0.0 so s > cutoff is only
applied when a positive cutoff is provided); update the same pattern around the
other occurrences referring to cutoff and the s > cutoff check.
…sehubbard docstring update
Member
|
@lucello Can you change the default cutoff for the singular values to 10^-12 rather than 0? Machines basically see anything under 10^-16 as 0 and the SVD can accumulate a lot of very small values like 10^-32 for example that are just errors from numerics. A setting slightly above 10^-16 is always a good idea to avoid this. Then this pull request is good to go and I'll merge it |
aaronleesander
changed the title
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
A constructor MPO.from_matrix() is added to the MPO class in order to create custom MPOs from dense matrices.