Skip to content

Do not discard a TS when the normal mode displacement check cannot be run - #975

Draft
calvinp0 wants to merge 1 commit into
fix_repeated_reactant_speciesfrom
fix_nmd_aa_reaction_atom_count
Draft

Do not discard a TS when the normal mode displacement check cannot be run#975
calvinp0 wants to merge 1 commit into
fix_repeated_reactant_speciesfrom
fix_nmd_aa_reaction_atom_count

Conversation

@calvinp0

@calvinp0 calvinp0 commented Aug 12, 2026

Copy link
Copy Markdown
Member

A validated transition state for any A + A reaction was discarded without ever being examined.

The TS had converged, had passed its imaginary-frequency check, and was then thrown away by a precondition guard that failed before the normal mode was looked at even once. The scheduler responded by switching to another TS guess, so the reaction burned through its entire guess list — each guess costing a fresh optimisation and frequency job — and ended with no transition state at all.

The index space

  • The atom-count gate in analyze_ts_normal_mode_displacement() read the deduplicated r_species, which holds one entry for a species that participates twice.
  • The forming, breaking and changed bond indices, the atom map, find_equivalent_atoms() and the TS geometry all live in the per-occurrence expanded space produced by get_reactants_and_products().
  • For OH + OH <=> H2O + O the TS has 4 atoms while the deduplicated reactants report 2, so the gate never matched.

The count is now taken from get_reactants_and_products() — the same call get_bonds() uses to build the bond indices — so the two agree by construction rather than by a parallel re-derivation that can drift.

To be clear about what this part does and does not do: the two derivations are numerically identical today. This was verified across six reaction shapes — A + B, A + A, A + A + A, a species appearing in both wells, A + A -> A + A, and unimolecular — each compared against the atom count get_bonds() actually indexes into. All three agree in every case. The gain here is coupling, not behaviour: get_reactants_and_products() is implemented as exactly this expansion, so a re-derivation alongside it agrees only for as long as that implementation stays put.

A count mismatch is an inability to check, not a verdict

A mismatch means the bond indices cannot be applied to this geometry at all. It now logs a warning naming both counts and returns None — the value the function already returns when no frequency job is given and when the normal mode displacements cannot be parsed — instead of False.

The reason False is wrong is not only that it is harsh:

  • False's sole consumer is the scheduler, which responds by calling switch_ts() to try a different TS guess.
  • Every TSGuess is validated against the same expanded reactant well by ARCReaction.check_atom_balance(), so all guesses for a reaction have the same atom count.
  • The condition is therefore invariant under the remedy. Switching guesses cannot change the atom count that made the analysis impossible. False sends the one signal whose only consumer responds with an action that cannot help, and the cost is the whole guess list plus a TS that was never examined.
  • Meanwhile a genuine atom imbalance is already a hard ReactionError, raised at reaction construction and again from the scheduler before any of this runs — so a real imbalance never reaches this gate quietly.

None does not trigger switch_ts(), and ts_passed_checks() still treats it as not passed, so the TS is withheld from the checks it must pass without being destroyed. IRC validation remains an independent gate.

Both returns in the function were audited. The count mismatch was the only one that reported an inability to check as a negative verdict; the final return False, after every candidate mapping has been tried, is a genuine negative and is unchanged.

Why this sits on top of the other PR rather than beside it

Ordering matters. The get_reactants_xyz()/get_products_xyz() expansion in the PR below is a prerequisite for the count fix to be useful — with the count fix present but the expansion absent, any later guard that compares a TS against the concatenated reactant geometry sees an unexpanded reactant well and silently disables the check for every A + A reaction. Stacking keeps the two from ever being separated.

Tests

  • A genuinely mismatched atom count yields None rather than False. Fails without the fix with AssertionError: False is not None.
  • A check_freq_job()-level assertion that the same mismatch does not reach switch_ts(). Fails without the fix with AssertionError: False is not None.
  • An A + A reaction whose TS survives the count gate.

The scheduler-level test is deliberately built on a genuine count mismatch rather than on an A + A reaction. Once the count fix is in place an A + A reaction clears the gate (4 == 4) and exits at a later None, so it never reaches the branch being changed and cannot discriminate this fix.

@calvinp0 calvinp0 changed the title fix nmd aa reaction atom count Do not discard a TS when the normal mode displacement check cannot be run Aug 12, 2026
Comment thread arc/scheduler_test.py Fixed
Comment thread arc/scheduler_test.py Fixed
… run

analyze_ts_normal_mode_displacement() compares the TS atom count against the reactant
atom count, but derived the latter by re-implementing the per-occurrence expansion
alongside the function that defines it. The forming, breaking and changed bonds it then
applies to the TS geometry come from ARCReaction.get_bonds(), which indexes into
get_reactants_and_products(). The count is now taken from that same call, so the two
agree by construction rather than by a parallel derivation that can drift.

The count mismatch also returned False, and the scheduler calls switch_ts() whenever the
NMD verdict is False. A count mismatch means the bond indices cannot be applied to this
geometry at all, which is an inability to perform the analysis rather than an observation
about the mode; switching to another TS guess cannot change the atom count that made the
analysis impossible, so False sends the one signal whose only consumer responds with an
action that cannot help, at the cost of a TS that had passed its imaginary-frequency
check and had never been examined. A genuine atom imbalance is already rejected loudly by
ARCReaction.check_atom_balance(), which raises at reaction construction and in the
scheduler. The mismatch now logs a warning naming both counts and returns None, the value
the function already returns when no job is given and when the normal mode displacements
cannot be parsed. None does not trigger switch_ts, and ts_passed_checks() still treats it
as not passed, so the TS is withheld from the checks it must pass without being destroyed.

Both returns were audited: the count mismatch was the only one that reported an inability
to check as a negative verdict. The final `return False` after every candidate mapping has
been tried is a genuine negative and is unchanged.

Tests: a genuinely mismatched atom count now yields None rather than False (fails without
the fix with "AssertionError: False is not None"); a check_freq_job()-level assertion that
the same mismatch does not reach switch_ts (fails without the fix with the same message);
and an A + A reaction whose TS survives the count gate.
@calvinp0
calvinp0 force-pushed the fix_nmd_aa_reaction_atom_count branch from bc25603 to d7d2af9 Compare August 12, 2026 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants