Fix multi-review findings: Gibbs ranking, worker-death hang, fp64 energies, FIRE decoupling - #180
Open
isayev wants to merge 2 commits into
Open
Fix multi-review findings: Gibbs ranking, worker-death hang, fp64 energies, FIRE decoupling#180isayev wants to merge 2 commits into
isayev wants to merge 2 commits into
Conversation
…ines, entry Critical: Gibbs-basis ranking (k>1) ordered and truncated on electronic energy, G_rel was referenced to the lowest-E conformer and overwrote correct upstream values, and the orchestrator hung forever when the isomer worker died by signal before emitting its queue sentinels. Major: energy-window selection was unsound on the Gibbs basis; fp64 model energies were rounded to float32 on the ranking path; ANI2xt's torchani requirement was never preflighted; optimizing.run()'s silent early return let stale outputs pass as fresh results; the @tautN suffix was parsed four ways (one truncating legitimate IDs); radicals passed the ANI compatibility gate silently; OpenEye tautomer selection cross-ranked different protonation states on raw E_tot; output finalize held the whole combined SDF in memory; auto3d --help imported torch and rdkit; Thermo_failed had two owners; optimizer workers bypassed get_device; default-output naming was triplicated. Minor: OptimizationError now exits 7; zero-molecule inputs exit 2 with a clear message; log-queue shutdown can no longer hang; dead symbols deleted; stale comments, docstrings and 20 pre-layer-move path headers corrected; benchmarks/scripts/examples import live paths; aimnet2-nse spin claims corrected; ProgressEvent exported. FIRE decoupling lands separately in the next commit (changes trajectories). Review-manifest: .claude/review-manifests/review-2026-08-28-multi.json Claude-Session: https://claude.ai/code/session_01DSLaykYiZKLckZb1iFqHzB
…rule The batched FIRE coupled every molecule's timestep adaptation to the whole bucket: in the all-progressing regime dt never grew (a solo molecule froze at dt=0.07 from step 1, dt_max=0.1), and one stalling batchmate permanently froze a progressing molecule's acceleration counter. Beyond wasted model evaluations, a molecule's optimization trajectory depended on which molecules shared its bucket, so adding an unrelated molecule to an input could shift another molecule's final conformer energy. Now nsteps_inc = progressing and speedup = progressing & past_nmin, per Bitzek et al. 2006, and all_progressing is gone. Converged geometries remain valid minima to opttol; trajectories change, which is why this is its own commit. The golden-trajectory checksum pinned the coupled behavior and is regenerated; two new tests pin solo-molecule dt growth and bucket independence. Claude-Session: https://claude.ai/code/session_01DSLaykYiZKLckZb1iFqHzB
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.
Summary
A five-perspective review of the whole codebase (pipeline correctness, computational chemistry, PyTorch/GPU, architecture, refactoring) surfaced 40 findings; this PR fixes 35 of them. Every Critical/Major fix carries a RED-verified regression test or an empirical repro confirmation.
Critical
k>1: selection and ordering used electronic energy;G_rel(kcal/mol)was referenced to the lowest-E conformer and overwrote correct upstream values with negative ones; the window cutoff could drop in-window conformers. One root cause (keptsorted on E_tot, treated as basis-sorted); the basis is now honored end to end. The pre-existing tests could not catch this — they used degenerate E vs G orderings; the new ones disagree deliberately.finallysentinels and the run hung forever. The parent now injects sentinels on abnormal exit and bounds the optimizer join (terminate backstop only on signal-death exit codes).Major (selection)
E_tot.opt_geometry/smiles2molscan no longer return a stale previous output when the input yields nothing;optimizing.run()now reports whether it wrote.auto3d --helpback to ~0.1 s (was 2.4 s —policy.pyimported torch+rdkit at module level); an import-boundary test pins it.@tautNparsing unified under one helper (a bare-@ID no longer produces a false exit-6 failure);Thermo_failedback to one owner; workers useget_deviceagain; combined-output finalize streams instead of holding 2.7x the file in memory.Minor
OptimizationError→ exit 7 (docs table + tests); zero-molecule input → exit 2; log-queue shutdown can't hang; dead symbols removed; 20 stale pre-layer-move path headers dropped; benchmarks/scripts/example notebooks import live paths again;aimnet2-nseCLI text no longer claims spin control that isn't threaded;ProgressEventexported and documented.Deferred (recorded in the local review manifest)
Layer relocation of chemistry modules out of
foundation/; deprecate-or-bless decision on theentry/auto3Dworker re-exports; adapter sync-reduction (needs GPU measurement); threading multiplicity to NSE models; Send2Trash-vs-delete for verbose tarballs.Test plan
https://claude.ai/code/session_01DSLaykYiZKLckZb1iFqHzB