Perf: reduced-op eigensolver - #97
Conversation
`get_extremal_eigenvalue` converted the operator to a 2^n sparse matrix and
ran an iterative Davidson solve for the smallest eigenvalue. Instead,
exploit that the spectral norm of a Hermitian Pauli sum depends only on the
*-algebra its terms generate: that algebra is M_{2^p} tensored with a
2^c-dimensional center, so the operator reduces to a 2^(p+c)-dimensional one
(p = independent anticommuting pairs, c = commuting directions) whose size
is set by the algebra, not the qubit count n.
The reduction (symplectic Gram-Schmidt over GF(2), with phases read from
Qiskit's Pauli arithmetic rather than derived by hand) yields a small
operator that is diagonalized densely and exactly when 2^(p+c) is small, and
otherwise handed to the iterative Davidson solver on the reduced (never the
full 2^n) operator. For the commutators encountered here this is exact,
avoiding potential convergence issues, and can be much faster.
The public (converged, eigenvalue) contract is unchanged.
Written with assistance of Claude ai tool.
Coverage Report for CI Build 33086560019Coverage increased (+2.5%) to 78.737%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Updated pickles verified by dense numpy eigvalsh on the original commutator (not reduced operator), agreeing to numerical precision (<1e-13).
uses private attributes inside qiskit so may need to revisit
- use new _reduce_operator() return signature - shrink size of exponentially large array made in p == 0 case (no anticommuting generators)
|
Compared to 1 month ago, main subroutines are now much more readable, at least for someone versed in qiskit.quantum_info. Custom function names are more descriptive; some custom code has been replaced with quantum_info methods. Overall I think |
mrossinek
left a comment
There was a problem hiding this comment.
Thanks a lot! I think I can follow your explanation in the PR and agree with the approach taken here. The code also looks reasonable to me, but it was not super easy to account for all potential edge cases. For this, I told Claude to do a review of this PR and here are its findings:
HIGH — qiskit_addon_slc/utils/davidson.py:94: lowest = 0.0 initialization clamps the result
The dense per-sector loop seeds lowest = 0.0 and then only ever takes min(lowest, ...).
If every sector's smallest eigenvalue is positive, the true minimum is never recorded and the
function returns 0.0.
spo = SparsePauliOp(["XI", "ZI", "II"], [0.5, 0.5, 3.0])
get_extremal_eigenvalue(spo) # -> (True, 0.0); true minimum is +2.2928932188134525Fuzzing 300 random Hermitian operators hit this 8 times — any operator with an identity term
large enough to shift the spectrum positive triggers it. The correct seed is +inf (or
float(np.linalg.eigvalsh(...)[0]) on the first sector).
get_extremal_eigenvalue is a documented public API (in qiskit_addon_slc/utils/__init__.py's
__all__, with an autofunction entry) and its docstring promises "the most-negative eigenvalue
of spo" for any Hermitian input. The addon's own caller only ever passes traceless commutators,
so production paths are unaffected — but the exported contract is broken.
MEDIUM — qiskit_addon_slc/utils/davidson.py:74: the p == 0, c == 0 case returns 0.0, discarding the operator's constant value
When spo is a multiple of the identity, _reduce_operator returns a 0-qubit SparsePauliOp.
Qiskit 2.5.2's to_matrix() on a 0-qubit operator yields [[0.+0.j]], dropping the coefficient,
so .diagonal().real.min() is 0.0:
get_extremal_eigenvalue(SparsePauliOp(["II"], [-0.73])) # -> (True, 0.0); true minimum is -0.73This is also the only case a 400-seed _reduce_operator spectrum fuzz flagged, confirming the
reduction itself is otherwise sound. forward.py:139-144 short-circuits identity-only commutators
before this call, so the addon's own path is safe, but the public API is not. Guarding on
reduced_op.num_qubits == 0 and returning float(sum(coeffs).real) fixes it.
LOW — tests/utils/test_davidson.py:41: the new test fixtures all have negative minima, so neither bug above is covered
All four DENSE_OPERATORS entries plus both other new tests have a strictly negative minimum
eigenvalue, so the lowest = 0.0 clamp is invisible to the suite. Adding one positive-definite
fixture (e.g. SparsePauliOp(["XI", "ZI", "II"], [0.5, 0.5, 3.0])) and one identity-only fixture
would catch both.
LOW — qiskit_addon_slc/utils/davidson.py:111: the new tol=1e-10 default is dead in the addon's own code path
forward.py:187 calls get_extremal_eigenvalue(commutator, tol=atol_eigenvalue), and
atol_eigenvalue defaults to 1e-8, so default_kwargs.update(kwargs) always overrides the new
tighter default. The docstring's rationale ("the default tol is tight because the eigenvalue
error runs well above tol") therefore does not apply to any in-repo caller; if the tightening is
meant to take effect for bound computations, atol_eigenvalue's default needs the same change.
Verified as correct (no finding)
Fuzz-tested the riskiest parts of the new reduce_op.py and found them sound:
_get_basismaintains proper reduced-row-echelon invariants (300 seeds)_symplectic_gram_schmidt's recursive center resolution preserves the pair/center commutation
structure, and_get_basis(center)re-reduction does not break it (400 seeds)- Pauli phases are correctly recovered through the
omega_phasecomputation, including signed and
imaginary input labels - The reduced operator always keeps the trailing
cqubits Z-only, which the sector-sign logic in
davidson.pydepends on (500 seeds) c <= nalways holds, so thep == 0sparse-diagonal memory claim is validbool(np.atleast_1d(converged)[0])correctly unwrapsdavidson1's ndarray return- The in-loop
block.coeffsreassignment is picked up byto_matrix()with no stale caching - The iterative path matches dense diagonalization on 60 traceless operators
The dense per-sector branch of get_extremal_eigenvalue() seeded its running
minimum with 0.0 and then only ever took min(lowest, ...). For an operator
whose spectrum lies entirely above zero, no sector minimum is ever below the
seed, so the function returned 0.0 instead of the true minimum:
SparsePauliOp(["XI", "ZI", "II"], [0.5, 0.5, 3.0])
-> (True, 0.0), true minimum +2.2928932188134525
Seed with +inf instead. The addon's own caller only passes traceless
commutators, whose spectra are symmetric about zero, so the bounds path was
unaffected -- but get_extremal_eigenvalue() is public and its docstring
promises the most-negative eigenvalue for any Hermitian input.
Adds a positive-definite fixture covering the per-sector path (which fails
without this fix) and a positive-definite fully-commuting one covering the
same spectrum shape on the p == 0 diagonal path.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
When spo is a multiple of the identity, _reduce_operator() reduces it to a
0-qubit SparsePauliOp. SparsePauliOp.to_matrix() on 0 qubits returns [[0]] --
the coefficients are dropped -- so the p == 0 diagonal branch reported 0.0
regardless of the operator's actual value:
SparsePauliOp(["II"], [-0.73]) -> (True, 0.0), true minimum -0.73
Handle num_qubits == 0 before the diagonal branch and return the sum of the
coefficients (summed rather than taken from a single term, since the input need
not be simplified).
forward.py short-circuits identity-only commutators before reaching here, so
the bounds path was unaffected; get_extremal_eigenvalue() is public, so its
contract was not.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nds path _davidson_extremal_eigenvalue()'s docstring explains why its tol default is tight, which reads as though bound computations get that accuracy. They do not: time_evolved_norm_forward() always forwards atol_eigenvalue (default 1e-8) as tol, so default_kwargs.update(kwargs) overrides the tighter value for every in-repo caller. Say so, and point at atol_eigenvalue as the knob that actually governs bound accuracy. Retuning that default is deliberately left alone: it is public API, and the `atol` deprecation shim in forward.py uses 1e-8 as its sentinel for "the user did not set atol_eigenvalue", so changing it would silently break that detection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both fixed cases are reachable through documented public API, so they get a reno entry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
In some cases, the main bottleneck in the SLC computation is getting the spectral norm (i.e. largest magnitude eigenvalue) of the commutator
[error, observable]. This matrix is sparse, but still potentially really big. This PR provides an optimization so we can work with a smaller matrix (fewer effective qubits) that still captures all distinct eigenvalues of the original operator. The main work of this PR is in the_reduce_operatorfunction.Additionally:
On a 1D 50-qubit Ising circuit with rx angle of pi/16 and rzz angle of -pi/2, this PR enabled
compute_forward_boundsto obtain several more layers of bounds within a 5-minute timeout window (up to ~16k error-generators processed from an original ~14k -- significant given subsequent layers are potentially exponentially harder to process).(This PR was developed with assistance from AI tool Claude. This PR summary write-up is not AI.)
_reduce_operator(SparsePauliOp)
For an operator with linearly-independent Pauli terms (i.e. a full rank tableau), the unique eigenvalues are set by the pairwise commutation relations between terms (and the coefficients). So one goal condition is to produce an operator with the same pairwise commutation relations, but fewer qubits. This removes degeneracies in which the same eigenvalue appears multiple times in the spectrum. For a trivial example, if your original operator Paulis were
(XXX, ZZZ), you'd get the same pairwise commutation relations by the new Paulis(X, Z). The general procedure is more complicated.We will reduce initial Paulis and coefficients
{(P_i, c_i)}to fewer-qubit Paulis and coeffs{(R_i, r_i)}(wherer_iis same asc_iup to a possible phase).Steps:
{G_j}, building-blocks we can multiply together to produce anyP_i. This shrinks the number of Paulis we need to deal with. If we can express these building-blocks as Paulis on fewer qubits, then we can use them to build a fewer-qubit operator that is equivalent to the original (same commutation relations between terms).X,Z) on the same qubit, reducing the qubit number.Afrom list of generators.Zoperator on a new qubit. We won't map any other generators to this qubit, guaranteeing the commutation relations are preserved.Bfrom the list. Represent(A, B)as(Z, X)on a new qubit. So that anticommutation property is preserved.{G}so that they all commute with bothAandB. IfGanticommutes withA(orB), multiplyGin-place byB(orA). The updated generators' tableaux still span the same space, so they still work as building-blocks for rebuilding our original operator. But sinceGnow commutes withAandB, we are free to assignGto a new reduced qubit. (This is the "symplectic Gram Schmidt" part).r_ican differ fromc_i).Choice of solver (secondary optimization)
The simple procedure is to pass the reduced operator to our existing Davidson solver. Since there are now fewer qubits, this can be significantly faster than without this PR.
For a small reduced operator, we do an additional optimization. In step 2 above, we identified commuting ("central") generators, corresponding to
Zon a reduced qubit, that generally commute with everything else. Say the loop producedcsuch generators, andppairs of anticommuting generators. We know each commutingZwill be replaced by an eigenvalue +1 or -1 everywhere it appears in the operator, but we don't know which. We can optionally plug in all2^ccombinations, then eigensolve2^csmaller operators instead of a single larger operator. This can be faster whenp+cis small.