Duplicate-determinant handling differs between TPB and GDB; cost of the #29 label conversion at scale

[garrison]

Two points from reviewing #29 that did not block it, but are worth recording. Both still hold on main at 6721877.

1. TPB and GDB now answer the duplicate-determinant question in opposite ways

sort_bitarray (framework/bit_manipulation.h:386) both sorts and deduplicates:

std::sort(a.begin(), a.end(), [](...){ return less_from_back(x,y); });
a.erase(std::unique(a.begin(), a.end()), a.end());

The GDB binding treats a shrink as a caller error, with a stated rationale (python/bindings.cpp:477-485):

// sort_bitarray also removes duplicates, which would leave
// part of the subspace unreachable, so reject them instead of
// dropping them silently.
...
throw std::invalid_argument("gdb_diag requires distinct determinants");

After #29 the TPB path takes the other branch: duplicates are dropped, the labels are derived from adet/bdet so they stay consistent with the amplitudes, and the caller is returned a smaller subspace than it asked for without notice.

#29's choice looks right to me — sort_bitarray's documented API permits duplicates, so a caller following it should not hit an exception, and deriving labels from the diagonalized lists is the property worth having. But the repo should not hold both answers silently. Either:

• TPB is correct and the GDB check should be reconsidered, or
• the difference is deliberate (GDB's single-list subspace really does become unreachable in a way TPB's tensor product does not) and a comment on the TPB path should say so.

Worth noting the GDB comment's concern is not mislabeling — it is the caller receiving a smaller subspace than requested. Post-#29 that outcome is silent on the TPB path. Probably acceptable, but it is a behavior change beyond "the size check no longer misfires on benign dedup," and it is not reachable from run_sqd_sbd.py (fermion.py dedupes with np.unique first), so only direct solve_sci / solve_sci_batch callers see it.

2. _sbd_dets_to_ci_strings is O(n) Python on the control process, called twice per diagonalization

#29 added:

solved_strings_a = _sbd_dets_to_ci_strings(adet, norb, backend, sbd_data.bit_length)
solved_strings_b = _sbd_dets_to_ci_strings(bdet, norb, backend, sbd_data.bit_length)

The helper is a per-determinant Python loop doing backend.makestring then int(binary_str, 2). At the bundled H2O case (275 strings) this is irrelevant. At the larger vendored inputs it is not: data/h2o/h2o-1em8-alpha.txt is 39,028 alpha strings, data/c4h4 14,690, data/nh3 118,755. That is tens of thousands of string-format-and-parse round trips per call, per spin sector, on rank 0 while every other rank waits.

Not a blocker and not worth trading away #29's correctness property — comparing len(adet) against len(strings_a) and converting only on mismatch would reintroduce exactly the "two lists expected to agree" coupling #29 removed. Recording it because it scales with the inputs we intend to run, and a vectorised conversion (or one pushed into the binding) would be a contained change if it ever shows up in a profile.

A note on #29's ordering argument

#29 describes the agreement between sort_bitarray's canonical order and ascending-integer order as a coincidence established by measurement. It is stronger than that: less_from_back (bit_manipulation.h:154) compares words from the highest index downward, i.e. most-significant word first, and from_string packs a big-endian bitstring so the most significant bits land in the last word. For fixed-width determinants that is ascending integer order, structurally.

This does not weaken the case for #29 — nothing contracts the packing either, and deriving labels from adet/bdet is the right fix regardless. But the measurement was confirming a consequence of the packing rather than a happy accident, which is worth knowing if anyone revisits this.

---

This issue was generated by Claude Opus 5 under my guidance.