Skip to content

Use the frequency log's geometry for the TS normal mode displacement … - #967

Open
calvinp0 wants to merge 1 commit into
mainfrom
fix_nmd_freq_frame_geometry
Open

Use the frequency log's geometry for the TS normal mode displacement …#967
calvinp0 wants to merge 1 commit into
mainfrom
fix_nmd_freq_frame_geometry

Conversation

@calvinp0

@calvinp0 calvinp0 commented Aug 12, 2026

Copy link
Copy Markdown
Member

Base of a 3-PR stack — review this one first. Reading order: #967#968#970. All three change the head of analyze_ts_normal_mode_displacement(); they are split by cause, not by file.

What breaks

  • The TS normal mode displacement (NMD) check accepted or rejected a transition state using a geometry that was not in the same coordinate frame as the normal modes it was compared against.
  • The geometry came from the optimization job's final_xyz; the modes came from the frequency job's log. Gaussian reports modes in that job's own orientation, which is not generally the orientation of the opt geometry.
  • Measured over 996 real benchmark frequency jobs: 82% agree to within 1°, but 17.6% differ by ~180°. These are exact rigid rotations (fit RMSD ~1e-16), not numerical noise.
  • Adding a mode vector expressed in one basis to a coordinate expressed in another corrupts the cross term of every displaced bond length, so the rotation-invariance of a bond length does not rescue the result.

