examples: remove dead CLI params, fix miswired ones, expose symmetrize_spin - #37
Merged
Merged
Conversation
Sophia Wen (hfwen0502)
force-pushed
the
sbd-fix-driver-parameters
branch
from
September 17, 2026 23:05
e2e2f28 to
41affcf
Compare
2 tasks
…e_spin Audited every CLI parameter in run_sqd_sbd.py, run_sqd_enlarge_subspace_sbd.py, and run_sbd_diag.py for ones that are exposed but don't actually do anything. run_sqd_sbd.py / run_sqd_enlarge_subspace_sbd.py: removed --sbd_do_rdm (SBD computes real RDMs when set, but neither driver ever reads result.rdm1/result.rdm2 back off the SCIResult -- real wasted compute for zero visible effect) and --sbd_do_shuffle (read into a local C++ variable in the det_vector-based diag() overload these drivers call, then never referenced again anywhere in that function -- the shuffle logic only exists in the other, file-based diag() overload, which this code path never reaches). Both were genuinely inert, not just discarded downstream. Both drivers also hardcoded symmetrize_spin=True in their diagonalize_fermionic_hamiltonian call, forcing ci_strs_a == ci_strs_b every iteration even though SBD itself supports distinct alpha/beta determinant sets. Added --symmetrize_spin (default 1, matching prior behavior) as a real toggle on both. run_sbd_diag.py had a different, more basic bug: --max_time, --init, --shuffle/--do_shuffle, --use_precalculated_dets, and --max_memory_gb_for_determinants were all parsed into args but never copied onto the TPB_SBD config object at all -- pure no-ops regardless of what SBD's C++ layer would otherwise do with them. Added the missing config.X = args.X lines (guarding the two Thrust-only fields with hasattr, matching sbd_solver.py's own _create_sbd_config pattern, since they don't exist on the CPU/OMP-offload backends' TPB_SBD). --bdetfile was a step further: tpb_diag_from_files's binding doesn't even accept a second determinant file, and always derives beta from the one adetfile it's given. Added --symmetrize_spin here too (default 1, same meaning): when 0, load --adetfile and --bdetfile independently via LoadAlphaDets + sort_bitarray and call the data-structure entry point (sbd.tpb_diag) instead of tpb_diag_from_files -- genuinely distinct alpha/beta subspaces now work. When 1 (default) and --bdetfile is also given, warn and ignore it rather than silently doing nothing with it. Also wired up real RDM retrieval in run_sbd_diag.py: reshapes SBD's raw one_p_rdm/two_p_rdm (same verified formula as sbd_solver._assemble_rdms, duplicated rather than imported to keep this file qiskit-free), prints trace(rdm1) and natural orbital occupations, and adds --rdm_output to save rdm1/rdm2 to a .npz file. Verified on h100: H2O default runs on both SQD drivers reproduce the known reference energies unchanged; --symmetrize_spin 0 produces materially different (and correctly asymmetric-shaped) subspaces on both; run_sbd_diag.py's --rdm 1 output has trace(rdm1)=10 for H2O with sensible natural orbital occupations; --symmetrize_spin 0 --bdetfile with a genuinely different file gives a different, physically sensible energy, while --symmetrize_spin 1 (default) with --bdetfile set warns and reproduces the plain single-file result; --max_time now visibly truncates convergence; --shuffle/--init no longer crash and behave as expected.
run_sbd_diag.py's RDM retrieval initially duplicated the reshape formula rather than importing it from sbd_solver.py, on the reasoning that this file should stay free of any qiskit-addon-sqd dependency. That reasoning doesn't hold up: sbd_solver.py's mpi4py import is already a real, unavoidable transitive dependency of importing sbd itself (sbd.init() uses it internally), and its pyscf/qiskit-addon-sqd imports are both soft (try/except), so importing sbd_solver.assemble_rdms adds no new hard dependency here. Renamed _assemble_rdms to assemble_rdms (it's no longer solver-internal -- two independent entry points now use it) and had run_sbd_diag.py import it instead of carrying its own copy. One formula, one place to fix or extend; run_sqd_sbd.py/run_sqd_enlarge_subspace_sbd.py already get correct rdm1/rdm2 on SCIResult via this same function without any further work if they ever want to surface RDMs again. Verified on h100: run_sbd_diag.py's --rdm 1 output (trace, natural orbital occupations, saved .npz) is bit-identical to before the de-duplication; run_sqd_sbd.py's H2O energy is unaffected. No automated regression test for this specific reshape formula -- it was verified by hand against PySCF's make_rdm1/make_rdm2 on all three SBD backends, both element-wise and via the energy identity, during development; a future regression would also surface immediately via run_sbd_diag.py's --rdm output.
run_sqd_sbd.py and run_sqd_enlarge_subspace_sbd.py each had their own `from pyscf import ao2mo, tools` just to turn an FCIDUMP file into dense hcore/eri arrays (tools.fcidump.to_scf(...).get_hcore() + ao2mo.restore(1, mf._eri, norb)). Added sbd_solver.load_integrals_from_fcidump, which builds the same dense chemist-notation arrays directly from SBD's own LoadFCIDump binding's raw (value, i, j, k, l) integrals list -- verified bit-identical (max diff 0.0) to the pyscf construction on H2O -- and reuses the already-existing _read_fcidump_ecore for the nuclear repulsion piece. Both drivers now use this instead. This does not make either driver pyscf-free overall: qiskit-addon-sqd itself declares pyscf as a hard, non-optional dependency and qiskit_addon_sqd/fermion.py imports it unconditionally at module level (`from pyscf import fci`), and both drivers fundamentally need diagonalize_fermionic_hamiltonian from that module. Verified directly: with pyscf blocked, both still fail, but now at `from qiskit_addon_sqd.fermion import ...` inside qiskit-addon-sqd's own file, not anywhere in this driver's code -- confirming the drivers' own pyscf usage is genuinely gone, even though the transitive dependency through qiskit-addon-sqd remains. run_sbd_diag.py (no qiskit-addon-sqd dependency at all) was already pyscf-free before this commit and stays that way -- verified it runs correctly with pyscf blocked entirely.
rdm1 and rdm2 are always computed and reported together -- there's no
partial case where you'd want one without the other -- so having a
separate --rdm {0,1} to turn computation on and --rdm_output PATH to
additionally save it was redundant. Removed --rdm; --rdm_output PATH is
now the sole control (empty, the default, skips RDM computation entirely),
matching this file's own existing convention for --dump_matrix_form_wf/
--loadname/--savename, where a path is what turns each feature on.
Verified on h100: --rdm_output still prints the same trace/natural-orbital
output and saves the same .npz as before; omitting it still skips RDM
computation cleanly (same as before).
Usage examples used a bare relative filename (rdms.npz), which doesn't illustrate that --rdm_output takes a path (directories included), even though the help text and README prose already call it that. Updated both usage examples to /tmp/h2o_rdms.npz, and added metavar='PATH' so --help itself reads --rdm_output PATH consistently with the docs.
…h arrays Explicitly contrast with upstream SBD's own CLI (two separate files, 1pRDM.txt/2pRDM.txt) in the help text, module docstring, and README -- this is exactly what caused confusion when discussing the feature. Also trims the README paragraph down from 9 lines to 5.
…vers" This reverts commit 41affcf03d33887a52dea41f0fd68a25a3ca4dc. qiskit-addon-sqd itself declares pyscf as a hard, non-optional dependency and imports it unconditionally in qiskit_addon_sqd/fermion.py, which run_sqd_sbd.py and run_sqd_enlarge_subspace_sbd.py both need regardless (that's their entire purpose). pyscf is therefore always present whenever either driver can run at all, so load_integrals_from_fcidump was duplicating pyscf's own FCIDUMP parsing for no actual dependency-reduction benefit -- pure added maintenance surface. Reverting back to pyscf.tools.fcidump.to_scf(...) + ao2mo.restore(1, ...) directly, as before. run_sbd_diag.py is unaffected: it never called load_integrals_from_fcidump (it doesn't build dense hcore/eri arrays at all -- SBD's own tpb_diag_from_files/tpb_diag load the FCIDUMP directly), only sbd_solver.assemble_rdms, which stays.
Sophia Wen (hfwen0502)
force-pushed
the
sbd-fix-driver-parameters
branch
from
September 17, 2026 23:58
397921d to
092916d
Compare
…o the concrete example The comment above 'from sbd.sbd_solver import assemble_rdms' justified why importing it doesn't add a new dependency -- reasoning that belongs in a commit message, not cluttering the import site. Also: README still described --rdm_output using the abstract 'PATH' placeholder in prose right after showing a concrete example path -- ties the description to that same example instead.
PATH reads as ambiguous between a file and a directory; this flag specifically takes a file location, so FILE is less confusing.
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.
Builds on #36 (merged) — uses its
assemble_rdms.Summary
Audited every CLI parameter in
run_sqd_sbd.py,run_sqd_enlarge_subspace_sbd.py, andrun_sbd_diag.pyfor ones that areexposed but don't actually do anything.
run_sqd_sbd.py/run_sqd_enlarge_subspace_sbd.py: removed--sbd_do_rdm(SBD computes real RDMs when set, but neither driver everread
result.rdm1/result.rdm2back off theSCIResult— real wastedcompute for zero visible effect) and
--sbd_do_shuffle(read into alocal C++ variable in the det_vector-based
diag()overload thesedrivers call, then never referenced again anywhere in that function —
the shuffle logic only exists in the other, file-based
diag()overload, which this code path never reaches). Both were genuinely
inert.
symmetrize_spin=True, forcingci_strs_a == ci_strs_bevery iteration even though SBD itself supportsdistinct alpha/beta determinant sets. Added
--symmetrize_spin(default1, matching prior behavior) as a real toggle on both.
run_sbd_diag.pyhad a more basic bug:--max_time,--init,--shuffle/--do_shuffle,--use_precalculated_dets, and--max_memory_gb_for_determinantswere parsed intoargsbut nevercopied onto the
TPB_SBDconfig object at all — pure no-ops. Added themissing
config.X = args.Xlines (guarding the two Thrust-only fieldswith
hasattr, since they don't exist on the CPU/OMP-offload backends'TPB_SBD).--bdetfilewas a step further:tpb_diag_from_files's binding doesn'teven accept a second determinant file, and always derives beta from the
one
adetfileit's given. Added--symmetrize_spinhere too:0loads--adetfileand--bdetfileindependently viaLoadAlphaDets+sort_bitarrayand callssbd.tpb_diaginstead oftpb_diag_from_files— genuinely distinct alpha/beta subspaces nowwork.
1(default) with--bdetfileset warns and ignores it ratherthan silently doing nothing.
run_sbd_diag.py, sharingsbd_solver.assemble_rdms(from sbd_solver: populate SCIResult.rdm1/rdm2 from SBD's computed RDMs #36) rather than duplicating it:--rdm_output filenameprintstrace(rdm1)and natural orbitaloccupations and saves
rdm1/rdm2to a.npzfile at that path --the sole control (no separate on/off flag, since rdm1/rdm2 are always
computed and reported together), matching this file's existing
--dump_matrix_form_wf/--loadname/--savenameconvention.Verification (on h100)
--helpon both SQD drivers: no more--sbd_do_rdm/--sbd_do_shuffle;both list
--symmetrize_spin.unchanged (
-76.2359466308/-76.2421767512) — removals changednothing about default behavior.
--symmetrize_spin 0produces materially different, genuinelyasymmetric subspaces on both drivers (confirmed via distinct
alpha/beta counts and different energies vs the default).
run_sbd_diag.py --rdm_output ...:trace(rdm1)= 10 for H2O,sensible natural orbital occupations,
.npzround-trips with correctshapes; omitting it skips RDM computation cleanly.
run_sbd_diag.py --symmetrize_spin 0 --bdetfile <different file>:routes through
sbd.tpb_diag, gives a different, physically sensibleenergy;
--symmetrize_spin 1(default) with--bdetfileset warns andreproduces the plain single-file energy exactly.
--max_time/--shuffle/--initnow visibly reach the C++ layer(confirmed truncated convergence, order-independence, and a different
but consistent convergence path respectively).