Avoid building per-term QubitOperators in jordan_wigner interaction transform#1427
Conversation
…ransform The interaction-operator path built a fresh single-term QubitOperator for every Pauli string in the O(N^4) two-body loop and summed them in, so each term paid the constructor's parse, validation, simplify and copy. This accumulates the Pauli strings into a plain dict and builds the QubitOperator once at the end. The public jordan_wigner_one_body and jordan_wigner_two_body share the same term builders, so their output is unchanged, verified bit-exact against the previous implementation.
There was a problem hiding this comment.
Code Review
This pull request refactors the Jordan-Wigner transformation in jordan_wigner.py to accumulate Pauli terms in a dictionary instead of repeatedly instantiating and summing QubitOperator objects, which improves performance. The reviewer suggests further optimizing this by passing the terms dictionary directly into _one_body_terms and _two_body_terms for in-place accumulation, thereby eliminating the need for the _merge_terms helper function.
| def _merge_terms(terms, contribution): | ||
| """Add every term of a contribution dict into terms.""" | ||
| for key, value in contribution.items(): | ||
| _add_term(terms, key, value) |
There was a problem hiding this comment.
Thanks, but I'd keep _merge_terms and the two-step here. The aim is output that stays bit-for-bit identical to the current transform, and the two levels of dropping are what make that hold. Upstream builds each contribution as its own operator via jordan_wigner_one_body/two_body (whose += drops sub-tolerance terms on its own, against zero), then folds that into the running total with a second += that drops again. _one_body_terms/_two_body_terms plus _merge_terms mirror exactly that. Accumulating straight into the shared dict removes the first drop, so a sub-EQ_TOLERANCE contribution gets tested against the live total instead of against zero. Two places it shows up: an all-zero operator comes out as {} instead of {(): 0.0}, and a sub-tolerance term folded onto an existing value shifts it by up to EQ_TOLERANCE. Both stay within isclose, but they'd break the exact match, so I'll keep the helper.
| def _one_body_terms(p, q, coefficient): | ||
| r"""Terms of a^\dagger_p a_q + h.c. under Jordan-Wigner, as a dict.""" | ||
| terms = {} |
There was a problem hiding this comment.
Update _one_body_terms to accept an optional terms dictionary to allow in-place accumulation of terms.
| def _one_body_terms(p, q, coefficient): | |
| r"""Terms of a^\dagger_p a_q + h.c. under Jordan-Wigner, as a dict.""" | |
| terms = {} | |
| def _one_body_terms(p, q, coefficient, terms=None): | |
| r"""Terms of a^\dagger_p a_q + h.c. under Jordan-Wigner, as a dict.""" | |
| if terms is None: | |
| terms = {} |
There was a problem hiding this comment.
Keeping the dict-return form here, same reason as the _merge_terms thread: the local drop has to happen against zero, not against the running total.
| def _two_body_terms(p, q, r, s, coefficient): | ||
| r"""Terms of a^\dagger_p a^\dagger_q a_r a_s + h.c. under JW, as a dict.""" | ||
| terms = {} |
There was a problem hiding this comment.
Update _two_body_terms to accept an optional terms dictionary to allow in-place accumulation of terms.
| def _two_body_terms(p, q, r, s, coefficient): | |
| r"""Terms of a^\dagger_p a^\dagger_q a_r a_s + h.c. under JW, as a dict.""" | |
| terms = {} | |
| def _two_body_terms(p, q, r, s, coefficient, terms=None): | |
| r"""Terms of a^\dagger_p a^\dagger_q a_r a_s + h.c. under JW, as a dict.""" | |
| if terms is None: | |
| terms = {} |
There was a problem hiding this comment.
Same here, keeping the dict return so that first drop stays local.
Third of the operator hot-path findings from #1407; the earlier two are #1415 and #1417.
jordan_wigneron anInteractionOperatormaps the one- and two-body integralsstraight to Pauli terms instead of going through a
FermionOperatorfirst, and#587 points at exactly this routine as the fast path to build on ("there is a
special routine that maps the InteractionOperators directly to qubits ... and
that is faster"). #121 is the other side of it: it flagged this same routine as
"surprisingly slow" back in 2017 and got closed without a fix.
The transform walks every one-body pair and every pair of pairs, so the two-body
part is O(N^4). For each Pauli string it built a fresh single-term
QubitOperatorand summed that into the running total. Each of thoseconstructions parses the term, validates every
(index, action)factor, runs thePauli
_simplify, and in the three-unique-index case multiplies by aZoperator, which deepcopies. None of that is needed to add a term to a sum: a
QubitOperator'stermsis just a dict from a sorted tuple of(index, action)factors to a coefficient, and the strings this routine emits are already sorted
with their factors already combined, so there is nothing for the constructor to
normalize.
This accumulates the
pauli_string -> coefficientpairs into a plain dict with asmall
_add_termhelper that does what__iadd__/__isub__do to a singleterm (add onto the running value, drop it once it falls under
EQ_TOLERANCE), andbuilds one
QubitOperatorat the end by assigning the finished dict to.terms.Accumulating into a dict instead of building and summing single-term operators
avoids that per-term construction and validation, and it measures about 2x
faster, with identical output.
Public functions
jordan_wigner_one_bodyandjordan_wigner_two_bodyare public and re-exported,and
bksf_testandjordan_wigner_testcall them directly, so they keep theirsignatures and their output. I pulled the term-building logic into two private
helpers (
_one_body_terms,_two_body_terms) that return the dict, and thepublic functions and the interaction-op driver both go through them, so there is
one copy of the logic and the public results are unchanged. The
Ztimeshopping-term product in the three-unique case is a single-qubit relabeling (the
shared qubit carries at most a parity
Z, andZsquared is the identity), so itis a direct toggle of that index in the string now rather than an operator
multiply and deepcopy.
Correctness
Before editing the source I captured the current
jordan_wigneroutput on 104random real
InteractionOperators and saved it. The mix is physical(symmetric, molecular-style) operators, dense generic real tensors, sparse and
very sparse tensors, one-body-only, constant-only, and the exact-zero operator,
at N = 1, 2, 3, 4, 6, 8, 10, 12. The new code reproduces that captured output
exactly on every case: identical term-key sets, max absolute coefficient
difference 0, and
(new - old).induced_norm(1)equal to 0. It also matches anindependent dict-based reference I wrote separately, to the same 0. Seeds are
fixed, so the check is deterministic.
jordan_wigner_test.pyandbksf_test.pypass (the two public helpers arechecked there against the
FermionOperatorpath over a full index grid withcomplex coefficients), and
transforms/passes as a whole. black and mypy areclean on the file. Every changed line is hit by the existing suite except the
pre-existing
n_qubits < count_qubits(iop)guard, which the old code did notcover either and which I left untouched.
The routine is still documented as real-only and this does not change that. It
computes the same coefficients from the same integrals; only the container they
land in changed.
Benchmarks
Random real InteractionOperators, cumulative
jordan_wignertime. Medians over 5repeats within each of 6 alternated upstream/branch rounds, fixed seeds, on an
Intel Core Ultra 5 225 laptop (Windows, Python 3.12, numpy 2.5).