examples: fix the enlarge driver's unreachable closure test - #40
Merged
Merged
Conversation
enlarge_via_singles() builds its bitstrings from the threshold-selected strings only, so it returns a superset of the *dominant* input strings, not of the subspace: a solved string whose every pair fell below --enlarge_threshold is never fed in and never comes back. The docstring claimed the opposite, and the loop's closure test believed it and used equality -- which additionally requires every solved string to be dominant. On any subspace with a low-weight tail that never held, so no_growth never fired, "expansion added nothing new" was dead code, and every run paid an extra round to establish energy/occupancy convergence. Replaced with the subset test the condition always meant (the same one run_sqd_sbd_carryover.py uses), still computed on the raw expansion before cap_to_max_dim, and corrected the docstring. Also makes a nearby --max_dim comment dataset-agnostic. Verified on H2O over 8 GPU ranks, main and the fix back to back: identical energy (-76.2421767512) and final subspace (1742x1742), stopping on closure at round 2 instead of on energy convergence at round 3. The expansion there is 1720 strings against 1742 solved -- a strict subset, adding nothing, which is exactly what equality could never detect.
Sophia Wen (hfwen0502)
force-pushed
the
fix-enlarge-expansion-bugs
branch
from
September 21, 2026 18:01
f6ffa82 to
d41b825
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
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
enlarge_via_singles()builds its bitstring rows from the threshold-selectedstrings only:
Row 0 of
transitionsis the identity, so everything fed in comes back — buta solved string whose every pair fell below
--enlarge_thresholdis never fedin, and never comes back. So the result is a superset of the dominant
strings, not of the subspace. The docstring claimed the opposite ("Always a
superset of the input ci_strs_a/ci_strs_b"), and the loop's closure test
believed it and tested equality.
Equality additionally requires every solved string to be dominant, which is
false on any subspace with a low-weight tail. So
no_growthnever becametrue, the
"expansion added nothing new"stop reason was dead code, and everyrun paid an extra round to establish energy/occupancy convergence — negligible
on a small molecule, minutes on anything large.
Fix: the subset test the condition always meant, matching
run_sqd_sbd_carryover.py:Still computed on the raw expansion, before
cap_to_max_dim— cappingfirst would mask real growth and could stall a run while reporting
convergence. Docstring corrected to describe the pruning. A nearby
--max_dimcomment is also made dataset-agnostic.
Verification (h100, 8 GPU ranks, H2O)
mainand the fix back to back, same flags:mainenergy convergedexpansion added nothing newIdentical energy to ten digits over an identical final subspace — the fix
changes when closure is recognised, not what the loop converges to.
Round 2 is the bug in miniature: the expansion is 1720 strings against
1742 solved. A strict subset adding nothing — but
1720 != 1742, soequality declared growth. (Same two numbers #39's description explains as a
quirk of this test; this is the cause.)
Notes for review
run_sbd_selected_ci.pythat no longer exists; examples: SQD driver that grows its subspace via SBD's own carryover #39 already corrects those,so fixing them here would only conflict.
cap_to_max_dimis untouched. Separately, once the include list reachesmax_dimit fills upstream's whole budget on its own, since upstream buildsunique(concat(include_a, include_b, carryover, samples))[:max_dim]— adesign question, not part of this fix.