added Krylov tolerance as high-level parameter#439
Conversation
…toolkit/yaqs into krylov-threshold
|
@CodeRabbit review |
aaronleesander
changed the title
✅ Actions performedReview triggered.
|
|
Warning Review limit reached
More reviews will be available in 16 minutes and 31 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe PR refactors the simulation parameter API by replacing the ChangesSimulation Parameter API Refactoring
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/mqt/yaqs/core/methods/tdvp.py (2)
72-77:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the touched docstrings to the renamed parameter names.
These docstrings still describe the legacy
thresholdfield even though the implementation now consumessim_params.svd_thresholdandsim_params.krylov_tol. Keeping the old names here will make the migration path harder to follow.Also applies to: 572-574, 687-692, 831-835
🤖 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/tdvp.py` around lines 72 - 77, Update the docstrings in tdvp-related functions (e.g., the TDVP method in src/mqt/yaqs/core/methods/tdvp.py) to replace legacy names like "threshold" with the current sim_params fields: reference sim_params.svd_threshold for SVD truncation tolerance and sim_params.krylov_tol for Krylov solver tolerance (and mention sim_params.trunc_mode/svd_distribution/max_bond_dim as applicable); make the same replacements in the other docstring blocks referenced around the file (the blocks near the top-level TDVP function and the other two docstring occurrences) so examples and parameter descriptions match the actual parameter names used by the implementation.
455-467: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winConvert the updated docstrings to Google style while touching them.
The newly added
krylov_tolentries keep the oldname (type):format, so this file still diverges from the repository’s Python docstring convention.As per coding guidelines,
**/*.py: Use Google-style docstrings in Python code.Also applies to: 504-513, 538-546
🤖 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/tdvp.py` around lines 455 - 467, The docstring for the TDVP evolution function still uses the old "name (type):" param style for the newly added krylov_tol and similar entries; update these to Google-style parameter entries (under Args:) e.g. "krylov_tol: float, optional. Tolerance for the adaptive Krylov/Lanczos matrix exponential." and similarly convert any other parameter entries in the docstrings around the tdvp-related functions (references: the docstring block containing projector, tensor, dt, proj_args, dense_threshold, krylov_tol and the other docstrings at the ranges noted) so all parameter lines follow "name: type, optional. Description." Keep descriptions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mqt/yaqs/core/data_structures/simulation_parameters.py`:
- Around line 51-59: Add a Google-style "Args:" section to the _validate_preset
docstring documenting the preset parameter (name, type SimulationPreset, and
what values/semantics are expected) so the docstring includes Args, Returns, and
Raises; update the docstring for the _validate_preset function to follow the
repo's Google-style format and describe the preset argument concisely.
- Around line 210-217: The constructor currently uses max_bond_dim: int | None =
None which conflates “omit to use preset” and “None means no cap”; change the
API to use a unique sentinel default so None can mean no cap: introduce a
module-level sentinel (e.g. _USE_PRESET = object()) and change the signature to
max_bond_dim: int | None | object = _USE_PRESET, then update all internal checks
(places that currently treat None as “use preset” — e.g. in
SimulationParameters.__init__ and the logic at the sites corresponding to the
prior lines 279-280, 334-340, 390-391, 437-443, 472) to treat _USE_PRESET as
“take preset value” and preserve None as “no cap”; finally update the
constructor and class docstrings to state “omit max_bond_dim to use the preset;
pass None for no cap.”
In `@tests/core/data_structures/test_simulation_parameters.py`:
- Around line 111-113: Change the test parameter typing so valid preset values
are typed as SimulationPreset instead of a bare str and remove the unnecessary
"# ty: ignore[invalid-argument-type]" comments; specifically update the test
function test_analog_simparams_presets(preset: str, ...) to preset:
SimulationPreset and drop the ty ignores on the AnalogSimParams(...) calls (and
make the analogous edits in the other test functions mentioned around the same
blocks for lines 130-132 and 170-172) so the tests keep correct static typing
without suppression.
In `@UPGRADING.md`:
- Around line 40-42: Update the migration note that currently reads "Accuracy
presets use the key `svd_threshold` in `ACCURACY_PRESETS`" so it references the
new preset API: replace `ACCURACY_PRESETS` with `SIMULATION_PRESETS` (keep
`svd_threshold` and surrounding wording unchanged) to reflect the refactor and
avoid confusing users; search for the exact phrase containing `svd_threshold`
and `ACCURACY_PRESETS` and update it to mention `SIMULATION_PRESETS`.
---
Outside diff comments:
In `@src/mqt/yaqs/core/methods/tdvp.py`:
- Around line 72-77: Update the docstrings in tdvp-related functions (e.g., the
TDVP method in src/mqt/yaqs/core/methods/tdvp.py) to replace legacy names like
"threshold" with the current sim_params fields: reference
sim_params.svd_threshold for SVD truncation tolerance and sim_params.krylov_tol
for Krylov solver tolerance (and mention
sim_params.trunc_mode/svd_distribution/max_bond_dim as applicable); make the
same replacements in the other docstring blocks referenced around the file (the
blocks near the top-level TDVP function and the other two docstring occurrences)
so examples and parameter descriptions match the actual parameter names used by
the implementation.
- Around line 455-467: The docstring for the TDVP evolution function still uses
the old "name (type):" param style for the newly added krylov_tol and similar
entries; update these to Google-style parameter entries (under Args:) e.g.
"krylov_tol: float, optional. Tolerance for the adaptive Krylov/Lanczos matrix
exponential." and similarly convert any other parameter entries in the
docstrings around the tdvp-related functions (references: the docstring block
containing projector, tensor, dt, proj_args, dense_threshold, krylov_tol and the
other docstrings at the ranges noted) so all parameter lines follow "name: type,
optional. Description." Keep descriptions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4c6aac27-dfac-4234-9c73-b06684310ddb
📒 Files selected for processing (21)
CHANGELOG.mdUPGRADING.mddocs/examples/analog_simulation.mddocs/examples/ensemble_evolution.mddocs/examples/simulation_parameters.mddocs/examples/simulator_initialization.mddocs/examples/strong_circuit_simulation.mddocs/examples/weak_circuit_simulation.mddocs/index.mdsrc/mqt/yaqs/analog/lindblad.pysrc/mqt/yaqs/core/data_structures/simulation_parameters.pysrc/mqt/yaqs/core/methods/bug.pysrc/mqt/yaqs/core/methods/dissipation.pysrc/mqt/yaqs/core/methods/scheduled_jumps.pysrc/mqt/yaqs/core/methods/stochastic_process.pysrc/mqt/yaqs/core/methods/tdvp.pytests/analog/test_ensemble.pytests/core/data_structures/test_simulation_parameters.pytests/core/methods/test_bug.pytests/core/methods/test_tdvp.pytests/test_simulator.py
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
1 participant
Description
This PR adds the Krylov tolerance used within the matrix exponential of TDVP as a high-level parameter.
This is based off work by @martinamonctum and its behavior is currently active research.
Additionally, it renames the threshold to svd_threshold to differentiate this setting more clearly.
Finally, an extra "exact" preset was added with the new tolerance.
The purpose of overriding tolerance values etc. is made more clear in the relevant docs example.
Checklist
If PR contains AI-assisted content:
Assisted-by: [Model Name] via [Tool Name]footer.