Clear the TS energy cache and checkfile when switching to the next TS guess - #1010
Open
calvinp0 wants to merge 1 commit into
Open
Clear the TS energy cache and checkfile when switching to the next TS guess#1010calvinp0 wants to merge 1 commit into
calvinp0 wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1010 +/- ##
==========================================
+ Coverage 64.60% 64.61% +0.01%
==========================================
Files 119 119
Lines 39785 39789 +4
Branches 10307 10307
==========================================
+ Hits 25703 25711 +8
+ Misses 11105 11104 -1
+ Partials 2977 2974 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
calvinp0
force-pushed
the
fix_switch_ts_stale_ts_energy
branch
from
August 23, 2026 08:56
6ac640f to
3b56545
Compare
… guess `switch_ts()` discards a TS guess and re-optimizes the next one, and it already invalidates every piece of state the new geometry makes meaningless: the chosen guess, the running jobs, the output paths and job_types (via `delete_all_species_jobs()`), the copied `freq.out`, the TS checks dict, and the rotors dict. It did not invalidate the energy data cached on the TS species itself, nor the checkfile pointing at the abandoned guess's wavefunction. All three energy fields are read straight off the species object, which is exactly where `delete_all_species_jobs()` cannot reach: * `e0` decides the E0 check. `compute_rxn_e0()` skips any species that already carries an `e0`, and `ARCReaction.copy_e0_values()` only fills an empty one, so the TS E0 was computed once, for the first guess, and every later guess was judged against it. `e0` is also written to the restart file, so the stale value survived a restart. Measured on a benchmark run, two guesses whose own E0 was 2.51 kJ/mol above the reactants were rejected on the first guess's 121.88 kJ/mol. * `e_elect` decides a second gate by the same mechanism, and this is the strongest reason to clear it. `check_rxn_e_elect()` — the check that runs when E0 cannot be determined — reads `reaction.ts_species.e_elect` directly off the species object, so a retained value silently answered that gate for the new guess too. * `freqs` is the primary source of the reported imaginary frequency. `arc/output.py::_get_ts_imag_freq()` takes the most negative entry of `spc.freqs` and only falls back to the chosen guess's `imaginary_freqs` when `spc.freqs` is empty, so without this clear the run reports the abandoned guess's imaginary frequency for the accepted TS. Caching is correct for the reactants and the products, whose geometries do not change here; it is wrong for the TS, whose geometry changes on every switch. Until the new sp and freq jobs replace them, `e_elect` and `freqs` are also saved to the restart file and reported in output.yml as if they belonged to the new guess. `checkfile` is the same defect class, reached through the route builder rather than through a check. `run_job()` reads the checkfile off the species and hands it to every job it spawns, and `GaussianAdapter` resolves the SCF initial guess for any polyatomic species as ` guess=read` when the checkfile exists and ` guess=mix` when it does not — the fallback is not TS-specific. Left uncleared, the new guess is seeded from the discarded geometry's converged orbitals instead of the symmetry-broken default that a fresh species gets. That branch is right when the checkfile is current; what is wrong is the stale pointer. Deliberately left alone: * `opt_level` — a level of theory, not a geometry-derived quantity. * `external_symmetry` and `optical_isomers` — also sticky, but only ever written onto the scheduler's species by the final Arkane rate/thermo run, after all switching is over. Excluding them is safe only because `compute_rxn_e0()` operates on `reaction.copy()`, which round-trips through `as_dict`/`from_dict` — a genuine deep copy — and merges only `e0` back onto the live reaction; were `copy()` ever to become shallow, this exclusion would become a bug of the same shape as the ones fixed here. * `t1` — a wavefunction diagnostic that no check reads and that output.yml does not carry. * `active` — considered, and deliberately not cleared. It has the identical sticky shape: set only when it is `None`, persisted to the restart file, and consumed by the Orca and Molpro CASSCF route builders. Whether an active space is guess-dependent is a chemistry question rather than a caching one, so it is left for a deliberate decision. The regression test asserts the invalidation of all four fields and, for `e0`, its consequence: that an E0 computed for the new guess is adopted by `copy_e0_values()` rather than masked by the old one.
calvinp0
force-pushed
the
fix_switch_ts_stale_ts_energy
branch
from
August 23, 2026 12:14
3b56545 to
7d706b3
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.
Scheduler.switch_ts()abandons a TS guess and optimises the next one. It already invalidated the state the new geometry makes meaningless — the chosen guess, running jobs, output paths andjob_types(viadelete_all_species_jobs()), the copiedfreq.out, the TS checks dict, the rotors dict. It did not invalidate the energy data cached on the TS species itself, nor the checkfile pointing at the abandoned guess's wavefunction.What that cost
Measured on a benchmark run of
O1[C]=CC=N1 + C#CC <=> c1ccno1 + C#C[CH2](isoxazol-5-yl + propyne → isoxazole + propargyl), wB97X-D/def2-TZVP, 1187 core-hours over 2.0 days — which produced no rate coefficient for a reaction ARC had actually solved:Guesses 18 and 30 clear the
> 1 kJ/molE0 gate on their own energies. They were rejected on guess 0's.Why the value persisted
compute_rxn_e0()skips any species that already carries ane0;ARCReaction.copy_e0_values()only fills an empty one (self.ts_species.e0 = self.ts_species.e0 or other_rxn.ts_species.e0); ande0is inas_dict/from_dict, so the stale value survived intorestart.yml. Three independent "don't overwrite what's cached" idioms, each correct alone, which together made one missed invalidation permanent.The four fields
e0decides the E0 check, as above.e_electdecides a second gate by the same mechanism —check_rxn_e_elect()readsreaction.ts_species.e_electdirectly off the species object, wheredelete_all_species_jobs()cannot reach. This is an independent bug the same fix closes.freqsis the primary source of the reported imaginary frequency:arc/output.py::_get_ts_imag_freq()takes the most negative entry ofspc.freqsand only falls back to the chosen guess'simaginary_freqs. Without this clear, the run above would have reported −798.8 cm⁻¹ (guess 0) as the imaginary frequency of the accepted guess 18 (−784.0).checkfilereaches the route builder rather than a check.run_job()hands it to every subsequent job, andGaussianAdapterresolves it toguess=read, seeding the new guess from the discarded geometry's converged orbitals instead of the default a fresh polyatomic species gets. That branch is right when the checkfile is current; the stale pointer is what is wrong.Caching is correct for reactants and products, whose geometries do not change here.
Deliberately not cleared
opt_level(a level of theory);external_symmetry/optical_isomers(written onto the scheduler's species only by the final Arkane run, after all switching — safe only becauseARCReaction.copy()round-trips throughas_dict/from_dict, a genuine deep copy, and merges back onlye0; werecopy()ever to become shallow this exclusion becomes a bug of the same shape);t1(no check reads it); andactive, which has the identical sticky shape and is left for a deliberate decision, since whether a CASSCF active space is guess-dependent is a chemistry question rather than a caching one.Review and verification
Independent adversarial review ("nothing blocks") and quantum-chemistry review ("SHIP"), run separately. Both flagged issues that were fixed before this was pushed: the docstring was cut from thirteen lines of rationale to a behavioural statement, and two physically impossible fixture constants were corrected —
e_electof −206942.5 kJ/mol (−78.8 Ha, an ethane-sized system) for an NH₃ species → −148340.0 (−56.5 Ha), andfreqsfrom three entries to six (3N−6 for a four-atom nonlinear species; three is not a possible mode count), exactly one negative.All four clears are mutation-verified independently: removing each makes the corresponding assertion fail with the stale value.
46 passedinarc/scheduler_test.py. One commit, two files.Known limitation, stated deliberately
Clearing the cache makes ARC accept guess 18, which is 2.79 kJ/mol higher in E0 than the true lowest saddle — underestimating k by roughly 3× at 300 K and 1.4× at 1000 K. That is a bounded, one-signed error and strictly better than the current outcome of no rate at all, but it is not the whole answer. Ranking surviving TSs by E0, and referencing a submerged barrier to the pre-reactive complex rather than to separated reactants, are tracked separately. This PR deliberately does not loosen the E0 gate, which is what protects Arkane from a below-reactant TS.
🤖 Generated with Claude Code