Skip to content

Skip the TS normal mode displacement analysis when the ESS reports no… - #968

Draft
calvinp0 wants to merge 1 commit into
fix_nmd_freq_frame_geometryfrom
fix_nmd_unsupported_ess_guard
Draft

Skip the TS normal mode displacement analysis when the ESS reports no…#968
calvinp0 wants to merge 1 commit into
fix_nmd_freq_frame_geometryfrom
fix_nmd_unsupported_ess_guard

Conversation

@calvinp0

@calvinp0 calvinp0 commented Aug 12, 2026

Copy link
Copy Markdown
Member

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

  • A single frequency job run with most of ARC's ESSs kills the entire run with an uncaught TypeError.
  • analyze_ts_normal_mode_displacement() guarded the parse with except 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 not None, so the guard was dead code, execution fell through to normal_mode_disp[0], and subscripting None raised TypeError.
  • Neither check_ts() call site in arc/scheduler.py is inside a try, so the exception propagates out of the scheduler to ARC.py::main(). One Orca frequency job was enough to end the run.
  • The same defect sits at a second site, arc/job/trsh.py::trsh_negative_freq, which unpacked the sentinel and then evaluated len(normal_modes_disp)TypeError: object of type 'NoneType' has no len(), reproduced against 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 nothing on that path catches it either. Frequencies parse for those files while displacements do not (orca_neg_freq_ts.out reports 15 frequencies, minimum −1271.62 cm⁻¹), so the troubleshooter is entered on exactly the files whose displacements are missing.

What the fix does

  • Adds get_normal_mode_displacement(), which centralizes the parse and returns the frequencies and displacements, or None — 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.
  • Applies that helper at both sites. 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 a JobAdapter because trsh_negative_freq() receives a path, and it returns the frequencies alongside the displacements because that caller needs both.
  • The helper lives in arc/parser/parser.py, beside parse_normal_mode_displacement() whose sentinel it interprets and beside determine_ess() which it calls. Both call sites already import that module on mainarc/checks/nmd.py as from arc.parser import parser, arc/job/trsh.py as from 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 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 nmd.py's own imports reach arc.job.trsh back through arc/__init__.py's eager package imports (CodeQL py/unsafe-cyclic-import).
  • Treats "this ESS does not report normal modes" as analysis not performed (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

  • Import graph, measured not asserted. Every module's top-level imports were walked on origin/main and on this branch (following if TYPE_CHECKING and try/except blocks, excluding function bodies). The production import graph is identical — the only difference anywhere is in the test module arc/checks/nmd_test.py, which gains arc.checks.ts, arc.scheduler and arc.species.vectors. arc.parser.parser reaches neither arc.checks nor arc.job.trsh at module level, in either direction.
  • Import order, both ways, in fresh interpreters run from an isolated directory (import arc.job.trsh first, import arc.checks.nmd first, import arc.parser.parser first): the helper resolves from arc.parser.parser in all three, arc/checks/nmd.py sees it, it returns None for orca_neg_freq_ts.out and a (15,) / (15, 7, 3) pair for TS_CH4_OH.log, and trsh_negative_freq() returns four empty lists for the Orca file and 2 conformers for the Gaussian one.
  • 2 new tests in parser_test.py for 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 for freq/yml_no_freqs.yml.
  • 4 new tests in nmd_test.py at the analysis level: the analysis is skipped for an unsupported ESS and for a log with no normal modes; it still reaches a bool verdict 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.
  • 1 test in trsh_test.py, over the six fixtures above, asserting trsh_negative_freq() returns its four empty lists rather than raising. Proved failing before the fix with the TypeError quoted above.
  • arc/checks/ + arc/parser/parser_test.py + arc/job/trsh_test.py pass in full: 112 tests.

Still not addressed

  • arc/checks/ts.py::get_rxn_zone_atom_indices() passes the same sentinel into get_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

  • Every parser adapter implementing parse_normal_mode_displacement, to establish what they actually return on the unsupported path — this is what showed the NotImplementedError guard could never fire.
  • Whether an existing helper already normalized "parsed or not" for this parser call before adding one; whether any "is this ESS supported for this parse method" predicate or registry of adapter capabilities exists. None does — settings['supported_ess'] lists the programs ARC can run, not what each parser adapter can read.
  • Every other caller of parse_normal_mode_displacement in the tree, which is how the trsh.py site was found; ts.py is the only remaining one.

@calvinp0
calvinp0 force-pushed the fix_nmd_unsupported_ess_guard branch from 4d76df3 to e59da7d Compare August 12, 2026 09:48
@calvinp0
calvinp0 changed the base branch from main to fix_nmd_freq_frame_geometry August 12, 2026 09:48
@codecov

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.22%. Comparing base (e0aff0f) to head (7d37f89).

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     
Flag Coverage Δ
functionaltests 64.22% <ø> (+0.08%) ⬆️
unittests 64.22% <ø> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread arc/job/trsh.py Fixed
@calvinp0
calvinp0 force-pushed the fix_nmd_unsupported_ess_guard branch from 2a02180 to 9d6d661 Compare August 15, 2026 13:48
Comment thread arc/job/trsh.py Fixed
… 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
calvinp0 force-pushed the fix_nmd_unsupported_ess_guard branch from 9d6d661 to 7d37f89 Compare August 15, 2026 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Module: trsh Troubleshooting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants