Use exact FCI traces for matrix exponentials - #279
Open
danielgaskins wants to merge 5 commits into
Open
Conversation
|
|
Member
|
Thank you for your contribution! I am a bit swamped this week, but will review your PR later next week! 👍 |
mrossinek
self-requested a review
August 20, 2026 08:30
mrossinek
added
performance
mrossinek
reviewed
Aug 27, 2026
mrossinek
left a comment
mrossinek
left a comment
Member
There was a problem hiding this comment.
Thanks a lot for getting this started! This already looks like it is in good shape. I will push some changes myself to improve upon the private attirbute patching.
…ribute The exact-trace change plumbed the trace to the matrix-exponential callers by patching a private `_trace` attribute onto the SciPy `LinearOperator` returned by `_linear_operator_`. That coupled a *public* protocol's return value to metadata only this package's wrapper sets: `SupportsLinearOperator` promises nothing more than a `scipy.sparse.linalg.LinearOperator`, so any other conforming operator reaching `Evolution` raised `AttributeError` on the read. The `isinstance` FermionOperator guard hid that today, but the comment above it records the intent to remove the guard once `SupportsLinearOperator` folds into `OperatorTrait` -- at which point the crash goes live. A SciPy `LinearOperator` is also simply the wrong carrier: it has nowhere to put the value, and composing one drops any attached attribute (`scale * linop` returns a bare `_ScaledLinearOperator`), so the metadata survived exactly one hop by luck. The trace does not need transporting at all. The internal `_fci_linear_operator_` carrier already returns the native `FciLinearOperator`, which exposes `trace` as a real, typed, documented getter next to the matrix-vector action. Going through that carrier gives both pieces from one object, so the private attribute, both `cast(Any, ...)` escapes and a stale `type: ignore` all disappear, and the public wrapper goes back to returning an unadorned operator. This keeps the numerical win intact: still 82x faster and ~63000x more accurate than `traceA=0.0` on a large-trace operator, measured against a dense `expm` reference. Behaviour is unchanged -- the trace handed to SciPy is bit-identical. The shared helper now takes the operator rather than a prebuilt operator/trace pair, which also confines the FCI-kernel knowledge to one place instead of leaving the fragile lookup duplicated at both call sites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`unittest.mock` was the only such usage in the suite -- everything else patches via pytest's `monkeypatch` -- and one test already mixed both styles, taking `monkeypatch` while importing `patch`. Porting them made it clear most had stopped earning their keep. Before the kernel refactor, every caller derived the trace itself (`_trace` off the linop, times `scale` for `Evolution`), so asserting the `traceA` kwarg per caller was the only way to catch a call site getting that arithmetic wrong. That derivation now happens once, inside `_expm_multiply_fci`; the callers just forward `scale`, and `OrbitalRotation` does not even do that. So the per-caller tests guarded arithmetic that no longer exists where they were looking, and a third test asserted `traceA == 8.5` when `test_fci_linear_operator.py` already asserts `kernel.trace == 8.5` directly, with no patching at all. All three are removed. What was never tested is the part that actually matters. `traceA` cannot change the result -- SciPy factors out `exp(traceA / n)` and multiplies it back -- so the sole observable effect of getting it right is how much work SciPy does. The new preconditioning test counts matrix-vector products: 6 with the exact trace versus 1374 without (~229x), asserted at 10x so it does not drift across SciPy versions. That is black-box, and it is the whole point of the branch. One kwarg assertion survives, for `trace(c * A) == c * trace(A)`: an unscaled trace still returns the right vector, only worse-conditioned, so it has no signature in the result and has to be observed where it is applied. Both replacements were checked against mutations of the source -- dropping the trace (`traceA=0.0`) and leaving it unscaled (`traceA=kernel.trace`) -- each of which fails three tests, so these assert on behaviour rather than passing by construction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mrossinek
self-requested a review
August 27, 2026 17:02
|
coverage: 98.045% (+0.02%) from 98.021% — danielgaskins:fix/exact-expm-trace into Qiskit:main |
Author
|
Thanks for improving this. Reading the trace directly from the FCI kernel is much cleaner than attaching it to the SciPy operator. I reviewed the updated branch, and the full check suite is passing. |
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
4 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.
Summary
The compiled FCI representation already contains every transition in the requested particle-number sector. This change uses that representation to calculate the exact sector trace and carries it through the SciPy linear operator wrapper.
Evolutionand the generalOrbitalRotationpath now pass the exact trace toexpm_multiply, including the appropriate time scaling for evolution. This removes the hard-coded zero without adding the extra work or nondeterminism of SciPy's trace estimator.Fixes #190
Testing
python -m pytest -q tests/pythontox -e lintcargo test -p qiskit-fermions-core --features pyext --lib linalg::fcicargo fmt --checkcargo clippy -p qiskit-fermions-core --features pyext --all-targets -- -D warningscargo clippy -p qiskit-fermions-pyext --all-targets -- -D warningsAI tool disclosure
OpenAI Codex with GPT-5 was used while preparing the implementation and tests.