examples: SQD driver that grows its subspace via SBD's own carryover - #39
Open
Sophia Wen (hfwen0502) wants to merge 4 commits into
Open
Sophia Wen (hfwen0502) wants to merge 4 commits into
Sophia Wen (hfwen0502) wants to merge 4 commits into
Conversation
tpb_diag has always returned carryover_adet/carryover_bdet in its results dict -- bindings.cpp unpacks SBD's co_adet/co_bdet into it -- but _solve_sci_core discarded them, and _create_sbd_config force-set carryover_type to 0 *because* they were discarded, as its own comment said. Structurally the same gap the RDMs had: SBD computes the thing, the wrapper throws it away. extract_carryover() converts them out of SBD's packed half-determinant format into plain CI strings via _sbd_dets_to_ci_strings, so they can feed include_configurations directly. SCIResult is upstream's frozen dataclass, so a carryover field cannot be added the way rdm1/rdm2 were -- those already existed. Hence SBDCarryoverResult, a subclass. This is safe because qiskit-addon-sqd's loop reads only .energy/.sci_state/.orbital_occupancies, selects with min(results, key=...), and passes that same object through: SCIResult( is constructed exactly once in all of fermion.py, inside upstream's own PySCF solver. Verified on 4 ranks that the subclass survives both the loop and upstream's rank-0-to-all-ranks broadcast with carryover intact. _assert_carryover_survived turns that assumption into a loud failure rather than a silent one, since a stripped result would make a driver quietly stop expanding while still reporting convergence. carryover_type = 0 (the default) still returns a plain SCIResult, so the normal SQD path is byte-for-byte unaffected and pays nothing. Confirmed run_sbd_diag.py still runs with qiskit-addon-sqd and qiskit both absent -- the subclass sits behind the existing try/except ImportError guard and its only other references are inside function bodies. bindings.cpp also exposes the extended-carryover fields, kept behind the existing #ifdef since binding them unconditionally fails to compile against plain upstream.
…over Third SQD driver. Same outer-loop structure as run_sqd_enlarge_subspace_sbd.py -- diagonalize_fermionic_hamiltonian called with max_iterations=1 in its own loop, expanded determinants fed forward as the next round's include_configurations -- with the expansion step swapped. Instead of qiskit-addon-sqd's JAX enlarge_batch_from_transitions, the new determinants come from SBD itself, selected inside the same C++ diagonalization that just ran. Verified on H2O (bundled 275-bitstring pool, 8 ranks): -76.2421767512 over a 1742x1742 subspace, identical to all ten digits and over an identical final subspace to run_sqd_enlarge_subspace_sbd.py. That is the real correctness check -- SBD carryover type 3 and enlarge_batch_from_transitions should compute the same expansion inside the same loop. They also agree bit-for-bit at threshold 1e-5 (-76.2436018956, 5007^2) and 1e-6 (-76.2437251036, 7881^2). Two practical differences, both following from the expansion running in MPI-distributed C++ rather than JAX on every rank: - Runs on 8 GPU ranks with JAX_PLATFORMS unset -- the exact configuration where the JAX driver dies with CUDA_ERROR_OUT_OF_MEMORY, since several ranks each try to claim a device. No JAX is involved here at all. - Detects closure a round earlier. The JAX driver compares its 1720-string expansion against the 1742-string solved subspace, so its no-growth test can never fire and it always waits for energy convergence; this driver unions the carryover with the solved subspace first. --sbd_carryover_type 0 is rejected up front rather than looping forever with nothing to expand. Values past 3 are accepted and passed through, but not advertised, since they need a build this repo does not ship; --sbd_eri_threshold is likewise suppressed from --help. Not yet demonstrated: the expansion-speed advantage. Per-round time is a wash at H2O size (38.02s vs 38.43s), because the expansion is negligible against a 3M-determinant diagonalization.
… drivers examples/README.md's parameter reference had drifted. It claimed to cover "every flag run_sqd_sbd.py accepts" while actually mixing three drivers, and scoped them three different ways: prose "X only" in some cells, split defaults like "1 (enlarge) / 3 (sqd)" in others, nothing at all in the rest, so a reader had to infer which flags were universal. The newest driver appeared nowhere. Fine at two drivers, confusing at three. Restructured around an explicit split: a short-name table up front mapping each driver to how its subspace grows; "same flag, same default, in all three"; "same flag, different default in each" with one column per driver; and "how the subspace grows -- one knob per driver, NOT interchangeable", since --sqd_carryover_threshold, --enlarge_threshold and --sbd_carryover_threshold are easy to mistake for each other and only the latter two are equivalent (at carryover type 3). Added "Tuning the expansion threshold", which the tables previously left to guesswork: a measured H2O sweep showing 1e-4/1e-5/1e-6 giving 1.60/0.175/ 0.052 mHa against this system's FCI reference over 3.0M/25.1M/62.1M determinants -- two orders of magnitude on the threshold buying 30x less error at 20x the determinants, with sharply diminishing returns. Measured with the carryover driver on the bundled pool, i.e. the exact documented command. It also names the trap: --max_dim inverts the relationship, because once pinned at the cap the cap random-fills from a larger candidate pool, so a lower threshold can move the energy the wrong way. In the top-level README the two enlargement drivers are now presented as one pair rather than two unrelated bullets, in the Examples list and as a single "SQD with subspace enlargement" subsection. They share the same outer-loop structure and differ only in the expansion engine, so a table contrasts JAX (no MPI awareness, needs JAX_PLATFORMS=cpu past one rank) against SBD carryover (MPI-distributed, closes a round earlier), and says which to prefer and when the JAX one is still right -- it is solver-agnostic, SBD's carryover is not. Also reworded both drivers' --max_dim comments to describe the situation rather than name a specific internal dataset.
Sophia Wen (hfwen0502)
force-pushed
the
sbd-carryover
branch
from
September 18, 2026 20:40
605927f to
ee97c97
Compare
Two problems, both found by benchmarking on a 45-orbital system. H2O could not have caught either. 1. The driver unioned SBD's carryover with the solved subspace before capping, so that the no-growth check had something to compare. But cap_to_max_dim keeps everything already in the subspace first, so once the solved subspace is itself max_dim strings the union makes existing consume the entire budget, every new candidate is discarded, and the subspace freezes. --energy_tol then reads the frozen energy as convergence. On a 45-orbital system at --max_dim 15000 it "converged" after four rounds at -296.0744895995, 3.41 Ha above where the same expansion actually reaches. Invisible with --max_dim unset, which is all H2O exercised. Fixed by forwarding the raw carryover and deriving no-growth from a subset test (carryover subset of solved) instead of a union. Not preserving the solved subspace is correct, not a regression: pruning is what carryover is for, run_sqd_enlarge_subspace_sbd.py's expansion prunes the same way (its identity row preserves the thresholded pairs, not the whole subspace), and qiskit-addon-sqd's own carryover plus fresh sampling re-supply anything that still matters. That driver escapes the deadlock precisely because it does not union. After the fix the two drivers agree round for round on that system -- identical energy, subspace size and expansion size at all 8 rounds, converging to -299.4859985587 over 15000^2. H2O unchanged at -76.2421767512 over 1742^2. 2. The docstring claimed the MPI-distributed expansion was "2-4x faster" than the JAX path. Measured head to head, it is not: 2248 s versus 2250 s. Each round is dominated by configuration recovery and by diagonalizing a 225M-determinant subspace, so where the expansion runs is not measurable. The original figure came from comparing against a driver with no configuration recovery at all, so it measured recovery overhead and was attributed to the wrong thing. Reworded here and in both READMEs: the reason to prefer this driver is operational -- no JAX in the expansion path, so no GPU contention between ranks and no JAX_PLATFORMS=cpu workaround -- and explicitly not speed.
Comment on lines
+59
to
+63
| class SBDCarryoverResult(SCIResult): # type: ignore[misc,valid-type] | ||
| """SCIResult plus the carryover determinants SBD selected itself. | ||
|
|
||
| ``SCIResult`` is upstream's frozen dataclass, so a new field cannot be | ||
| added to it -- hence this subclass. It is safe to hand back to |
There was a problem hiding this comment.
This PR has potential overlap with #32. In Qiskit/qiskit-addon-sqd#369, there is a new carryover field on the SCIResult. It would be great to converge on our thinking/plans next week.
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
2 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
Adds a third SQD driver that grows its subspace using SBD's own carryover
(MPI-distributed C++) instead of qiskit-addon-sqd's JAX single excitations,
plus the
sbd_solver.pyplumbing to get those determinants back out of SBD.The gap this closes.
tpb_diaghas always returnedcarryover_adet/carryover_bdetin its results dict —bindings.cppunpacksSBD's
co_adet/co_bdetinto it — but_solve_sci_corediscarded them, and_create_sbd_configforce-setcarryover_type = 0because they werediscarded (its comment said so outright). Same shape as the RDM gap fixed in
#36: SBD computed the thing, the wrapper threw it away.
extract_carryover()converts them out of SBD's packed half-determinantformat into plain CI strings via
_sbd_dets_to_ci_strings, so they can feedinclude_configurationsdirectly.SCIResultis upstream's frozen dataclass, so a carryover field cannotbe added the way
rdm1/rdm2were (those already existed). HenceSBDCarryoverResult, a subclass.carryover_type = 0(the default) still returns a plainSCIResult, so thenormal SQD path is unaffected and pays nothing.
Why the subclass is safe. qiskit-addon-sqd's loop reads only
.energy/.sci_state/.orbital_occupancies, selects withmin(results, key=...), and passes that same object through —SCIResult(isconstructed exactly once in all of
fermion.py, inside upstream's own PySCFsolver. Verified on 4 ranks that the subclass survives both the loop and
upstream's rank-0-to-all-ranks broadcast with carryover intact.
_assert_carryover_survivedturns that assumption into a loud failure ratherthan a silent one, since a stripped result would make a driver quietly stop
expanding while still reporting convergence.
Docs.
examples/README.md's parameter tables had drifted: the sectionclaimed to cover "every flag
run_sqd_sbd.pyaccepts" while actually mixingthree drivers with three different scoping conventions, and the newest driver
appeared nowhere. Restructured into shared-flags / per-driver-defaults /
per-driver-expansion-knob tables with explicit short names, and added a
Tuning the expansion threshold section carrying a measured H2O sweep plus
the
--max_dimtrap (once pinned at the cap, the cap random-fills, so a lowerthreshold can move the energy the wrong way).
Verification (h100, 8× H100)
run_sqd_enlarge_subspace_sbd.pyIdentical to all ten digits over an identical final subspace — the real
correctness check, since SBD carryover type 3 and
enlarge_batch_from_transitionsshould compute the same expansion whenembedded in the same loop. They also agree bit-for-bit at threshold
1e-5(-76.2436018956, 5007²) and
1e-6(-76.2437251036, 7881²).JAX_PLATFORMSunset — the exact configurationwhere the JAX driver dies with
CUDA_ERROR_OUT_OF_MEMORY, since no JAX isinvolved in the expansion at all.
--sbd_carryover_type 0is rejected up front rather than silently loopingforever with nothing to expand.
run_sbd_diag.pystill runs withqiskit-addon-sqdandqiskitimport-blocked (verified the shim actually hid them, so the check wasn't
vacuous) — the new subclass sits behind the existing
try/except ImportErrorguard, and its only other references are inside function bodies.
The round-count difference has a specific cause worth noting: the JAX driver
compares its 1720-string expansion against the 1742-string solved subspace,
so its no-growth test can never fire and it always waits for energy
convergence. This driver unions the carryover with the solved subspace first,
so closure is detected directly.
On performance — measured, and the earlier claim was wrong. Benchmarked
head to head on a 45-orbital system (8 ranks,
--max_dim 15000, threshold1e-4), this driver and the JAX one agree round for round — identicalenergy, subspace size and expansion size at all 8 rounds, both converging to
-299.4859985587 over 15000² — in 2248 s vs 2250 s. So there is no speed
advantage: each round is dominated by configuration recovery and by
diagonalizing a 225M-determinant subspace, and where the expansion runs is
not measurable. An earlier "2-4x faster" note in the docstring was a
misattribution (it compared against a driver with no configuration recovery,
so it measured recovery overhead) and has been corrected here and in both
READMEs. The reason to prefer this driver is operational — no JAX in the
expansion path, so no inter-rank GPU contention and no
JAX_PLATFORMS=cpuworkaround — explicitly not speed.
That benchmark also caught a real bug, now fixed: the driver unioned the
carryover with the solved subspace before capping, which deadlocks once
--max_dimbinds (the cap keeps existing determinants first, so the unionconsumes the whole budget and the subspace freezes, with
--energy_tolreading the frozen energy as convergence). It landed 3.41 Ha high. No-growth
is now a subset test instead, needing no union. Not preserving the solved
subspace is correct — pruning is what carryover is for, and the JAX driver
prunes the same way, which is exactly why it never hit this. Unreachable with
--max_dimunset, which is all H2O exercised.Notes for review
SBD-native selected-CI driver being added and later removed, since the
carryover driver supersedes it for this package's SQD focus. Squash-merge
collapses that, but commit-by-commit review will look noisier than the diff.
bindings.cppgains the#ifdef-guarded extended-carryover fields. Thoseare deliberately not advertised —
--sbd_eri_thresholdisargparse.SUPPRESSed and--sbd_carryover_type's help stays vendor-neutralabout values past 3 — since they need a build this repo does not ship. They
remain reachable for anyone whose build has them.