Skip the TS normal mode displacement analysis when the ESS reports no… - #968
Draft
calvinp0 wants to merge 1 commit into
Draft
Skip the TS normal mode displacement analysis when the ESS reports no…#968calvinp0 wants to merge 1 commit into
calvinp0 wants to merge 1 commit into
Conversation
calvinp0
force-pushed
the
fix_nmd_unsupported_ess_guard
branch
from
August 12, 2026 09:48
4d76df3 to
e59da7d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## fix_nmd_freq_frame_geometry #968 +/- ##
===============================================================
+ Coverage 64.14% 64.22% +0.08%
===============================================================
Files 119 119
Lines 39555 39564 +9
Branches 10262 10264 +2
===============================================================
+ Hits 25371 25410 +39
+ Misses 11205 11179 -26
+ Partials 2979 2975 -4
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_nmd_unsupported_ess_guard
branch
from
August 14, 2026 16:45
e59da7d to
336ed3f
Compare
calvinp0
force-pushed
the
fix_nmd_unsupported_ess_guard
branch
from
August 14, 2026 19:16
336ed3f to
2a02180
Compare
calvinp0
force-pushed
the
fix_nmd_unsupported_ess_guard
branch
from
August 15, 2026 13:48
2a02180 to
9d6d661
Compare
… modes
analyze_ts_normal_mode_displacement() guarded parse_normal_mode_displacement() with an
`except NotImplementedError` that can never fire. No ESS parser adapter raises it: Orca, Molpro,
Q-Chem, TeraChem, CFOUR and Psi4 each return the sentinel `(None, None)` instead, and Gaussian,
xtb and the YAML adapter return `(None, None)` when a file holds no modes. make_parser() only
consults `raise_error` when the adapter result is None, and `(None, None)` is not None, so the
sentinel flowed straight through the dead `except` to `normal_mode_disp[0]`:
TypeError: 'NoneType' object is not subscriptable
Reproduced on the shipped fixtures freq/orca_neg_freq_ts.out (a genuine negative-frequency Orca
TS), freq/orca_example_freq.log, freq/CH2O_freq_molpro.out, freq/C2H6_freq_QChem.out and
freq/CH2O_freq_terachem.dat.
Nothing catches it. The call chain Scheduler.check_freq_job -> post_freq_actions -> check_ts ->
check_normal_mode_displacement -> analyze_ts_normal_mode_displacement holds no try/except, nor does
Scheduler.schedule_jobs, Scheduler.__init__, ARC.execute or ARC.py's main(), so a single frequency
job run on any of those six programs aborts the whole ARC run at the point the TS is validated,
discarding every other species and reaction in it.
get_normal_mode_displacement() now returns the frequencies and the displacements, or None, warning
and naming the ESS via the existing determine_ess() when a file yields none, and the analysis
returns None (unknown) rather than False. This distinction is load-bearing:
Scheduler.post_freq_actions() calls switch_ts() when ts_checks['NMD'] is False, so reporting False
for an ESS ARC simply cannot read would silently discard converged transition states and search for
a replacement that would fail the same way. None is also the value populate_ts_checks() initialises
the entry to, so an unrun check and an unreadable one now agree.
Keyed on the returned value rather than on a list of which adapters implement the method, so an
adapter that gains normal mode displacement parsing is picked up with no change here, and a
supported ESS whose log happens to hold no modes is skipped by the same path.
trsh.trsh_negative_freq() had the same defect and is routed through the same helper. It unpacked
`freqs, normal_modes_disp` from the sentinel and immediately evaluated `len(normal_modes_disp)`:
TypeError: object of type 'NoneType' has no len()
reproduced on freq/orca_neg_freq_ts.out, freq/orca_example_freq.log, freq/orca6_example.out,
freq/CH2O_freq_molpro.out, freq/C2H6_freq_QChem.out and freq/CH2O_freq_terachem.dat. It is reachable
for any non-TS species: Scheduler.check_freq_job() and parse_composite_geo() both call
troubleshoot_negative_freq() when a frequency job converges with an imaginary frequency and
trsh_ess_jobs is on, and neither the scheduler nor ARC.execute catches it. The frequencies parse for
those files while the displacements do not -- freq/orca_neg_freq_ts.out reports 15 frequencies with
a minimum of -1271.62 cm^-1 -- so the negative frequency is detected and the troubleshooter is
entered on exactly the files whose displacements are missing. The helper takes the log file path
rather than the job object so that both call sites can use it; trsh_negative_freq() receives a path,
not a JobAdapter. Its existing "Could not troubleshoot negative frequency" branch is now reached
instead of the exception, so the species is left for the ordinary ESS troubleshooter.
Fixed here rather than in make_parser(). Making `(None, None)` trigger the documented
NotImplementedError for every consumer is the more correct contract, but it does not fix this bug:
`raise_error` defaults to False and no production caller of any make_parser() product passes True,
so the caller here would still receive the sentinel. Fixing it there would mean raising
unconditionally, which changes the return contract of all fifteen parse_* entry points at once,
including the remaining consumer of this one, ts.py's get_rxn_zone_atom_indices(), which passes
raise_error=False precisely to keep going, for no gain over the local guard.
Searched for an existing helper before adding one, by behaviour rather than by name: for any
"is this ESS supported for this parse method" predicate, for any guard against a parse method that
silently returns nothing, and for any registry of adapter capabilities. None exists. settings'
supported_ess lists the programs ARC can run jobs on, not what each parser adapter can read from
their output, and cannot answer this. determine_ess() is reused for the ESS name in the warning.
The helper lives in arc/parser/parser.py, beside parse_normal_mode_displacement whose sentinel it
interprets and beside determine_ess which it calls, so that both call sites reach one shared
implementation through an import that already exists on main: arc/checks/nmd.py imports
`from arc.parser import parser` and arc/job/trsh.py imports `from arc.parser.parser import ...`.
The module-level import graph of the arc package is therefore unchanged by this commit, verified by
walking every module's top-level imports on both revisions. Defining the helper in arc/checks/nmd.py
instead would have made arc/job/trsh.py import arc.checks.nmd, an edge main does not have and one
that closes a cycle, since arc/checks/nmd.py's own imports reach arc.job.trsh back through
arc/__init__.py's eager package imports. arc.parser.parser reaches neither arc.checks nor
arc.job.trsh at module level, in either direction. ts.py can reuse the helper when
get_rxn_zone_atom_indices(), which passes the same sentinel into get_rms_from_normal_mode_disp(), is
fixed. That third site is left alone here because it needs a decision about what an empty reaction
zone should mean, not a guard.
The helper's own contract is tested in arc/parser/parser_test.py next to it, over a Gaussian, a
YAML and an xtb log for the parsed case and over the six unreadable fixtures plus freq/yml_no_freqs.yml
for the None case. arc/checks/nmd_test.py keeps the analysis-level tests: that the analysis returns
None rather than raising for those files, that it still reaches a bool verdict for an xtb log, and
that the scheduler does not switch the TS on an unknown verdict.
calvinp0
force-pushed
the
fix_nmd_unsupported_ess_guard
branch
from
August 15, 2026 14:06
9d6d661 to
7d37f89
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.
Sits on #967. Reading order: #967 → #968 → #970. Both change the head of
analyze_ts_normal_mode_displacement(), but the causes are independent — #967 is about coordinate frames, this one is about ESSs that report no modes at all. Either can be reverted without resurrecting the other's bug.What breaks
TypeError.analyze_ts_normal_mode_displacement()guarded the parse withexcept NotImplementedError— but no parser adapter ever raises it. Only 3 of ARC's 9 concrete adapters return real data (Gaussian, xtb, YAML); the other 6 (CFour, Molpro, Orca, Psi4, QChem, TeraChem) return the sentinel(None, None).(None, None)is notNone, so the guard was dead code, execution fell through tonormal_mode_disp[0], and subscriptingNoneraisedTypeError.check_ts()call site inarc/scheduler.pyis inside atry, so the exception propagates out of the scheduler toARC.py::main(). One Orca frequency job was enough to end the run.arc/job/trsh.py::trsh_negative_freq, which unpacked the sentinel and then evaluatedlen(normal_modes_disp)—TypeError: object of type 'NoneType' has no len(), reproduced againstfreq/orca_neg_freq_ts.out,freq/orca_example_freq.log,freq/orca6_example.out,freq/CH2O_freq_molpro.out,freq/C2H6_freq_QChem.outandfreq/CH2O_freq_terachem.dat. It is reachable for any non-TS species:Scheduler.check_freq_job()andparse_composite_geo()both calltroubleshoot_negative_freq()when a frequency job converges with an imaginary frequency andtrsh_ess_jobsis on, and nothing on that path catches it either. Frequencies parse for those files while displacements do not (orca_neg_freq_ts.outreports 15 frequencies, minimum −1271.62 cm⁻¹), so the troubleshooter is entered on exactly the files whose displacements are missing.What the fix does
get_normal_mode_displacement(), which centralizes the parse and returns the frequencies and displacements, orNone— with a warning naming the ESS and the file — whenever no normal mode displacements can be obtained. It handles both the(None, None)sentinel and an empty displacement array.trsh_negative_freq()now reaches its existing "Could not troubleshoot negative frequency" branch instead of raising, leaving the species to the ordinary ESS troubleshooter. The helper takes a log file path rather than aJobAdapterbecausetrsh_negative_freq()receives a path, and it returns the frequencies alongside the displacements because that caller needs both.arc/parser/parser.py, besideparse_normal_mode_displacement()whose sentinel it interprets and besidedetermine_ess()which it calls. Both call sites already import that module onmain—arc/checks/nmd.pyasfrom arc.parser import parser,arc/job/trsh.pyasfrom arc.parser.parser import ...— so one shared implementation is reached from both at module level, with no function-level import and no new import edge. Defining it inarc/checks/nmd.pyinstead would have madearc/job/trsh.pyimportarc.checks.nmd, an edgemaindoes not have and one that closes a cycle, sincenmd.py's own imports reacharc.job.trshback througharc/__init__.py's eager package imports (CodeQLpy/unsafe-cyclic-import).None), not TS rejected (False). An ESS that cannot report modes is telling us nothing about the TS, so it must not count as evidence against it.How it was verified
origin/mainand on this branch (followingif TYPE_CHECKINGandtry/exceptblocks, excluding function bodies). The production import graph is identical — the only difference anywhere is in the test modulearc/checks/nmd_test.py, which gainsarc.checks.ts,arc.schedulerandarc.species.vectors.arc.parser.parserreaches neitherarc.checksnorarc.job.trshat module level, in either direction.import arc.job.trshfirst,import arc.checks.nmdfirst,import arc.parser.parserfirst): the helper resolves fromarc.parser.parserin all three,arc/checks/nmd.pysees it, it returnsNonefororca_neg_freq_ts.outand a(15,) / (15, 7, 3)pair forTS_CH4_OH.log, andtrsh_negative_freq()returns four empty lists for the Orca file and 2 conformers for the Gaussian one.parser_test.pyfor the helper's own contract: the frequencies and displacements are returned unchanged for a Gaussian, a YAML and an xtb log;None— not the(None, None)sentinel — comes back for the six unreadable fixtures and forfreq/yml_no_freqs.yml.nmd_test.pyat the analysis level: the analysis is skipped for an unsupported ESS and for a log with no normal modes; it still reaches aboolverdict for an xtb log; and a scheduler-level test that the TS is not discarded when its ESS reports no displacements — the behaviour that actually matters downstream.trsh_test.py, over the six fixtures above, assertingtrsh_negative_freq()returns its four empty lists rather than raising. Proved failing before the fix with theTypeErrorquoted above.arc/checks/+arc/parser/parser_test.py+arc/job/trsh_test.pypass in full: 112 tests.Still not addressed
arc/checks/ts.py::get_rxn_zone_atom_indices()passes the same sentinel intoget_rms_from_normal_mode_disp(). It is left alone because it needs a decision about what an empty reaction zone should mean for the caller, not a guard — unlike the two sites here, which both already had a "give up cleanly" branch that the crash was jumping over. It can now reuse the helper when it is fixed.What I searched for
parse_normal_mode_displacement, to establish what they actually return on the unsupported path — this is what showed theNotImplementedErrorguard could never fire.settings['supported_ess']lists the programs ARC can run, not what each parser adapter can read.parse_normal_mode_displacementin the tree, which is how thetrsh.pysite was found;ts.pyis the only remaining one.