What the fix does

  • get_ts_xyz_in_normal_mode_frame() takes the TS geometry from the frequency job's output file — the same file the displacements are parsed from — so geometry and modes share a frame by construction.
  • For Gaussian that resolves to the last Standard orientation: block, falling back to Input orientation:. Both are right: the fallback only applies when Gaussian suppressed the standard orientation (e.g. under nosymm, which ARC's troubleshooting can add), and in that case the frequency analysis is done in the input orientation too.
  • Falls back to the TS species geometry when the freq log yields no geometry, or when the parsed geometry's element symbol sequence differs from the species'. Both fallbacks log a warning that the frame may not match.
  • The fallback is one-directional. When the species has no geometry — ARCSpecies.get_xyz() returns None for a TS with no final_xyz/initial_xyz/conformer and no TS guess carrying an initial_xyz — the parsed geometry is used rather than discarded. Returning None there would hand the caller nothing to measure: it immediately does len(ts_xyz['symbols']).
  • is_ts_atom_order_consistent_with_reactants() makes the analysis return None when the TS atom order differs from the concatenated reactant order. Nothing checked this before: the forming/breaking bond indices are indices into the reactant order, so on a mismatch the check silently scored the wrong atoms and still returned a confident verdict.

Why this shape

  • A frame mismatch or an atom-order mismatch is a missing precondition, not evidence against the TS — hence None (analysis not performed) rather than False (TS rejected).
  • ARC already pairs a geometry with modes from the same log elsewhere: arc/job/trsh.py::trsh_negative_freq parses both from one log_file before perturbing along the mode. The NMD check was the outlier, not the precedent.

How it was verified

  • Independently confirmed by the Eckart condition. Modes expressed in their own frame satisfy Σ mₐ(rₐ − R_com) × dₐ = 0, and that quantity is frame-dependent, so it is a direct test of whether the pairing is consistent: 73% of affected jobs exceed 3σ when evaluated in the opt frame, versus 0 of 111 in the freq frame.
  • 9 new tests: frames agree, a deliberately rotated geometry, an element symbol mismatch, a log with no geometry, a log that fails to parse, a species with no geometry at all, the atom-order guard (unit and end-to-end), and a scheduler-level test that an unknown (None) NMD verdict does not trigger a TS switch.
  • arc/checks/ passes in full — 60 tests.

Open note for the reviewer

  • The line just above the new guard, if n_ts != n_expected: return False, is the same class of missing precondition but still returns False. It predates this PR and is left alone here; worth deciding whether it should become None too.
  • analyze_ts_normal_mode_displacement() still does not tolerate a None geometry, which after this change can only arise when the freq log and the species both yield nothing. That len(ts_xyz['symbols']) on a possibly-None value is unchanged from main and is left as-is rather than widened here.

What I searched for

  • Whether ARC already had a helper that resolves a geometry into the frame of a job's parsed modes — searched by behaviour (geometry / frame / orientation reconciliation) rather than by an intended name. trsh_negative_freq does it inline for a different purpose; there was no reusable helper to import.
  • Confirmed the geometry parse goes through the existing arc/parser entry point rather than a new reader, and that the symbol-sequence comparison had no existing equivalent.

@calvinp0
calvinp0 force-pushed the fix_nmd_freq_frame_geometry branch from 80d7504 to ba0c867 Compare August 12, 2026 08:55
Comment thread arc/checks/nmd_test.py Fixed
@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.41%. Comparing base (d033cbb) to head (b5e1935).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #967      +/-   ##
==========================================
- Coverage   64.43%   64.41%   -0.02%     
==========================================
  Files         119      119              
  Lines       39601    39617      +16     
  Branches    10269    10270       +1     
==========================================
+ Hits        25515    25520       +5     
- Misses      11109    11112       +3     
- Partials     2977     2985       +8     
Flag Coverage Δ
functionaltests 64.41% <ø> (-0.02%) ⬇️
unittests 64.41% <ø> (-0.02%) ⬇️

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/checks/nmd_test.py Fixed
@calvinp0
calvinp0 force-pushed the fix_nmd_freq_frame_geometry branch from bad172c to e0aff0f Compare August 15, 2026 13:48
@calvinp0
calvinp0 marked this pull request as ready for review August 20, 2026 05:24
Copilot AI lite review requested due to automatic review settings August 20, 2026 05:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

…analysis

analyze_ts_normal_mode_displacement() displaces the TS as x + a*w*d, taking x from
reaction.ts_species.get_xyz() and d from parser.parse_normal_mode_displacement() on the
frequency job's log. Those are two different coordinate frames: get_xyz() returns final_xyz,
which is written only by the opt handlers (Scheduler.parse_opt_geo and parse_composite_geo)
and is never refreshed by check_freq_job, while Gaussian reports its normal modes in the
standard orientation of the frequency job itself. When get_xyz() falls through to
initial_xyz, a conformer or a TS guess, the frames are unrelated altogether.

Adding a mode vector expressed in one frame to a coordinate expressed in another corrupts
the cross term of every displaced bond length, so the displaced geometry is not a rotation
of the correct one and the verdict is not rotation invariant. Any non-identity rotation
between the two frames breaks it, proper or improper; a pure translation is harmless.

get_ts_xyz_in_normal_mode_frame() sources the geometry from the same file the modes are
parsed from, and falls back to the species geometry, with a warning, when that file yields
no geometry, when parsing it raises, or when the parsed element symbol sequence differs from
the species' one, so mismatched atoms are never silently compared.
The fallback is one-directional: when the species has no geometry to compare against, the
parsed geometry is used rather than discarded, since it is the geometry the analysis exists
to obtain and its absence would leave the caller nothing to measure.

Verified against the shipped fixtures:
- Gaussian: parse_geometry() returns the last 'Standard orientation:' block, the frame the
  'Frequencies --' mode vectors are printed in. For freq/TS_CH4_OH.log it reproduces the
  species geometry to a 0.000 degree rotation (fit RMSD 1e-16), so the change is a no-op on
  the existing tests.
- nosymm: with no 'Standard orientation:' present, parse_geometry() falls back to
  'Input orientation:', which is the mode frame when Gaussian does not reorient and is also
  the geometry ARC submitted, i.e. final_xyz.
- composite and optfreq: parse_composite_geo() and parse_opt_geo() set final_xyz from the
  same log that check_freq_job() then reads, so those frames already agreed.
- xtb: parse_normal_mode_displacement() reads a sibling g98.out, and parse_geometry()
  returns None for the freq output.out of normal_mode/HO2 and normal_mode/TS_0, so the
  fallback preserves the previous behaviour exactly.
- Orca, Q-Chem, Molpro, TeraChem, CFOUR and Psi4 return (None, None) from
  parse_normal_mode_displacement(), so this analysis cannot run for them either way.
  Molpro's parse_geometry() raises TypeError, which the fallback absorbs rather than
  turning into a new failure mode.

Fixing this in nmd.py rather than refreshing final_xyz from the frequency log in the
scheduler keeps the blast radius to the one consumer that needs the mode frame. final_xyz is
the geometry ARC reports, saves to the restart and output files, passes to Arkane and feeds
to every subsequent job; rewriting it with a reoriented copy would rotate all of those for no
benefit, since none of them depend on the frame.

Correcting the frame also exposes a second precondition that was never enforced. The forming,
breaking and changed bond indices are indices into the concatenated reactant geometry, while
a TS geometry may order its atoms differently, for instance heavy atoms first where the
reactant concatenation interleaves them. The indices then address the wrong atoms entirely,
and the resulting bond lengths can happen to agree with the expected pattern, so the check
returned a confident verdict about atoms it was not looking at. Sharpening the geometry turns
some of those accidental passes into equally unfounded rejections.

is_ts_atom_order_consistent_with_reactants() compares the TS element symbol sequence against
get_reactants_xyz(), which concatenates in the same order as the existing atom count check
above it, and analyze_ts_normal_mode_displacement() now returns None rather than a bool when
they differ. None is what distinguishes 'could not check' from 'checked and failed':
Scheduler.check_freq_job() switches to another TS guess on `ts_checks['NMD'] is False`, so
returning False here would discard transition states on the strength of indices known to be
meaningless, while None leaves the guess in place. Canonicalising the atom order belongs at
ingestion in the TS adapters and is not attempted here.
@calvinp0
calvinp0 force-pushed the fix_nmd_freq_frame_geometry branch from e0aff0f to b5e1935 Compare August 20, 2026 05:51
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.

3 participants