Make the IRC verdict meaningful, and never publish a rate silently from an IRC-invalid TS - #937
Open
calvinp0 wants to merge 3 commits into
Open
Make the IRC verdict meaningful, and never publish a rate silently from an IRC-invalid TS#937calvinp0 wants to merge 3 commits into
calvinp0 wants to merge 3 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
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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_irc_gated_kinetics
branch
from
August 2, 2026 08:51
414d0d3 to
723df52
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #937 +/- ##
==========================================
- Coverage 63.53% 63.40% -0.13%
==========================================
Files 114 114
Lines 38325 38366 +41
Branches 10030 10038 +8
==========================================
- Hits 24348 24325 -23
- Misses 11068 11113 +45
- Partials 2909 2928 +19
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 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
Benchmark reaction
r3_16(nitroethane ⇌ ethyl nitrite,intra_NO2_ONO_conversion) produced a fitted Arrhenius expression ofEa = 65.8 kJ/mol. The correct barrier is ~253.6 kJ/mol — the published rate coefficient was wrong by ~9 orders of magnitude in k at 1000 K.ARC had already determined the TS was invalid:
ts_checks['IRC'] = False, because both optimized IRC endpoints re-perceive as the product (the "TS" is a torsional saddle of ethyl nitrite, connecting product↔product). That verdict was written torestart.yml— and then the Arrhenius fit was written toRMG_kinetics.yml,rate_plots.pdfand the kinetics library with no indication that validation had failed.Arkane is not at fault: its
E0values are self-consistent and reproduce the fittedEaexactly (R1 −81.43,P1 −73.74,TS −18.07 kJ/mol). It correctly computed the rate for the structure it was given; the TS search supplied the wrong structure.Two changes
1.
ts_checks['IRC'] = Falsenow means only "checked and failed".check_irc_species_and_rxnpessimistically setFalseup front and only wroteTrueon success. An early return —rxn.get_bonds()raising in the fallback — leftFalsein place having compared nothing, soFalseconflated "checked and failed" with "could not check". It now starts atNoneand writesFalseonly where a comparison actually happened. (A second no-comparison path also existed purely because of the up-frontFalse: ifcheck_xyz_dict()orget_reactants_and_products()raised, the exception propagated out with a "failed" verdict already recorded.)2. Every kinetics artifact carries the failed-validation marker.
When
ts_checks['IRC'] is False, the marker is stamped onto the Arrheniuscomment, theRMG_kinetics.ymlentry, the kinetics-librarylongDesc, and therate_plots.pdftitle, and anERRORis logged naming the reaction.The rate is deliberately NOT suppressed — it is diagnostically valuable (it is how this bug was found). It is computed, recorded, and clearly labelled.
ts_checks['IRC'] is None(IRC not run, e.g.job_types: irc: false) is treated as unknown: not marked, not suppressed. Many users skip IRC because it is expensive; they are unaffected.Evidence
ts_test,common_test,processor_test,plotter_test,arkane_test).test_check_irc_identical_endpointsusesassertIs(..., False)rather thanassertFalse(...)—assertFalsepasses onNonetoo, so the pre-existing IRC test could not have caught this regression class.Ea = 253.639 kJ/molfrom a re-run that found the correct saddle, versus the RMG NOx2018 library value of253.8— agreement to 0.06%.Notes for review
except Exceptionin the fallback was kept (not narrowed) and made loud rather than silent.get_bonds()reachesatom_map→map_reaction,spc.copy()and.index()lookups, so the raisable surface is genuinely unbounded; narrowing it wrongly would turn a benign "cannot atom-map" into a scheduler crash mid-run.arc/tckdb/does not exist onmain.