Test the qiskit-addon-sqd entry points, serially and under MPI - #18
Draft
garrison wants to merge 1 commit into
Draft
Test the qiskit-addon-sqd entry points, serially and under MPI#18garrison wants to merge 1 commit into
garrison wants to merge 1 commit into
Conversation
Nothing covered the solver wrapper qiskit-addon-sqd is meant to be handed. test_reference_energies goes through tpb_diag_from_files, and the one place diagonalize_fermionic_hamiltonian ran was run_sqd_sbd.ipynb under nbmake -- which asserts nothing, so it only ever caught a raised exception, and only on a single rank. Three checks, each in a _standalone and an _mpi variant sharing one body: - solve_sci_batch over a fixed 40-determinant subspace, energy pinned. The determinants are given rather than sampled, so the answer is deterministic and the same on any process count -- which is the property worth pinning. - The same over the full 1em3 selection, asserting the energy published in the vendored data. Marked slow. - diagonalize_fermionic_hamiltonian itself, so a change to the sci_solver contract or to SCIResult/SCIState fails here rather than in a user script. Bracketed rather than pinned: the subspace comes from upstream's sampling. All leave fcidump_path unset, so rank 0 regenerates the FCIDUMP and broadcasts it -- the default path, and the multi-rank one 7c7cc09 fixed. Two variants rather than one test because pytest-mpi filters on the mpi marker in opposite directions (--only-mpi skips what is not marked, no flag skips what is), so a single function cannot run in both modes. The bodies size the determinant grid from MPI.COMM_WORLD, which is 1 in one process. bit_length is 63, not the wrapper's default of 20: SBD fixes the packed word count process-wide on the first diagonalization, so every test in a process must imply the same one, and 63 gives the single word test_reference_energies already gets from 64. It is also the largest legal value -- 64 shifts a 64-bit size_t by 64 in bitadvance(), which is undefined behavior on the very paths these tests exercise (#12). That leaves multi-word packing uncovered, which is what a default caller gets; noted in the module for a follow-up. Verified identical to ten digits on 1, 2 and 4 ranks. qiskit-addon-sqd and pyscf move into the test extra so tox -e py and tox -e mpi both cover this; the tests skip if the imports are missing. Assisted-by Claude Opus 5
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
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.
Nothing in the suite covered the solver wrapper that qiskit-addon-sqd is
meant to be handed.
test_reference_energiesgoes throughtpb_diag_from_files, and the only placediagonalize_fermionic_hamiltonianran was
run_sqd_sbd.ipynbundernbmake— which asserts nothing, so itcaught a raised exception and nothing else, on a single rank.
What this adds
test/test_sqd_integration.py, three checks each in a_standaloneand an_mpivariant sharing one body:test_small_subspace_*-85.29400074571684test_published_energy_*-76.23594663(slow)test_diagonalize_fermionic_hamiltonian_*The first two are deterministic: the determinants are given rather than
sampled, so the answer does not depend on the process count, which is the
property worth pinning. The third runs the real self-consistent loop, so a
change to the
sci_solvercontract or toSCIResult/SCIStatefails hereinstead of in a user's script; its energy is bracketed because the subspace
comes from upstream's sampling and recovery.
All three leave
fcidump_pathunset, so rank 0 regenerates the FCIDUMP intoa temp dir and broadcasts the path for every rank to open — the default, what
a caller coming through
diagonalize_fermionic_hamiltoniangets, and themulti-rank path 7c7cc09 fixed.
Verified identical to ten digits on 1, 2 and 4 ranks.
Why two variants instead of one test
pytest-mpi filters on the
mpimarker in opposite directions:--only-mpiskips what is not marked, and no flag skips what is. So one function cannot
run in both modes. The bodies size the alpha-determinant grid from
MPI.COMM_WORLD, which is 1 in a single process, so the same code servesboth —
tox -e pyruns the_standalonevariants andtox -e mpithe_mpiones, with no new tox env or CI plumbing needed.bit_length
Set to 63 rather than the wrapper's default of 20. SBD fixes the packed word
count in a process-wide inline static on the first diagonalization and throws
det_vector: elem_size mismatchfor any later one implying a differentcount, so every test sharing a process must agree; 63 gives h2o the single
word that
test_reference_energiesalready gets from 64.63 is also the largest legal value. As #12 established,
bitadvance()computes
(((size_t) 1) << bit_length) - 1, so 64 shifts a 64-bitsize_tby 64 — undefined behavior, reached from
mpi_redistribution()andmpi_sort_bitarray(), which is exactly what the_mpitests exercise.Two known gaps, both noted in the module:
SBD_DEFAULT_BIT_LENGTH(20) actually gets. Covering it needs a modulethat does not share a process with these.
test_reference_energiesstill pins 64. Same UB, but ontpb_diag_from_filesrather than the MPI paths, and out of scope here.Not a performance question either way: at 275 determinants the solve takes
1.9s at 63 against 2.1s at 20.
Other changes
conftest.py:device_configandcounts_pathfixtures. The solverwrappers take a
DeviceConfigrather than a backend module, so the"was this backend built?" skip is factored into a helper both fixtures
call instead of being duplicated.
pyproject.toml:qiskit-addon-sqdandpyscfmove into thetestextra so
tox -e pyandtox -e mpiboth cover this path. The testsskip themselves if the imports are unavailable.
This PR was generated by Claude Opus 5 under my guidance.