From d1b67ea5658e104d69ec2a97a5d5a2ce99ee71a3 Mon Sep 17 00:00:00 2001 From: Calvin Pieters Date: Wed, 12 Aug 2026 11:56:13 +0300 Subject: [PATCH] Skip the TS normal mode displacement analysis when the ESS reports no 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. --- arc/checks/nmd.py | 10 +++-- arc/checks/nmd_test.py | 81 +++++++++++++++++++++++++++++++++++++++ arc/job/trsh.py | 9 +++-- arc/job/trsh_test.py | 15 +++++++- arc/parser/parser.py | 31 +++++++++++++++ arc/parser/parser_test.py | 21 ++++++++++ 6 files changed, 158 insertions(+), 9 deletions(-) diff --git a/arc/checks/nmd.py b/arc/checks/nmd.py index e6a3ed9013..dbf5c80166 100644 --- a/arc/checks/nmd.py +++ b/arc/checks/nmd.py @@ -47,6 +47,8 @@ def analyze_ts_normal_mode_displacement(reaction: ARCReaction, Returns: bool | None: Whether the TS normal mode displacement is consistent with the desired reaction. + ``None`` if the analysis could not be performed, either because no job was given + or because no normal mode displacements could be parsed from the job's output file. """ if job is None: return None @@ -71,11 +73,11 @@ def analyze_ts_normal_mode_displacement(reaction: ARCReaction, f'breaking bond indices refer to the reactant order, so they do not describe the intended ' f'atoms of this TS. Skipping the normal mode displacement analysis.') return None - try: - freqs, normal_mode_disp = parser.parse_normal_mode_displacement(log_file_path=job.local_path_to_output_file) - except NotImplementedError: - logger.warning(f'Could not parse frequencies for TS {reaction.ts_species.label}.') + parsed_modes = parser.get_normal_mode_displacement(log_file_path=job.local_path_to_output_file, + label=reaction.ts_species.label) + if parsed_modes is None: return None + normal_mode_disp = parsed_modes[1] amplitude_list = [amplitude] if isinstance(amplitude, (float, int)) else amplitude weights_array = get_weights_from_xyz(xyz=ts_xyz, weights=weights) diff --git a/arc/checks/nmd_test.py b/arc/checks/nmd_test.py index 5e9cedde86..3bae6ea8b4 100644 --- a/arc/checks/nmd_test.py +++ b/arc/checks/nmd_test.py @@ -9,6 +9,7 @@ import math import os import shutil +from unittest.mock import patch import numpy as np @@ -20,6 +21,7 @@ from arc.molecule import Molecule from arc.parser.parser import parse_normal_mode_displacement from arc.reaction import ARCReaction +from arc.scheduler import Scheduler from arc.species.species import ARCSpecies from arc.species.converter import check_xyz_dict from arc.species.vectors import rotate_vector @@ -539,6 +541,85 @@ def test_analyze_ts_normal_mode_displacement_for_hypervalence_nitrogen(self): weights=True) self.assertTrue(valid) + def test_analyze_ts_normal_mode_displacement_skips_an_unsupported_ess(self): + """Test that an ESS ARC cannot parse normal mode displacements from is skipped rather than raising.""" + for file_name in ['orca_neg_freq_ts.out', 'orca_example_freq.log', 'CH2O_freq_molpro.out', + 'C2H6_freq_QChem.out', 'CH2O_freq_terachem.dat']: + log_file_path = os.path.join(ARC_TESTING_PATH, 'freq', file_name) + self.assertEqual(parse_normal_mode_displacement(log_file_path=log_file_path), (None, None)) + self.generic_job.local_path_to_output_file = log_file_path + valid = nmd.analyze_ts_normal_mode_displacement(reaction=self.rxn_1, + job=self.generic_job, + amplitude=0.25) + self.assertIsNone(valid) + + def test_analyze_ts_normal_mode_displacement_skips_a_log_without_normal_modes(self): + """Test that a supported ESS log file that holds no normal modes is skipped rather than raising.""" + log_file_path = os.path.join(ARC_TESTING_PATH, 'freq', 'yml_no_freqs.yml') + self.assertEqual(parse_normal_mode_displacement(log_file_path=log_file_path), (None, None)) + self.generic_job.local_path_to_output_file = log_file_path + valid = nmd.analyze_ts_normal_mode_displacement(reaction=self.rxn_1, job=self.generic_job, amplitude=0.25) + self.assertIsNone(valid) + + def test_analyze_ts_normal_mode_displacement_reaches_a_verdict_for_an_xtb_log(self): + """Test that an ESS whose normal mode displacements do parse still reaches a verdict.""" + base_path = os.path.join(ARC_TESTING_PATH, 'composite', 'C3H7') + rxn = ARCReaction(r_species=[ARCSpecies(label='iC3H7', smiles='C[CH]C', + xyz=os.path.join(base_path, 'iC3H7.gjf'))], + p_species=[ARCSpecies(label='nC3H7', smiles='[CH2]CC', + xyz=os.path.join(base_path, 'nC3H7.gjf'))]) + xtb_path = os.path.join(ARC_TESTING_PATH, 'normal_mode', 'TS_0') + rxn.ts_species = ARCSpecies(label='TS', is_ts=True, xyz=os.path.join(xtb_path, 'g98.out')) + self.generic_job.local_path_to_output_file = os.path.join(xtb_path, 'output.out') + valid = nmd.analyze_ts_normal_mode_displacement(reaction=rxn, job=self.generic_job, amplitude=0.25) + self.assertIsInstance(valid, bool) + + def test_the_scheduler_does_not_switch_a_ts_for_an_unsupported_ess(self): + """Test that a TS is not discarded when its ESS reports no normal mode displacements.""" + rxn = ARCReaction(r_species=[ARCSpecies(label='CH4', smiles='C', xyz=self.ch4_xyz), + ARCSpecies(label='OH', smiles='[OH]', xyz="""O 0.48890387 0.0 0.0 + H -0.48890387 0.0 0.0""")], + p_species=[ARCSpecies(label='CH3', smiles='[CH3]', xyz="""C 0.0 0.0 0.0 + H 1.06690511 -0.17519582 0.05416493 + H -0.68531716 -0.83753536 -0.02808565 + H -0.38158795 1.01273118 -0.02607927"""), + ARCSpecies(label='H2O', smiles='O', xyz="""O -0.00032832 0.39781490 0.0 + H -0.76330345 -0.19953755 0.0 + H 0.76363177 -0.19827735 0.0""")]) + rxn.index = 0 + rxn.ts_label = 'TS_unsupported_ess' + rxn.ts_species = ARCSpecies(label='TS_unsupported_ess', is_ts=True, xyz=self.ts_1_xyz) + rxn.ts_species.rxn_index = 0 + project_directory = os.path.join(ARC_PATH, 'Projects', 'tmp_nmd_unsupported_ess_project') + self.addCleanup(shutil.rmtree, project_directory, ignore_errors=True) + sched = Scheduler(project='tmp_nmd_unsupported_ess_project', + ess_settings={'gaussian': ['local']}, + species_list=[rxn.ts_species] + rxn.r_species + rxn.p_species, + rxn_list=[rxn], + opt_level=Level(repr='b3lyp/6-31g'), + freq_level=Level(repr='b3lyp/6-31g'), + sp_level=Level(repr='b3lyp/6-31g'), + project_directory=project_directory, + testing=True, + ) + job = job_factory(job_adapter='gaussian', + species=[rxn.ts_species], + job_type='freq', + level=Level(method='b3lyp', basis='6-31g'), + project='tmp_nmd_unsupported_ess_project', + project_directory=project_directory, + ) + job.local_path_to_output_file = os.path.join(ARC_TESTING_PATH, 'freq', 'orca_neg_freq_ts.out') + job.job_status = ['done', {'status': 'done', 'keywords': [], 'error': '', 'line': ''}] + with patch.object(Scheduler, 'switch_ts') as mock_switch_ts: + freq_ok, switched = sched.post_freq_actions(label='TS_unsupported_ess', + job=job, + vibfreqs=[-1200.0, 500.0, 900.0]) + self.assertTrue(freq_ok) + self.assertFalse(switched) + self.assertFalse(mock_switch_ts.called) + self.assertIsNone(sched.species_dict['TS_unsupported_ess'].ts_checks['NMD']) + def test_translate_all_tuples_simultaneously(self): """Test the translate_all_tuples_simultaneously() function.""" translated_tuples = nmd.translate_all_tuples_simultaneously(list_1=[(0, 1)], diff --git a/arc/job/trsh.py b/arc/job/trsh.py index 8a8d248625..8f4ac2b864 100644 --- a/arc/job/trsh.py +++ b/arc/job/trsh.py @@ -30,9 +30,9 @@ from arc.species.converter import (displace_xyz, ics_to_scan_constraints) from arc.species.species import determine_rotor_symmetry from arc.species.vectors import calculate_dihedral_angle, calculate_distance -from arc.parser.parser import (parse_1d_scan_coords, +from arc.parser.parser import (get_normal_mode_displacement, + parse_1d_scan_coords, parse_geometry, - parse_normal_mode_displacement, parse_scan_args, parse_scan_conformers, determine_ess @@ -598,10 +598,11 @@ def trsh_negative_freq(label: str, factors = [0.25, 0.50, 0.75, 1.0, 1.5, 2.5] factor = factors[0] max_times_to_trsh_neg_freq = len(factors) + 1 - freqs, normal_modes_disp = parse_normal_mode_displacement(log_file_path=log_file, raise_error=False) - if not len(normal_modes_disp): + parsed_modes = get_normal_mode_displacement(log_file_path=log_file, label=label) + if parsed_modes is None: logger.error(f'Could not troubleshoot negative frequency for species {label}.') return [], [], output_errors, [] + freqs, normal_modes_disp = parsed_modes if len(neg_freqs_trshed) > max_times_to_trsh_neg_freq: logger.error(f'Species {label} was troubleshooted for negative frequencies too many times.') if 'rotors' not in job_types: diff --git a/arc/job/trsh_test.py b/arc/job/trsh_test.py index 251a9742cf..b2b98454da 100644 --- a/arc/job/trsh_test.py +++ b/arc/job/trsh_test.py @@ -15,7 +15,7 @@ from arc.common import ARC_TESTING_PATH, save_yaml_file from arc.exceptions import TrshError from arc.imports import settings -from arc.parser.parser import parse_1d_scan_energies +from arc.parser.parser import parse_1d_scan_energies, parse_normal_mode_displacement supported_ess = settings["supported_ess"] @@ -1016,6 +1016,19 @@ def test_trsh_negative_freq(self): self.assertEqual(output_errors, list()) self.assertEqual(output_warnings, list()) + def test_trsh_negative_freq_for_an_ess_without_normal_mode_displacements(self): + """Test that an ESS output file reporting no normal mode displacements is reported, not indexed.""" + for file_name in ['orca_neg_freq_ts.out', 'orca_example_freq.log', 'orca6_example.out', + 'CH2O_freq_molpro.out', 'C2H6_freq_QChem.out', 'CH2O_freq_terachem.dat']: + log_file = os.path.join(ARC_TESTING_PATH, 'freq', file_name) + self.assertEqual(parse_normal_mode_displacement(log_file_path=log_file), (None, None)) + current_neg_freqs_trshed, conformers, output_errors, output_warnings = \ + trsh.trsh_negative_freq(label='spc', log_file=log_file) + self.assertEqual(current_neg_freqs_trshed, list()) + self.assertEqual(conformers, list()) + self.assertEqual(output_errors, list()) + self.assertEqual(output_warnings, list()) + def test_scan_quality_check(self): """Test scan quality check for 1D rotor""" log_file = os.path.join(ARC_TESTING_PATH, 'rotor_scans', 'CH2OOH.out') diff --git a/arc/parser/parser.py b/arc/parser/parser.py index bcda968a83..3ad0d70ba5 100644 --- a/arc/parser/parser.py +++ b/arc/parser/parser.py @@ -267,6 +267,37 @@ def parser(log_file_path: str, raise_error: bool = False) -> return_type: ) +def get_normal_mode_displacement(log_file_path: str, + label: str = '', + ) -> tuple[np.ndarray, np.ndarray] | None: + """ + Get the frequencies and normal mode displacements reported in a frequency job's output file. + + ``None`` is returned, along with a warning naming the ESS, whenever the file yields no normal mode + displacements, so that a caller can skip an analysis that requires them rather than operate on + missing data. Only some of ARC's ESS parser adapters report normal mode displacements at all, + the rest report none for every output file they are given. + + Args: + log_file_path (str): The path to the frequency job's output file. + label (str, optional): The label of the species the job was run for, used in the warning message. + + Returns: + tuple[np.ndarray, np.ndarray] | None: The frequencies and the normal mode displacements, + ``None`` if they could not be parsed. + """ + parsed = parse_normal_mode_displacement(log_file_path=log_file_path) + freqs, normal_mode_disp = parsed if parsed is not None else (None, None) + if normal_mode_disp is None or not len(normal_mode_disp): + ess = determine_ess(log_file_path=log_file_path, raise_error=False) or 'unidentified ESS' + label_str = f' for {label}' if label else '' + logger.warning(f'Could not parse normal mode displacements{label_str} from the {ess} output file ' + f'{log_file_path}. Not every ESS parser adapter in ARC reports normal mode ' + f'displacements.') + return None + return freqs, normal_mode_disp + + def parse_1d_scan_energies_from_specific_angle(log_file_path: str, initial_angle: float, ) -> tuple[list[float] | None, list[float] | None]: diff --git a/arc/parser/parser_test.py b/arc/parser/parser_test.py index 8eb41f2163..538ec89f0e 100644 --- a/arc/parser/parser_test.py +++ b/arc/parser/parser_test.py @@ -345,6 +345,27 @@ def test_parse_normal_mode_displacement(self): [-0.16184923713199378, -0.3376354950974596, 0.787886990928027]], np.float64) np.testing.assert_almost_equal(normal_modes_disp[0], expected_normal_modes_disp_4_0) + def test_get_normal_mode_displacement_returns_the_parsed_displacements(self): + """Test that the frequencies and normal mode displacements are returned unchanged.""" + for log_file_path in [os.path.join(ARC_TESTING_PATH, 'freq', 'TS_CH4_OH.log'), + os.path.join(ARC_TESTING_PATH, 'freq', 'output.yml'), + os.path.join(ARC_TESTING_PATH, 'normal_mode', 'TS_0', 'output.out')]: + expected_freqs, expected_disp = parser.parse_normal_mode_displacement(log_file_path=log_file_path) + parsed_modes = parser.get_normal_mode_displacement(log_file_path=log_file_path, label='TS') + self.assertIsNotNone(parsed_modes, msg=log_file_path) + np.testing.assert_array_equal(parsed_modes[0], expected_freqs) + np.testing.assert_array_equal(parsed_modes[1], expected_disp) + + def test_get_normal_mode_displacement_returns_none_when_no_modes_are_reported(self): + """Test that an output file yielding no normal mode displacements returns None, not the (None, None) sentinel.""" + for file_name in ['orca_neg_freq_ts.out', 'orca_example_freq.log', 'orca6_example.out', + 'CH2O_freq_molpro.out', 'C2H6_freq_QChem.out', 'CH2O_freq_terachem.dat', + 'yml_no_freqs.yml']: + log_file_path = os.path.join(ARC_TESTING_PATH, 'freq', file_name) + self.assertEqual(parser.parse_normal_mode_displacement(log_file_path=log_file_path), (None, None)) + self.assertIsNone(parser.get_normal_mode_displacement(log_file_path=log_file_path), msg=file_name) + self.assertIsNone(parser.get_normal_mode_displacement(log_file_path=log_file_path, label='TS')) + def test_parse_xyz_from_file(self): """Test parsing xyz from a file""" path1 = os.path.join(ARC_TESTING_PATH, 'xyz', 'CH3C(O)O.gjf')