added linalg submodule to open a new path for optimizations and stop BLAS thread oversubscription for stability#429
Merged
Merged
Conversation
Member
Author
|
@CodeRabbit full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (29)
📝 WalkthroughWalkthroughAdds mqt.yaqs.core.linalg with thread-limited expm/eigh/svd and truncation helpers; refactors two-site merge/split decomposition and rewires matrix-exponential and SVD usage across MPS, TDVP, dissipation, stochastic methods, libraries, MPO/MPS utilities, and tests. ChangesCore linalg submodule and tensor decomposition refactoring
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mqt/yaqs/core/methods/stochastic_process.py (1)
123-140:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winNorm computed before tensors are updated in two-site jump probability.
The
dp_mcalculation at line 126 computesjumped_state.norm(site)before the merged+jumped tensor is split and written back tojumped_state.tensors. This means the probability is computed from the original state, not the post-jump state.Compare with the 1-site case (lines 103-104) where the tensor is updated before computing the norm. The 2-site path should follow the same pattern.
🐛 Proposed fix: move norm computation after tensor update
merged = merge_two_site(tensor_left, tensor_right) # apply the 2-site jump operator merged = oe.contract("ab, bcd->acd", jump_op, merged) - dp_m = dt * gamma * jumped_state.norm(site) # split the tensor (always contract singular values right for probabilities) tensor_left_new, tensor_right_new = split_two_site( merged, [state.physical_dimensions[site], state.physical_dimensions[site + 1]], svd_distribution="right", trunc_mode=cast("TruncMode", sim_params.trunc_mode), threshold=sim_params.threshold, max_bond_dim=sim_params.max_bond_dim, min_bond_dim=sim_params.min_bond_dim, ) jumped_state.tensors[site], jumped_state.tensors[site + 1] = tensor_left_new, tensor_right_new # compute the norm at `site` - + dp_m = dt * gamma * jumped_state.norm(site) dp_m_list.append(float(dp_m.real))🤖 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/stochastic_process.py` around lines 123 - 140, The two-site jump probability dp_m is computed using jumped_state.norm(site) before writing the post-jump tensors back, so move the dp_m calculation to after the call to split_two_site and after assigning jumped_state.tensors[site], jumped_state.tensors[site + 1] (i.e., compute dp_m = dt * gamma * jumped_state.norm(site) only once tensor_left_new and tensor_right_new from split_two_site have been stored); keep the merge_two_site, oe.contract, split_two_site, and truncation parameters as-is and only reorder the dp_m and dp_m_list.append lines so the norm reflects the updated state.
🤖 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 `@CHANGELOG.md`:
- Line 14: Add an UPGRADING.md entry describing the new linalg submodule
introduced in the CHANGELOG (PR `#428`, `@aaronleesander`): explain the user-facing
behavior and stability changes (including that BLAS thread oversubscription is
addressed), list any breaking or configuration impacts, provide concise
migration steps and recommended actions (e.g., how to switch to the linalg
submodule, recommended BLAS/OpenMP thread settings to avoid oversubscription,
and sample config keys to update), and include a short note referencing the
CHANGELOG line so users can correlate the release note with the upgrade
instructions.
- Line 14: Update the changelog entry "added linalg submodule to open a new path
for optimizations and stop BLAS thread oversubscription for stability ([`#428`])"
to reference [`#429`] instead of [`#428`], and add a new PR link reference "[`#429`]:
.../pull/429" to the PR links section so the entry points to the correct pull
request; ensure the link follows the existing PR link formatting used in the
file.
In `@src/mqt/yaqs/core/linalg/_threading.py`:
- Around line 24-27: Move the conditional import out of the function by adding a
top-level guarded import: wrap "from threadpoolctl import threadpool_limits" in
a try/except ImportError and set threadpool_limits = None in the except branch,
removing the "# noqa: PLC0415" comment; then modify threadpool_limits_one() to
check if threadpool_limits is None and return contextlib.nullcontext() in that
case, otherwise return threadpool_limits(limits=1). Ensure you reference the
module-level name threadpool_limits and the function threadpool_limits_one in
the change.
In `@src/mqt/yaqs/core/linalg/eigh.py`:
- Around line 36-46: Update the Google-style docstring for the public function
eigh_tridiagonal in eigh.py by adding an "Args:" section immediately after the
initial description: list parameters d (array_like): diagonal elements of the
tridiagonal matrix, e (array_like): off-diagonal elements, lapack_driver (str,
optional): LAPACK driver to use, default "stemr", and check_finite (bool,
optional): whether to check input arrays for finiteness, default True; give
one-line descriptions and default values where applicable, and keep the existing
"Returns:" paragraph unchanged so the docstring conforms to the repository's
Google-style docstring guidelines.
In `@src/mqt/yaqs/core/linalg/svd_utils.py`:
- Around line 65-69: The loop computing how many singular values to keep has an
off-by-one: when iterating reversed(s_vec) and hitting the threshold you set
keep = n - idx which leaves one extra value; change the calculation to subtract
the current index as well (e.g., keep = n - (idx + 1) or keep = n - idx - 1)
while still applying the min_keep clamp so the final keep respects min_keep and
correctly excludes the discarded element; update the calculation in the block
that uses reversed(s_vec)/discard/threshold to use this corrected expression.
- Around line 74-77: The current logic sets keep = min(keep, max_bond_dim) then
enforces keep = max(keep, min_keep), which lets min_keep override max_bond_dim;
update the code in the svd routine that computes keep to either (a) validate
inputs up-front (raise ValueError if max_bond_dim is not None and max_bond_dim <
min_keep) or (b) enforce the cap last by computing keep = max(keep, min_keep)
followed by return min(keep, n, max_bond_dim) (or equivalent) so that
max_bond_dim always remains a hard cap; reference the variables max_bond_dim,
min_keep, keep and the return computation to locate where to change.
In `@src/mqt/yaqs/core/linalg/svd.py`:
- Around line 59-68: The public function svd() docstring lacks a Google-style
Args: section; update the docstring for svd to use full Google-style sections:
add an "Args:" section listing each parameter (e.g., a, full_matrices,
compute_uv, lapack_driver, overwrite_a, check_finite, and any other supported
kwargs) with types and brief descriptions, keep the existing "Returns:" section
(describe return types for both compute_uv True/False), and preserve the
existing summary and behavior notes about driver fallback and retry behavior;
ensure formatting matches the project's Google-style examples and mirrors
scipy.linalg.svd semantics.
In `@tests/core/linalg/test_svd.py`:
- Around line 50-52: The test test_svd_explicit_gesvd_driver compares singular
values from our linalg.svd (called with lapack_driver="gesvd") against SciPy,
but the reference SciPy call uses lapack_driver="gesdd"; change the SciPy
reference call in this test to use lapack_driver="gesvd" so both sides use the
same driver (update the scipy.linalg.svd call that assigns s_ref to pass
lapack_driver="gesvd").
---
Outside diff comments:
In `@src/mqt/yaqs/core/methods/stochastic_process.py`:
- Around line 123-140: The two-site jump probability dp_m is computed using
jumped_state.norm(site) before writing the post-jump tensors back, so move the
dp_m calculation to after the call to split_two_site and after assigning
jumped_state.tensors[site], jumped_state.tensors[site + 1] (i.e., compute dp_m =
dt * gamma * jumped_state.norm(site) only once tensor_left_new and
tensor_right_new from split_two_site have been stored); keep the merge_two_site,
oe.contract, split_two_site, and truncation parameters as-is and only reorder
the dp_m and dp_m_list.append lines so the norm reflects the updated state.
🪄 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: abcf7db3-c5be-4b68-acff-e8676ed95774
📒 Files selected for processing (29)
CHANGELOG.mdsrc/mqt/yaqs/analog/lindblad.pysrc/mqt/yaqs/analog/mcwf.pysrc/mqt/yaqs/core/data_structures/mpo.pysrc/mqt/yaqs/core/data_structures/mps.pysrc/mqt/yaqs/core/libraries/circuit_library_utils.pysrc/mqt/yaqs/core/libraries/gate_library.pysrc/mqt/yaqs/core/linalg/__init__.pysrc/mqt/yaqs/core/linalg/_threading.pysrc/mqt/yaqs/core/linalg/eigh.pysrc/mqt/yaqs/core/linalg/expm.pysrc/mqt/yaqs/core/linalg/svd.pysrc/mqt/yaqs/core/linalg/svd_utils.pysrc/mqt/yaqs/core/methods/decompositions.pysrc/mqt/yaqs/core/methods/dissipation.pysrc/mqt/yaqs/core/methods/matrix_exponential.pysrc/mqt/yaqs/core/methods/scheduled_jumps.pysrc/mqt/yaqs/core/methods/stochastic_process.pysrc/mqt/yaqs/core/methods/tdvp.pysrc/mqt/yaqs/digital/utils/mpo_utils.pytests/conftest.pytests/core/linalg/__init__.pytests/core/linalg/test_eigh.pytests/core/linalg/test_expm.pytests/core/linalg/test_svd.pytests/core/linalg/test_svd_utils.pytests/core/methods/test_decompositions.pytests/core/methods/test_tdvp.pytests/digital/utils/test_mpo_utils.py
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Member
Author
|
@CodeRabbit full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
src/mqt/yaqs/core/linalg/svd_utils.py (1)
71-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix off-by-one in
discarded_weightkeep computation.The current expression keeps one extra singular value when threshold is crossed. Subtract the current index as well.
🐛 Proposed fix
for idx, s in enumerate(reversed(s_vec)): discard += float(s) ** 2 if discard >= threshold: - keep = max(n - idx, min_keep) + keep = max(n - (idx + 1), min_keep) break🤖 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/linalg/svd_utils.py` around lines 71 - 75, The keep computation in the reversed(s_vec) loop is off-by-one: when discard crosses threshold the code sets keep = max(n - idx, min_keep) but must subtract the current index too because idx counts from 0 in the reversed order; update the assignment to compute keep = max(n - idx - 1, min_keep) (use the same variables n, idx, min_keep) so the current singular value is not incorrectly retained.
🤖 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/data_structures/mps.py`:
- Around line 544-552: The call to split_two_site in mps.py hardcodes
min_bond_dim=2 which conflicts with the caller-exposed max_bond_dim from
truncate(); update the split_two_site calls (the one shown and the similar ones
around the other occurrences) to compute min_bond_dim = min(1, max_bond_dim) or
better: clamp min_bond_dim to not exceed the caller max_bond_dim (e.g.
min_bond_dim = min(1, max_bond_dim) or min_bond_dim = max(1, min(desired_min,
max_bond_dim)) ), or simply set min_bond_dim=1 when constructing the
split_two_site call so the user-provided max_bond_dim=1 is respected; ensure
this change is applied to every split_two_site invocation in this module
(references: split_two_site, truncate()).
In `@src/mqt/yaqs/core/linalg/expm.py`:
- Around line 39-53: The ishermitian function currently lets np.allclose raise
on non-square inputs; add an explicit guard at the start of ishermitian to
return False for any input that is not a 2-D square matrix (e.g., check a.ndim
== 2 and a.shape[0] == a.shape[1]) before performing the conjugate-transpose
comparison with np.allclose, preserving the existing rtol/atol behavior and
return type.
In `@src/mqt/yaqs/core/methods/decompositions.py`:
- Around line 129-133: Check and validate the length of the physical_dimensions
sequence before indexing: ensure physical_dimensions has exactly two elements
(or at least two, per intended contract) and raise a ValueError with the
existing message if not; move this check to precede the lines that assign d_left
and d_right (references: physical_dimensions, d_left, d_right, merged) so
malformed inputs produce the documented ValueError instead of an IndexError.
In `@tests/core/linalg/test_svd_utils.py`:
- Around line 33-35: The test is intended to exercise the smax==0 branch of
linalg.truncate but uses s0 = np.array([0.0, 1.0]) (smax==1.0); change the test
vector to an all-zero vector (e.g., s0 = np.array([0.0, 0.0], dtype=np.float64))
so that smax==0 and the call to linalg.truncate(s0, mode="relative",
threshold=0.1, min_keep=1) truly hits and verifies the zero-maximum branch
(keep0 == 1).
In `@tests/core/methods/test_decompositions.py`:
- Around line 10-11: Update the module docstring to accurately reflect the
current tests: mention that the file covers left and right QR decompositions as
well as split_two_site behavior and linalg.svd edge-cases; locate the
top-of-file docstring (the existing one about left/right QR) and expand or
replace it to include split_two_site and linalg.svd so readers know the full
test scope.
- Around line 163-183: Add Google-style docstrings for the three helper
functions: _rand_unitary_like, _theta_from_singulars, and _as_merged_two_site.
For each function include a one-line summary, an Args section documenting
parameters (with types and meaning, e.g., seed, m, n, s, d0/d1/d_left/d_right),
a Returns section describing the return type (NDArray[np.complex128]) and
shape/meaning, and any relevant notes about behavior (e.g., uses QR to produce a
random unitary-like matrix, constructs theta from singular values,
reshapes/transposes merging two sites). Place the docstrings immediately above
each function in Google style.
In `@tests/core/methods/test_tdvp.py`:
- Line 46: The private import _split_two_site_tdvp is flagged and the
suppression # noqa: PLC2701 needs a short justification or removal; either
remove the noqa and refactor the test to exercise the public API instead, or
keep the private import but add an inline comment explaining why white-box
access is required for this test (e.g., "requires internal split behavior to
validate two-site TDVP edge cases"), and retain the noqa only after that
explanatory comment next to the import in tests/core/methods/test_tdvp.py so
reviewers understand why the private symbol is used.
---
Duplicate comments:
In `@src/mqt/yaqs/core/linalg/svd_utils.py`:
- Around line 71-75: The keep computation in the reversed(s_vec) loop is
off-by-one: when discard crosses threshold the code sets keep = max(n - idx,
min_keep) but must subtract the current index too because idx counts from 0 in
the reversed order; update the assignment to compute keep = max(n - idx - 1,
min_keep) (use the same variables n, idx, min_keep) so the current singular
value is not incorrectly retained.
🪄 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: dbd83d85-882a-424f-97b5-f4e092e73d71
📒 Files selected for processing (29)
CHANGELOG.mdsrc/mqt/yaqs/analog/lindblad.pysrc/mqt/yaqs/analog/mcwf.pysrc/mqt/yaqs/core/data_structures/mpo.pysrc/mqt/yaqs/core/data_structures/mps.pysrc/mqt/yaqs/core/libraries/circuit_library_utils.pysrc/mqt/yaqs/core/libraries/gate_library.pysrc/mqt/yaqs/core/linalg/__init__.pysrc/mqt/yaqs/core/linalg/_threading.pysrc/mqt/yaqs/core/linalg/eigh.pysrc/mqt/yaqs/core/linalg/expm.pysrc/mqt/yaqs/core/linalg/svd.pysrc/mqt/yaqs/core/linalg/svd_utils.pysrc/mqt/yaqs/core/methods/decompositions.pysrc/mqt/yaqs/core/methods/dissipation.pysrc/mqt/yaqs/core/methods/matrix_exponential.pysrc/mqt/yaqs/core/methods/scheduled_jumps.pysrc/mqt/yaqs/core/methods/stochastic_process.pysrc/mqt/yaqs/core/methods/tdvp.pysrc/mqt/yaqs/digital/utils/mpo_utils.pytests/conftest.pytests/core/linalg/__init__.pytests/core/linalg/test_eigh.pytests/core/linalg/test_expm.pytests/core/linalg/test_svd.pytests/core/linalg/test_svd_utils.pytests/core/methods/test_decompositions.pytests/core/methods/test_tdvp.pytests/digital/utils/test_mpo_utils.py
Member
Author
|
@CodeRabbit full review |
Contributor
✅ Actions performedFull review triggered. |
This was referenced May 26, 2026
11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR creates a linalg submodule to mimic scipy. It includes several of the robust optimizations that were previously scattered throughout the library. This includes LAPACK fallbacks to stabilize the SVD as well as single threading for dense matrix exponentiation to avoid oversubscription due to parallelization in trajectories.
Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.