Enforce the IRC check — reject a TS whose IRC endpoints do not match the wells - #938
Open
calvinp0 wants to merge 4 commits into
Open
Enforce the IRC check — reject a TS whose IRC endpoints do not match the wells#938calvinp0 wants to merge 4 commits into
calvinp0 wants to merge 4 commits into
Conversation
check_irc_species_and_rxn() pessimistically set ts_checks['IRC'] = False before performing any comparison, and one path (the bond-list fallback failing to obtain the reaction's connectivity) returned while leaving that False in place. A False IRC verdict therefore conflated "the IRC endpoints were compared and do not match the reactants/products" with "the IRC endpoints could not be compared at all", which makes False unusable as a gate. The check now starts from None (unknown, the ts_checks default) and only writes False where a comparison was actually carried out and did not match: after a failed isomorphism comparison, and after a failed bond-list comparison. The no-comparison path leaves None and logs the reason, including the exception.
…hem silently A TS whose IRC endpoints re-perceive as something other than the reaction's reactants and products does not describe that reaction, yet ARC still fitted and published an Arrhenius expression for it with nothing in the output indicating that validation had failed (benchmark reaction nitroethane <=> ethyl nitrite, family intra_NO2_ONO_conversion: ts_checks IRC False, k off by ~9 orders of magnitude at 1000 K). The rate is deliberately still computed and reported - it is diagnostically valuable and it is how this class of bug is found - but every artifact carrying it is now labeled, via the new common.get_ts_validation_comment() helper: - Arkane parse_reaction_kinetics(): a 'ts_validation' key on reaction.kinetics and the marker appended to the Arrhenius comment. - processor.compare_rates(): a 'ts_validation' field in output/RMG_kinetics.yml. - plotter.save_kinetics_lib(): the marker as the Arrhenius comment and at the top of the entry's longDesc in the RMG kinetics library. - plotter.draw_kinetics_plots(): the marker in the rate_plots.pdf plot title. Each of these also logs an error naming the reaction. Only a ts_checks['IRC'] value of False (checked and failed) triggers the marker; None (not checked, e.g. job_types irc: false) and True are left untouched.
calvinp0
changed the title
calvinp0
force-pushed
the
fix_enforce_irc_check
branch
from
August 2, 2026 08:46
afa108b to
8491574
Compare
Two changes, both confined to lines this PR already touched: 1. Drop the TYPE_CHECKING import of ARCSpecies from arc.common. The annotation on get_ts_validation_comment is already a quoted string, so the import is not needed at runtime or for type checking to resolve lazily. Its only effect was to add a common -> species import edge that CodeQL reads as a module-level cycle (arc.common -> arc.species.species -> arc.common), cascading into 41 py/unsafe-cyclic-import alerts across every module in that cycle. 2. Drop ARC_PATH from the arc.common import in arc/statmech/arkane_test.py. It was already unused on main; adding TS_IRC_FAILED_MARKER to that line made CodeQL count the pre-existing py/unused-import as new in changed code.
calvinp0
force-pushed
the
fix_enforce_irc_check
branch
from
August 2, 2026 08:51
8491574 to
9fdc4b3
Compare
…he wells The IRC verdict was written into ``TS.ts_checks['IRC']`` but never read by any production code path: ``check_ts()`` explicitly exempted 'IRC' from ``ts_passed_checks()``, and ``Scheduler.check_irc_species()`` only computed the verdict and dropped it. A TS whose two IRC endpoints both optimize to the same well was therefore accepted, and a rate coefficient that does not describe the requested reaction was published. A positively failed IRC check now rejects the TS and triggers a search for a different TS guess, mirroring the existing treatment of a failed normal mode displacement check. The check is three-valued: only ``False`` rejects. ``None`` means the check was not performed (e.g., IRC jobs were not requested) and never rejects anything, in ``ts_passed_checks()`` as well. Rejection reuses ``switch_ts()``, which resets the TS checks, the output paths and job types, the rotors, and deletes the IRC species spawned from the rejected guess, so the newly chosen guess re-runs opt/freq/sp/IRC from a clean state. Termination is bounded by ``chosen_ts_list``: once every guess was tried, ``determine_most_likely_ts_conformer()`` sets ``ts_guesses_exhausted``, and the TS is marked unconverged instead of being switched again. Also guard the deletion of an empty ``running_jobs`` entry in the main loop, since rejecting a TS while processing the opt job of one of its IRC species removes that species' entry.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## fix_irc_gated_kinetics #938 +/- ##
==========================================================
+ Coverage 63.40% 63.51% +0.11%
==========================================================
Files 114 114
Lines 38366 38387 +21
Branches 10038 10042 +4
==========================================================
+ Hits 24325 24383 +58
+ Misses 11113 11086 -27
+ Partials 2928 2918 -10
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
marked this pull request as ready for review
August 2, 2026 11:36
This was referenced Aug 10, 2026
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.
What went wrong
ts_checks['IRC']was computed correctly and read nowhere. It is written inarc/checks/ts.pyand consumed by no production code. It was explicitly exempted:and
check_ts's docstring still carriedTodo: check IRC. By contrast, an NMD failure does act — it logs and callsswitch_tsto try another guess. There was no IRC equivalent.Consequence:
r3_16accepted a TS that connects product↔product, and published a rate wrong by ~9 orders of magnitude (see #937). A re-run that happened to reject that guess found the true saddle and producedEa = 253.639 kJ/mol, matching the literature.The fix
exemptions=['E0', 'warnings']), and drop the staleTodo.Scheduler.process_irc_verdict: onIRC is False, log at ERROR and callswitch_ts, mirroring the existing NMD path, with the same E0-style exhaustion fallback (restores the verdict and marks the TS unconverged when no guesses remain).check_all_done's "stay unconverged" guard extended fromE0to['E0', 'IRC'].Three-valued logic — the trap this had to avoid
ts_checksvalues areTrue/False/None, andts_passed_checksusednot value, which treatsNoneandFalseidentically. Naively un-exempting IRC would have failed every run that skips IRC (job_types: irc: false). So:Scoped to IRC only;
Nonesemantics for the other keys are unchanged.Is switching the TS after IRC actually supported? Yes — verified
IRC runs later in the pipeline than NMD, so this needed checking.
switch_ts→delete_all_species_jobsresetsoutput[ts]['paths'], alljob_types,convergence, discards pending pipe work, deletes the IRC species spawned from the rejected guess and setsirc_label = None— sospawn_post_opt_jobsre-enqueues the full chain for the new guess. No stale state survives. Termination is bounded bylen(ts_guesses)viachosen_ts_list.This surfaced one latent bug that only appears once switching is enabled:
check_irc_speciesis reached while the main loop iterates the IRC species' label; rejecting the TS deletes that label, and the loop then diddel self.running_jobs[label]unconditionally →KeyError. Now guarded.Measured impact across all 61 completed benchmark runs
r3_16— the invalid TS is rejected, the search continues, and the wrongEa = 65.8is no longer published.IRC = None(r2_11,r2_19,r3_01,r3_10) are unaffected by construction.IRC = Trueruns: behaviour identical, one extra INFO line.Evidence
60 tests pass (
ts_test,scheduler_test), including exhaustion-termination and theNone→ no-switch case. Full suite:2501 passed, 5 failed— the 5 are pre-existing TorchANI path failures, unrelated to the touched modules.174 tests pass with both stack layers applied together — the first time these two changes were exercised as a unit.