Separate SQD parameters from SBD parameters, and expose the ones that were unreachable - #28
Merged
Merged
Conversation
Sophia Wen (hfwen0502)
force-pushed
the
clarify-sqd-vs-sbd-parameters
branch
from
September 15, 2026 16:07
8368203 to
8ad18c1
Compare
run_sqd_sbd.py exposed 13 flags that configure SBD and 3 that reach the SQD loop,
while the three most useful SQD knobs were unreachable. Several names read as one
layer and acted on the other:
--tolerance looked like SQD convergence; was SBD's Davidson stop
--carryover_threshold looked like the loop's carryover cutoff; was SBD's
--max_iterations vs --iteration two different iteration counts
--carryover_type/--ratio SBD internals, and inert on this path
Now grouped by layer, with the SBD ones prefixed and every old spelling kept as an
alias (our templates and validation gate pass --tolerance, --iteration, --block):
SQD loop samples_per_batch, num_batches, max_iterations,
energy_tol, occupancies_tol, sqd_carryover_threshold
SBD solver sbd_method, sbd_eps, sbd_max_it, sbd_max_nb, sbd_do_rdm,
sbd_do_shuffle, sbd_bit_length
SBD carryover (no effect on SQD results)
sbd_carryover_type, sbd_carryover_ratio, sbd_carryover_threshold
MPI grid adet/bdet/task_comm_size
energy_tol, occupancies_tol and the loop's carryover_threshold are now actually
passed to diagonalize_fermionic_hamiltonian; before this they were stuck at the
addon's defaults with no way to set them.
The run prints which layer each setting belongs to, and notes that
--sbd_carryover_* cannot change an SQD result -- SBD's carryover is its own
iterative mechanism, driven by re-running its CLI with --carryover_adetfile, and
sbd_solver.py does not read it. Verified earlier: types 0/1/2/3 give bit-identical
energies.
Deliberately NOT added: a warning that energy_tol should be looser than sbd_eps.
davidson.h:205 documents eps as the norm of the residual vector, not an energy;
energy error goes roughly as |R|^2/gap, so the two are not on the same scale and
the default energy_tol == eps is not the inconsistency it looks like. Both help
strings say so.
… evolves Remove --sbd_carryover_type / _ratio / _threshold from run_sqd_sbd.py rather than document them as inert. A flag that provably cannot change the result does not belong beside flags that can; passing one now gets "unrecognized arguments" instead of silence. They remain meaningful on run_sbd_diag.py, which calls tpb_diag() directly and receives carryover_adet/carryover_bdet. sbd_solver.py stops asking SBD to compute a carryover it discards: default carryover_type=0. _solve_sci_core has not read results["carryover_*"] since the #19 fix, so this removes real work -- for type 2 it built singles-extended determinant lists -- with no effect on results. A caller can still set it through sbd_config. Also document the per-iteration subspace, which is the thing people keep asking about. Each iteration is strs_a = include_a ++ carryover_strings_a ++ samples_a (dedupe, max_dim, sort) with exactly two channels feeding forward from the previous iteration: the average orbital occupancies, which re-repair the ORIGINAL bitstrings through configuration recovery (not cumulative -- it re-derives from raw each time), and the wavefunction amplitudes, which decide what carries over via --sqd_carryover_threshold. Notes the priority order, since include and carryover are truncated ahead of fresh samples when max_dim is set, and states that SBD's own carryover plays no part.
The previous table implied --occupancies_tol controlled a feedback channel. It does not: it is a stopping criterion and has no effect on the subspace. Split the three parameters by what they actually do. Shapes the subspace: --sqd_carryover_threshold is the |coefficient| cutoff for carrying determinants forward, and the only one of the three that changes a computed number -- lower it to carry more. Alongside it, --samples_per_batch / --num_batches, and include_configurations / max_dim as the static floor and the cap. Decides when to stop, affecting nothing else: --energy_tol on the iteration-to-iteration energy change, --occupancies_tol on the largest change in any single orbital occupancy -- an infinity norm, not an average. Both stopping criteria must hold in the SAME iteration; the test is an `and` (fermion.py:584). So a run that reaches --max_iterations may be converged in energy while the occupancies still move, and loosening one tolerance alone will not stop it. Also notes that neither tolerance is comparable to --sbd_eps, which is a residual norm inside one diagonalization.
…rences The banner printed internal dest names -- "eps", "carryover_threshold" -- which are exactly the ambiguous labels the regrouping exists to remove. It now prints the flag spellings a user types, layer included: --sbd_eps, --sqd_carryover_threshold. python/examples/README.md still cited the old names in four places: the SQD driver's key options, the parameter table's cross-reference to the inner iteration count, and the note about SBD's carryover, which no longer has flags on that driver. run_sbd_diag.py keeps the unprefixed spellings and now says why -- that driver IS SBD. The top-level README.md needs no change; it does not document these flags.
The guide and the standalone "How the SQD loop changes the subspace" section described the same parameters from two angles, which is the duplication that made these flags confusing to begin with. One place now: the mechanism first, then the parameters split by whether they shape the subspace or only decide when to stop. Corrections to the old intro while merging: - it said the loop converges in 3-5 iterations, and mentioned only the occupancy feedback. Both feedback channels are now named, and the stopping rule is stated properly: energy_tol and occupancies_tol must BOTH hold in the same iteration (fermion.py:584), so a run can sit at max_iterations because one orbital's occupancy is still moving. - --samples_per_batch now notes that symmetrize_spin merges the alpha and beta string sets, so the subspace is up to (2N)^2 rather than N^2. That is why 300 samples produced a 360,000-determinant subspace, not 90,000. - added the measured cost note: on 8xH100 for a 45-orbital/46-electron case, going from subspace 360,000 to 36,000,000 cost only 1.28x the wall time while lowering the energy ~3 Ha, because small runs are dominated by counts parsing and configuration recovery rather than diagonalization. A cheap-looking run usually means the subspace is too small to be using the hardware.
samples_per_batch 300 -> 3000, sbd_config gains max_nb, and fcidump_path is shown since solve_sci_batch accepts it (sbd_solver.py:81) and reusing one FCIDUMP across batches is the normal case. Adds why samples_per_batch is the parameter to get right: symmetrize_spin merges the alpha and beta string sets, so the subspace is up to (2N)^2 -- 3000 gives ~36M determinants -- and small subspaces are dominated by counts parsing and configuration recovery rather than the diagonalization, so a fast run usually means the hardware is idle. Links to the SQD Parameter Guide for how each parameter feeds the loop.
run_sqd_sbd.py: samples_per_batch 300 -> 3000, sbd_eps 1e-8 -> 1e-5, sbd_max_it 100 -> 10. The old values were the outliers in the stack: upstream's TPB_SBD struct defaults to max_it=1 / eps=1e-4 and run_sbd_diag.py uses eps=1e-3, so 1e-8 / 100 asked for far more inner work than anything else. On the h2o counts case eps=1e-5 and eps=1e-8 give the same energy to ten decimal places, which is expected: eps is a residual norm and the energy error goes as |R|^2/gap, so 1e-5 already implies accuracy well beyond what energy_tol=1e-8 tests. Documents two things that were not written down anywhere: - --sbd_max_it is a CAP, not a criterion. davidson.h:422 just falls out of its loop when the count is exhausted, so a run that never reaches eps returns a partially converged vector with no warning. Now that the cap is 10 rather than 100 that matters more, so the help text and the guide say to watch the printed tol= values and cross-batch agreement. - the inner-solver defaults, in their own table in the parameter guide, next to a note on how they compare with upstream's. Top README snippet updated to eps=1e-5 to match.
… defaults The notebook's callback labelled its output "carryover dim" and explained it as "carryover dets selected for the next iteration, not the 75,625 diagonalized". That described the #19 bug: amplitudes came back over the carryover subset, so the printed number really was the carryover size. Now that amplitudes cover the diagonalized subspace, the label is simply wrong -- and the very next markdown cell already said the callback printed "subspace dim", so the notebook contradicted itself. Relabelled, with a note on why older output shows a much smaller number. sbd_config: eps 1e-8 -> 1e-5 to match the new default, and the carryover_type / ratio / threshold keys removed -- they are not read on the SQD path, and the comment on carryover_type was wrong anyway (type 1 is ratio-based; type 2 is the one that extends by singles). samples_per_batch stays 300 because the notebook runs serially, now with a comment saying so and pointing at the cost note rather than leaving it as an implied recommendation.
Addresses #22. h_comm_size is gone from both TPB_SBD and GDB_SBD. Upstream declares the field but never reads it: diag() shadows it with a local `mpi_size / (task_comm_size * base_comm_size)` and passes THAT to DetBasisCommunicator. So the attribute could only ever report 1 while ignoring whatever was assigned -- worse than absent. Assignment now raises AttributeError instead of being silently discarded. The gdb_diag guard message no longer advises setting it either; the helper dimension changes with the rank count. Rank-count divisibility now warns from the library, in tpb_diag, tpb_diag_from_files and gdb_diag, so every caller sees it rather than only run_sqd_sbd.py. It warns and does not raise, because SBD itself accepts the situation: DetBasisCommunicator is pure modular arithmetic with no validation, and the helper dimension comes from integer division, so a non-multiple rank count silently leaves the remainder idle. Refusing to run would override a choice that is legal, just wasteful; the point is only that it stops being invisible. The warning is wrapped so a diagnostic can never break the call. Verified on macOS: h_comm_size absent from both objects and assignment raising; the warning firing for 2 ranks on a grid of 3 and staying silent for grids of 1 and 2; pytest and the 2-rank MPI test green; h2o unchanged at -76.2359465468.
…config keys
From a 45-orbital/46-electron run at samples_per_batch=100000: iteration 1 finished
(subspace 799,079,824, Davidson 1.0 s) but helper construction took 9.3 s, and
iteration 2 spent 507 s in helper construction and then died with
cudaErrorMemoryAllocation. None of the three knobs that address this were reachable
from the SQD driver.
--max_dim cap on strings per spin sector, so the
subspace cannot exceed max_dim^2
--sbd_use_precalculated_dets Thrust: 0 switches from a whole-subspace
determinant index to per-thread storage
--sbd_max_memory_gb_for_determinants Thrust: cap that per-thread buffer
The ordering in mult_thrust.h matters and is easy to get backwards: the memory cap
sits in the ELSE branch at :257-273, so it applies ONLY when
use_precalculated_dets is 0. With the default 1, :217 allocates
D_size x adets x bdets -- the entire subspace -- and the cap is never consulted.
Both help strings say so.
On the setup cost: sbdiag.h:271-278 shows "helper construction" is MakeHelpers +
RemakeHelpers, host-side and superlinear in determinants per spin, already
distributed over the rank grid. No GPU setting affects it; bounding the subspace
does, which is what --max_dim is for. The growth between iterations comes from
carryover accumulating, so --sqd_carryover_threshold is the other lever.
Separately, sbd_solver applied sbd_config with `if hasattr(...)` and silently
dropped anything else -- so a typo, or a Thrust-only key on a CPU build, did nothing
without saying so. It now warns and names the keys.
Verified locally: parser accepts all three; the dropped-key warning fires for
Thrust-only keys on this CPU build and for a typo, and stays silent for valid keys;
pytest and the 2-rank MPI test green.
Silently dropping a key leaves the caller believing they configured something they did not, so an unknown key now raises ValueError and lists the valid keys. But a blanket raise would break a config meant to run on several backends: use_precalculated_dets, max_memory_gb_for_determinants and thrust_collapse_loops are guarded by #ifdef SBD_THRUST in bindings.cpp and are legitimately absent on CPU and OMP-offload builds, so those warn instead. Verified: valid keys silent, Thrust-only keys warn on a CPU build, a typo raises, and a typo mixed with Thrust-only keys still raises.
…ns away Adds --max_dim to the subspace-shaping table (it is exposed now), and the two Thrust-only knobs to the inner-solver table, including that the memory cap is only consulted when use_precalculated_dets is 0. Adds a short paragraph on the failure mode, since a run can start comfortably and fail several iterations later as carryover accumulates: exploding helper-construction time is host-side and no GPU setting touches it, while cudaErrorMemoryAllocation on Thrust is the whole-subspace determinant index. Gives the order to reach for --max_dim, then --sqd_carryover_threshold, then --sbd_use_precalculated_dets 0, and notes the measured 1.0 s Davidson against 9.3 s helper construction so people tune the subspace rather than the solver.
Drop _warn_rank_grid from the wrapper. Upstream already checks: TaskCommunicator (chemistry/tpb/helper.h:361) computes task x adet x bdet x h_comm_size and throws invalid_argument unless it equals the rank count. Measured on 8 ranks with a grid of 3: RC=1, "ValueError: MPI Size of twister is not a square of a integer". So our warning duplicated an existing check -- and printed once per rank. It was also wrong about the consequence. Both it and the PR #21 warning in run_sbd_diag.py, and the MPI Decomposition section, said a non-multiple rank count leaves ranks idle rather than failing. It aborts. All three corrected, and the README now quotes upstream's actual message, which names neither the grid nor the rank count and is therefore hard to act on without knowing the arithmetic. I had claimed upstream does no validation. That came from reading DetBasisCommunicator in gdb/helper.h and generalising; the TPB path uses TaskCommunicator in tpb/helper.h, which does. Verified on 8xH100 that dropping the h_comm_size attribute changed nothing about the derivation: grid 4x2 (helper=1) and 2x2 (helper=2) both give -76.2359465468, and the attribute is absent on cpu, gpu and gpu-omp.
… of 64 bitadvance() (framework/bit_manipulation.h) computes ((size_t)1 << bit_length) - 1; at bit_length=64 that shift is undefined behavior for a 64-bit size_t. In practice x86-64 shift instructions mask the count mod 64, so 1<<64 commonly evaluates as 1<<0=1, zeroing the mask bitadvance relies on -- silently corrupting determinant enumeration rather than crashing, which is worse than a clean error. Nothing upstream validates this, and nothing in our wrapper did either: argparse used plain type=int with no range check, and bit_length was a raw def_readwrite property. Replaced def_readwrite with a validating def_property on both TPB_SBD and GDB_SBD, so every caller -- direct API or either example script -- is protected once, centrally, rather than only where a --help string happens to document the danger. test_reference_energies.py used BIT_LENGTH = 64, which now raises. Checked reachability before assuming the test was wrong to use it: bitadvance() is only reachable via mpi_redistribution/mpi_sort_bitarray, and TPB's file-loading path (sort_bitarray, unconditional) never calls either, so 64 was never actually unsafe for THIS test -- it was safe by an implementation detail of what gets called, not in general. Changed to 63, which gives the identical word count (1) for every molecule the test uses (h2o 24, n2 18, nh3 29), so this is a pure constant swap with no behavior change. Also directly verified bit_length is NOT silently dropped by the wrapper before reaching this conclusion: two same-process tpb_diag_from_files calls for the same norb with different bit_length trigger 'det_vector: elem_size mismatch' -- only possible if the second call's value genuinely reached C++ and changed the internal word packing. If it had been ignored, both calls would have succeeded identically. Verified: 0/64/65/100 raise ValueError on both config classes; 1/20/32/63 accept; pytest and the 2-rank MPI test green; h2o still -76.2359465468 via run_sbd_diag.py.
…l check Reverts 0e7b004 in full. bindings.cpp and test/test_reference_energies.py are byte-identical to their pre-0e7b004 state. The risk (bitadvance()'s (1<<bit_length)-1 at bit_length=64 is UB) is real, but it is conditional on downstream calls (mpi_redistribution/mpi_sort_bitarray, reached via do_shuffle or GDB's redistribution flags) that the wrapper cannot know about at the point bit_length is set. Enforcing it there forced an unrelated, already-passing test to change for no functional reason. Per direction: minimize constraints in the wrapper layer: pass values through, and let the SBD C++ layer itself be the one to raise if something is actually wrong. Upstream does not validate this either, so this returns the wrapper's behavior to the same level of strictness upstream has always had. Verified: pytest and the 2-rank MPI test green with test/ fully unmodified; bit_length=64 now accepted silently (no raise, no warning) on TPB_SBD.
Reverts the warn-on-unknown-key mechanism in full, back to ed8bf9e's parent. python/sbd_solver.py's _create_sbd_config is byte-identical to before that commit. 5273fe4 (raise-on-typo, warn-on-Thrust-only-absence) was already reverted in e7d0454 for bit_length, but that only undid 5273fe4's refinement -- the warning mechanism itself originated one commit earlier, in ed8bf9e, which this reverts. That commit's OTHER changes (--max_dim, --sbd_use_precalculated_dets, --sbd_max_memory_gb_for_determinants, all in run_sqd_sbd.py) are untouched; the diff against ed8bf9e's parent for sbd_solver.py was a pure insertion of the warn block alone, so removing it changes nothing else. Same direction as e7d0454: minimize wrapper-layer constraints, pass values through, let the SBD C++ layer be the one to raise if something is genuinely wrong. Verified: an unknown key (typo or otherwise) now produces zero warnings and no exception; the three subspace-cost knobs still wire through correctly; pytest and the 2-rank MPI test green; h2o still -76.2359465468.
Isolated on an 8xGPU node against the 45-orbital/46-electron case that originally OOM'd (samples_per_batch=100000): --max_dim 20000 alone, with use_precalculated_dets left at its default of 1, gives the same result as combining it with use_precalculated_dets=0 and a memory cap -- helper construction 5.5 s in both iterations either way, same energy to all printed digits. The Thrust knobs added nothing measurable. So the "reach for them in this order" framing oversold the third one. max_dim is what fixes this, because it bounds every O(dim) structure; use_precalculated_dets bounds one GPU buffer and leaves the subspace, and so the host-side helper construction, exactly as large as it was.
… hooks Both use diagonalize_fermionic_hamiltonian's existing include_configurations / initial_occupancies parameters -- no changes to qiskit_addon_sqd or SBD. --include_hf forces the single Slater determinant with the lowest num_elec_a/num_elec_b orbital indices occupied into every iteration's subspace. Motivation: on the 45-orbital Boston case, that determinant's own diagonal energy (<D|H|D>, computed directly from hcore/eri) is -299.17, close to the -299.521 classical reference and BETTER than several iterations of unforced SQD -- which means the sampled bit_array was missing it. Forcing it in is a direct fix; growing max_dim/iterations is not, since neither helps if the good configuration was never in the pool to begin with. --checkpoint_file writes ci_strs_a/ci_strs_b/occupancies/energy to JSON after every iteration (atomic replace, so a killed run keeps its last complete iteration). --resume_from seeds a NEW run's include_configurations and initial_occupancies from a previous checkpoint's last entry. Not a bit-identical continuation -- RNG state is fresh -- but starts the new run's subspace and configuration recovery from where the old one stopped instead of from raw samples again, which matters for iterative overnight exploration where restarting from scratch each time wastes the compute already spent. Verified: py_compile clean. Not yet run end-to-end -- qiskit_addon_sqd isn't installed in any local env; smoke-testing on h100 next.
occupancies -> orbital_occupancies, the field's actual name. Caught by the local smoke test this needed: h2o, --include_hf (iteration 1 -76.2359466308, unaffected) + --checkpoint_file (2 iterations recorded, 275/275 strings, sensible occupancies) + --resume_from (reads the checkpoint, converges immediately at the same energy since it inherits the prior run's already-converged subspace).
…re the SQD loop
recover_configurations (qiskit_addon_sqd/configuration_recovery.py) loops in pure
Python over every row of the input pool, every SQD iteration:
for bitstring, freq in zip(bitstring_matrix, probabilities):
...
corrected_dict[bs_str] += freq
No vectorized or distributed implementation -- confirmed still true on GitHub main
as of 2026-09-15 (qiskit-addon-sqd 0.13.1, the version installed here). Checked for
existing upstream work first: no PR or issue matching "recover_configurations",
"slow", "performance", or "vectoriz" in Qiskit/qiskit-addon-sqd. Looks like a real,
unreported gap -- their typical pool sizes apparently never approached the
1M-bitstring hardware counts file used for the Boston case here.
py-spy dump on a live 8-rank run confirmed this directly: every rank but rank 0 was
sitting in an MPI broadcast (fermion.py:429, "no distributed implementation, so
only the control process performs it") while rank 0 ran this loop over the full
1,000,000-row pool -- SBD's own reported per-iteration time was ~7-15s, but total
wall time per iteration was ~300s. Cost scales with pool size, not
--samples_per_batch, so it swamps SBD/GPU-backend tuning entirely at this scale.
--max_pool_size N randomly subsamples (fixed seed) the counts pool down to N rows
before building the BitArray, when N is smaller than the pool. A no-op otherwise.
Lossless specifically when every row's count is equal (no real hardware-sampling
weight to preserve, as measured true for the Boston counts file this session) --
not a general claim that subsampling never loses information.
Verified on h2o: truncates to the requested count when smaller than the pool,
no-ops when larger, runs without error either way.
--checkpoint_file -> --checkpoint_path: same write behavior, but the help text now says explicitly what --resume_from's read side actually requires -- every rank opens this path itself (no rank-0-reads-then-broadcasts step), so it must be visible under the same path from every rank. A per-node-local /tmp on a multi-node job would silently leave other nodes' ranks unable to find it. --checkpoint_frequency N: write every N iterations instead of every one, plus always on the last iteration regardless of alignment (so a completed run's checkpoint reflects its true final state, not whatever iteration happened to land on a multiple of N). Matters once ci_strs_a/b get large -- one int per string, so --max_dim 100000 is up to 200,000 ints written every iteration at N=1 -- or once iterations are fast enough that the JSON write itself is a non-trivial fraction of the per-iteration cost. Also updated --resume_from's help text to state plainly that every string from the resumed iteration becomes a permanent include, not subject to carryover_threshold decay the way a true single-process continuation's own carryover would be -- a resumed run is seeded richer than a continuation would have been at that point, not identical to one. Verified: checkpoint_frequency=3 over 5 iterations checkpoints only at 3 and 5 (the forced final write), not 1/2/4; resume_from still reads a checkpoint written under the new --checkpoint_path name and reproduces the converged energy immediately.
…xample, grouped like the CLI itself, and add --include_hf/--max_pool_size/checkpointing The parameter guide lived buried as a #### subsection inside "2. run_sqd_sbd.py", organized by role (shapes-the-subspace / stopping / inner-solver). Promoted it to its own numbered item -- "4. SQD Parameters" -- alongside the other examples, and regrouped it to match run_sqd_sbd.py --help's own three argument groups (SQD loop, SBD solver, MPI grid), plus a fourth for checkpointing. Section 2 keeps a one-line pointer where the old subsection was. Also documented three flags that were completely absent from this README despite being on this branch already: --include_hf (added to the "shapes the subspace" table, with the variational-bound rationale in one sentence), --max_pool_size (same table, one sentence on the recover_configurations cost it addresses), and the new Checkpointing parameters group (--checkpoint_path, --checkpoint_frequency, --resume_from) -- what's actually preserved (CI strings + occupancies, not the wavefunction amplitudes matrix), the shared-filesystem requirement, and that a resumed run is seeded richer than a true continuation, not identical to one. --sbd_max_nb's row gets one added sentence on the mid-Davidson OOM mechanism (peak memory depends on how many sub-iterations Davidson actually needs, not just --max_dim), since that surprised us live on the Boston case and the existing text gave no hint it could happen. MPI grid parameters point to the existing "## MPI Decomposition" section rather than re-explaining the 4D grid, to avoid duplicating it.
Manually refine the content
Reverts the flag, load_counts_as_bitarray's truncation branch, and the call-site wiring, back to a plain unconditional load. The README's table row for it was already removed in a manual edit; this completes the removal in the actual script. Verified: h2o still loads its full 275-row pool and runs correctly with the flag gone; pytest and the 2-rank MPI test green.
Our vendored SBD is a git submodule pinned to an exact commit, so citing its line numbers is safe -- we already re-verify them whenever we bump it. qiskit-addon-sqd is an external pip-installed dependency we don't pin at all; a line number cited against it can go stale the moment it's upgraded, with no way for us to notice. Replaced every fermion.py:NNN citation with a function or variable name instead (diagonalize_fermionic_hamiltonian, _prepare_ci_strings), which drifts far less and still tells a reader where to look. Also rewrote "the test is an `and` (fermion.py:584)" -- confusing as a noun phrase -- into a plain sentence: the convergence check combines the two criteria with a logical `and`.
…e nit Fix a real bug: the manual heading-level edit (### 4. SQD Parameters -> ## SQD Parameters) left the pointer at the top of the file still linking to #4-sqd-parameters, which no longer matched any heading and would have 404'd. Repointed it to #sqd-parameters, the anchor the new heading actually generates. #mpi-decomposition was never touched and already matched correctly. Removed "## Extra dependencies (only for the SQD examples)" entirely: the top README's "## Integration with qiskit-addon-sqd" already covers the same install command and version requirement, and its conda recipe already includes pyscf -- this was a duplicate. Repointed the one place that linked to it. Reverted "### **How each iteration builds its subspace.**" from a heading back to a bold lead-in within the paragraph -- it isn't a fifth parameter group alongside SQD loop / SBD solver / MPI grid / Checkpointing, it's the introduction to the first of them, and heading it at the same level implied otherwise. Left alone, per direction: the removed "non-multiple rank count aborts" explanation (defer to SBD's own error) and the removed CPU/gpu-omp libnvomp warning (real but users only pick one backend in practice). Also fixed the Overview's backend bullet, which said "GPU (OpenMP target offload)" with no vendor note, while Backend Selection further down already correctly said NVIDIA and AMD -- now consistent.
Sophia Wen (hfwen0502)
force-pushed
the
clarify-sqd-vs-sbd-parameters
branch
from
September 15, 2026 16:44
8ad18c1 to
0ee54af
Compare
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
1 participant
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.
The SQD driver mixed two unrelated parameter sets under one flat namespace: knobs
belonging to the
qiskit-addon-sqdloop, and knobs belonging to the SBDdiagonalization inside it. Several parameters that shape SQD convergence were not
reachable at all, and several that were reachable have no effect on an SQD result.
Parameter separation
--helpnow has three groups -- SQD loop, SBD solver (--sbd_*), MPI grid -- andthe run banner prints the flag spellings it is reporting, so the output says which
switch produced which value. Legacy spellings are kept as aliases.
Newly reachable
--energy_tol,--occupancies_toland: both must hold in the same iteration (fermion.py:584). Neither is comparable to SBD'seps, which is a residual norm--sqd_carryover_threshold--max_dimmax_dim^2--sbd_use_precalculated_dets,--sbd_max_memory_gb_for_determinantsRemoved
h_comm_sizeis gone from both config classes. Upstream declares the field butdiag()shadows it with a locally derived value, so the attribute could only everreport
1and ignore what you assigned -- worse than being absent (#22). The rankgrid is validated by SBD itself, which throws
invalid_argumenton a badcombination; the wrapper no longer duplicates or paraphrases that check.
SBD's
carryover_type/ratio/thresholdare documented as having no effect onan SQD result -- the SQD path never reads SBD's carryover output -- and
carryover_typeis now set to0rather than1so the work is not done at all.Runaway subspace growth
A 45-orbital/46-electron run at
samples_per_batch=100000reached iteration 2 with507 s in helper construction and then died with
cudaErrorMemoryAllocation. None ofthe knobs that address it were reachable from the driver.
Isolated on an 8-GPU node:
--max_dim 20000alone fixes it -- helperconstruction 507 s to 5.5 s, run completes. Adding
use_precalculated_dets=0and amemory cap on top changes nothing measurable, because those bound one GPU buffer
while
max_dimbounds every O(dim) structure including the host-side helperconstruction. The README says this rather than presenting three equal options.
Verification
pytestand the 2-rank MPI test green throughout. h2o unchanged at-76.2359465468(1e-3) and-76.2429584035(1e-4) across cpu, gpu and gpu-omp. TheBoston case completes at
-294.2521495427.Two changes made during development were reverted before this PR, deliberately: a
bit_lengthrange check and an unknown-sbd_config-key warning. Both put policy inthe wrapper that belongs to the SBD layer; values are passed through unchanged.