Skip to content

Commit 5f9fbfa

Browse files
committed
Show a Level's args in its string form without losing its freq scale factor
`Level.__init__` normalises `args` into both a `keyword` and a `block` slot, so a keyword-only level (the common case) has an empty `block` slot and `all(val for val in self.args.values())` is False. `__str__` therefore dropped args entirely: two levels differing only in args compared equal (`__eq__` is str-based) while their `as_dict()` differed. The predicate now uses `any()`. Flipping that predicate alone silently changes a physical quantity. `assign_frequency_scale_factor()` keys on `str(level)` and returns None on a miss. All 67 keys in `data/freq_scale_factors.yml` have the form `method/basis[, solvation_method: ..., solvent: ..., software: ...]`; none carries args. Once args entered the string, every args-carrying level lost its scale factor, and since `calc_freq_factor` defaults to True that miss spawns a full campaign of QM jobs to recompute the factor by Truhlar's method. The database is keyed on the physical level of theory - method, basis, dispersion, solvation, software - whereas args are ESS input-file directives (extra route keywords and input blocks) that the database has never distinguished. The lookup key is therefore built from a reduced string rather than from `str(level)`: `get_freq_scale_factor_key()` hands the same key to both consumers, `assign_frequency_scale_factor()` and `output._resolve_freq_scale_factor_source()`. `args` are the only attribute dropped. Everything else the string carries stays in the key, including attributes the database happens not to use: `dispersion`, `auxiliary_basis` and `cabs` are part of the model chemistry, and `year` and `software_version` select a specific parameterisation elsewhere in ARC, so for all of them "no published factor for this level" is the true answer rather than a reason to widen the match. Keying on something narrower, such as `Level.simple()`, would match nothing at all, since every key carries `software:` and `simple()` omits it. A level with both arg slots populated missed the database even before this change and now resolves, consistent with the keyword-only and block-only cases. The printing loop only ever emitted the `keyword` slot, so a block-only level would have printed `keyword args: {}` - meaningless output and a broken key. Every non-empty slot is now printed under its own name. `utils/scale.py` prints a level as a yaml key for pasting into the database; that key now comes from the same helper. Its project folder name still derives from `str(level)`, so levels differing only in args keep separate directories, but `rename_level()` substituted only `/ * + ( ) space :` and now maps every remaining character outside `settings['valid_chars']` - the set ARC already requires of a project name - onto `_`. Args reach that name and can carry `{ } ' % = \ " $` and backticks, and the name is joined to a path that is passed to `shutil.rmtree()` and interpolated into double-quoted shell commands. `__eq__` is str-based, so levels differing only in args no longer compare equal. Audited: the scheduler's "sp reuses the opt output" shortcut and its conf_sp/conf_opt comparison now treat args-differing levels as different, which is the conservative direction - an extra job rather than reusing an output computed under different directives. Pipe batching is unaffected: `TaskSpec.level` is a `Level.as_dict()` dict, and the planner compares dicts, not strings. `as_dict()` is untouched here and still drops keyword-only args, so on its own this change leaves `__str__` and `as_dict()` asymmetric in the opposite direction to the one described above. The `as_dict()` predicate is fixed separately in the branch that preserves args through the as_dict round-trip; the two are independent and merge cleanly. Looked for an existing normaliser before adding one: nothing strips args. `Level.simple()` drops software and solvation, `statmech.arkane._level_to_str()` builds Arkane `LevelOfTheory` strings, and `scale.rename_level()` only sanitises a string for folder naming.
1 parent f45edb0 commit 5f9fbfa

6 files changed

Lines changed: 132 additions & 14 deletions

File tree

arc/level.py

Lines changed: 35 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,18 @@ def __str__(self) -> str:
125125
"""
126126
Return a humane-readable string representation of the object.
127127
128+
Returns:
129+
str: The level of theory string representation.
130+
"""
131+
return self.string_representation()
132+
133+
def string_representation(self, include_args: bool = True) -> str:
134+
"""
135+
Return a humane-readable string representation of the object.
136+
137+
Args:
138+
include_args (bool, optional): Whether to include the software-specific ``args``.
139+
128140
Returns:
129141
str: The level of theory string representation.
130142
"""
@@ -149,12 +161,10 @@ def __str__(self) -> str:
149161
str_ += f', software: {self.software}'
150162
if self.software_version is not None:
151163
str_ += f', software_version: {self.software_version}'
152-
if self.args is not None and self.args and all([val for val in self.args.values()]):
153-
if any([key == 'keyword' for key in self.args.keys()]):
154-
str_ += ', keyword args:'
155-
for key, arg in self.args.items():
156-
if key == 'keyword':
157-
str_ += f' {arg}'
164+
if include_args and self.args is not None and any([val for val in self.args.values()]):
165+
for key, arg in self.args.items():
166+
if arg:
167+
str_ += f', {key} args: {arg}'
158168
return str_
159169

160170
def copy(self):
@@ -464,6 +474,24 @@ def determine_compatible_ess(self):
464474
self.compatible_ess.append(ess)
465475

466476

477+
def get_freq_scale_factor_key(level: str | dict | Level) -> str:
478+
"""
479+
Get the key under which a level of theory is stored in ``data/freq_scale_factors.yml``.
480+
The key is the level of theory string representation without the software-specific ``args``.
481+
482+
Args:
483+
level (str | dict | Level): The level of theory.
484+
485+
Returns:
486+
str: The frequency scale factor database key.
487+
"""
488+
if isinstance(level, dict):
489+
level = Level(repr=level)
490+
if isinstance(level, Level):
491+
return level.string_representation(include_args=False)
492+
return str(level)
493+
494+
467495
def assign_frequency_scale_factor(level: str | Level) -> float | None:
468496
"""
469497
Assign a frequency scaling factor to a level of theory.
@@ -484,8 +512,7 @@ def assign_frequency_scale_factor(level: str | Level) -> float | None:
484512
except ValueError:
485513
# A string ARC cannot parse as a level has no scale factor on file.
486514
return None
487-
level_str = str(level)
488-
entry = freq_scale_factors.get(level_str)
515+
entry = freq_scale_factors.get(get_freq_scale_factor_key(level))
489516
if entry is not None:
490517
return entry['factor'] if isinstance(entry, dict) else entry
491518
return None

arc/level_test.py

Lines changed: 68 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
import unittest
1010

1111
from arc.common import ARC_PATH, read_yaml_file
12-
from arc.level import Level, assign_frequency_scale_factor
12+
from arc.level import Level, assign_frequency_scale_factor, get_freq_scale_factor_key
1313

1414

1515
class TestLevel(unittest.TestCase):
@@ -235,6 +235,61 @@ def test_str(self):
235235
self.assertEqual(str(Level(basis='def2svp', compatible_ess=['gaussian', 'terachem'],method='wb97xd',
236236
method_type='dft', software='gaussian')), 'wb97xd/def2svp, software: gaussian')
237237

238+
def test_str_with_args(self):
239+
"""Test the __str__() method for levels carrying args."""
240+
keyword_level = Level(method='wb97xd', basis='def2tzvp', args={'keyword': {'general': 'IOp(99/33=1)'}})
241+
self.assertEqual(str(keyword_level),
242+
"wb97xd/def2tzvp, software: gaussian, keyword args: {'general': 'iop(99/33=1)'}")
243+
block_level = Level(method='wb97xd', basis='def2tzvp', args={'block': {'general': '%scf MaxIter 500 end'}})
244+
self.assertEqual(str(block_level),
245+
"wb97xd/def2tzvp, software: gaussian, block args: {'general': '%scf maxiter 500 end'}")
246+
both_level = Level(method='wb97xd', basis='def2tzvp',
247+
args={'keyword': {'general': 'IOp(99/33=1)'}, 'block': {'general': '%scf MaxIter 500 end'}})
248+
self.assertEqual(str(both_level),
249+
"wb97xd/def2tzvp, software: gaussian, keyword args: {'general': 'iop(99/33=1)'}, "
250+
"block args: {'general': '%scf maxiter 500 end'}")
251+
self.assertNotEqual(keyword_level, block_level)
252+
self.assertNotEqual(keyword_level, Level(method='wb97xd', basis='def2tzvp'))
253+
254+
def test_str_keeps_year_and_software_version(self):
255+
"""Test that __str__() retains the year and the software_version."""
256+
self.assertEqual(str(Level(method='wb97xd', basis='def2tzvp', year=2023)),
257+
'wb97xd/def2tzvp, year: 2023, software: gaussian')
258+
self.assertEqual(str(Level(method='wb97xd', basis='def2tzvp', software_version=16)),
259+
'wb97xd/def2tzvp, software: gaussian, software_version: 16')
260+
self.assertNotEqual(Level(method='wb97xd', basis='def2tzvp', year=2023),
261+
Level(method='wb97xd', basis='def2tzvp'))
262+
self.assertNotEqual(Level(method='wb97xd', basis='def2tzvp', software_version=16),
263+
Level(method='wb97xd', basis='def2tzvp'))
264+
265+
def test_get_freq_scale_factor_key(self):
266+
"""Test the get_freq_scale_factor_key() function."""
267+
key = 'wb97xd/def2tzvp, software: gaussian'
268+
self.assertEqual(get_freq_scale_factor_key(key), key)
269+
self.assertEqual(get_freq_scale_factor_key(Level(method='wb97xd', basis='def2tzvp')), key)
270+
self.assertEqual(get_freq_scale_factor_key(Level(method='wb97xd', basis='def2tzvp',
271+
args={'keyword': {'general': 'IOp(99/33=1)'}})), key)
272+
self.assertEqual(get_freq_scale_factor_key(Level(method='wb97xd', basis='def2tzvp',
273+
args={'block': {'general': '%scf MaxIter 500 end'}})), key)
274+
self.assertEqual(get_freq_scale_factor_key({'method': 'wb97xd', 'basis': 'def2tzvp',
275+
'args': {'keyword': {'general': 'IOp(99/33=1)'}}}), key)
276+
self.assertEqual(get_freq_scale_factor_key(Level(method='wb97xd', basis='def2tzvp', solvation_method='smd',
277+
solvent='water',
278+
args={'keyword': {'general': 'IOp(99/33=1)'}})),
279+
'wb97xd/def2tzvp, solvation_method: smd, solvent: water, software: gaussian')
280+
281+
def test_get_freq_scale_factor_key_keeps_the_level_identity(self):
282+
"""Test that get_freq_scale_factor_key() retains every attribute other than args."""
283+
self.assertEqual(get_freq_scale_factor_key(Level(method='b3lyp', basis='def2tzvp', dispersion='gd3bj')),
284+
'b3lyp/def2tzvp, dispersion: gd3bj, software: gaussian')
285+
self.assertEqual(get_freq_scale_factor_key(Level(method='dlpno-ccsd(t)', basis='def2-tzvp',
286+
auxiliary_basis='def2-tzvp/c')),
287+
'dlpno-ccsd(t)/def2-tzvp, auxiliary_basis: def2-tzvp/c, software: orca')
288+
self.assertEqual(get_freq_scale_factor_key(Level(method='wb97xd', basis='def2tzvp', year=2023,
289+
software_version=16,
290+
args={'keyword': {'general': 'IOp(99/33=1)'}})),
291+
'wb97xd/def2tzvp, year: 2023, software: gaussian, software_version: 16')
292+
238293
def test_assign_frequency_scale_factor(self):
239294
"""Test the assign_frequency_scale_factor() method."""
240295
self.assertEqual(assign_frequency_scale_factor(Level(method='CCSD(T)', basis='cc-pvtz')), 0.975)
@@ -256,6 +311,18 @@ def test_assign_frequency_scale_factor_from_str(self):
256311
with self.assertRaises(ValueError):
257312
Level(repr=repr_)
258313

314+
def test_assign_frequency_scale_factor_with_args(self):
315+
"""Test that assign_frequency_scale_factor() resolves for levels carrying args."""
316+
self.assertEqual(assign_frequency_scale_factor(
317+
Level(method='wb97xd', basis='Def2TZVP', args={'keyword': {'general': 'IOp(99/33=1)'}})), 0.988)
318+
self.assertEqual(assign_frequency_scale_factor(
319+
Level(method='wb97xd', basis='Def2TZVP', args={'block': {'general': '%scf MaxIter 500 end'}})), 0.988)
320+
self.assertEqual(assign_frequency_scale_factor(
321+
Level(method='wb97xd', basis='Def2TZVP', args={'keyword': {'general': 'IOp(99/33=1)'},
322+
'block': {'general': '%scf MaxIter 500 end'}})), 0.988)
323+
self.assertEqual(assign_frequency_scale_factor(
324+
Level(method='CBS-QB3', args={'keyword': {'general': 'IOp(99/33=1)'}})), 1.004)
325+
259326

260327
if __name__ == '__main__':
261328
unittest.main(testRunner=unittest.TextTestRunner(verbosity=2))

arc/output.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
from arc.imports import settings
2020
from arc.job.env_run import rmg_env_command
2121
from arc.job.local import execute_command
22+
from arc.level import get_freq_scale_factor_key
2223
from arc.parser.parser import parse_1d_scan_energies, parse_e_elect, parse_ess_version, parse_opt_steps, parse_zpe_correction
2324
from arc.species.converter import xyz_to_str
2425
from arc.statmech.arkane import (
@@ -188,8 +189,7 @@ def _resolve_freq_scale_factor_source(freq_level) -> str | None:
188189

189190
sources = data.get('sources', {})
190191
factors = data.get('freq_scale_factors', {})
191-
level_key = str(freq_level) if not isinstance(freq_level, str) else freq_level
192-
entry = factors.get(level_key)
192+
entry = factors.get(get_freq_scale_factor_key(freq_level))
193193
if not isinstance(entry, dict):
194194
return None
195195
source_key = entry.get('source')

arc/output_test.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,14 @@ def test_known_level_returns_citation(self):
9696
self.assertIsNotNone(result)
9797
self.assertIn('10.1021/ct100326h', result)
9898

99+
def test_known_level_with_args_returns_citation(self):
100+
"""A level carrying args should resolve to the same citation as the same level without args."""
101+
level = Level(method='wb97xd', basis='def2tzvp', software='gaussian',
102+
args={'keyword': {'general': 'IOp(99/33=1)'}})
103+
result = _resolve_freq_scale_factor_source(level)
104+
self.assertIsNotNone(result)
105+
self.assertIn('10.1021/ct100326h', result)
106+
99107

100108
class TestParseThermoDataBlock(unittest.TestCase):
101109
"""Tests for parse_thermo_data_block in arkane.py."""

arc/utils/scale.py

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,8 @@
1717
initialize_log,
1818
time_lapse,
1919
)
20-
from arc.level import Level
20+
from arc.imports import settings
21+
from arc.level import Level, get_freq_scale_factor_key
2122
from arc.parser.parser import parse_zpe_correction
2223
from arc.scheduler import Scheduler
2324
from arc.species.species import ARCSpecies
@@ -30,6 +31,8 @@
3031

3132
logger = get_logger()
3233

34+
valid_chars = settings['valid_chars']
35+
3336
HEADER = 'FREQ: A PROGRAM FOR OPTIMIZING SCALE FACTORS (Version 1)\n'\
3437
' written by \n'\
3538
'Haoyu S. Yu, Lucas J. Fiedler, I.M. Alecu, and Donald G. Truhlar\n'\
@@ -241,7 +244,8 @@ def summarize_results(lambda_zpes: list,
241244
text += f'(execution time: {execution_time})\n'
242245
logger.info(text)
243246
f.write(text)
244-
database_formats.append(f""" '{level}': {harmonic_freq_scaling_factor:.3f}, # [4]\n""")
247+
database_formats.append(
248+
f""" '{get_freq_scale_factor_key(level)}': {harmonic_freq_scaling_factor:.3f}, # [4]\n""")
245249
logger.info(database_text)
246250
f.write(database_text)
247251
for database_format in database_formats:
@@ -355,4 +359,4 @@ def rename_level(level: str) -> str:
355359
level = level.replace(')', 'b')
356360
level = level.replace(' ', '_')
357361
level = level.replace(':', '..')
358-
return level
362+
return ''.join([char if char in valid_chars else '_' for char in level])

arc/utils/scale_test.py

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
get_species_list,
1616
rename_level,
1717
summarize_results,
18+
valid_chars,
1819
)
1920

2021

@@ -120,6 +121,17 @@ def test_rename_level(self):
120121
self.assertEqual(renamed_level2, 'wb97xd_6-311pGb2d,2pb')
121122
self.assertEqual(renamed_level3, 'wb97xd_aug-ccpZQZ,_solvation_method.._SMD,_solvent.._DMSO,_software.._gaussian')
122123

124+
def test_rename_level_sanitizes_args(self):
125+
"""Test that rename_level() maps the characters an args-carrying level contributes onto valid ones."""
126+
level = Level(method='wb97xd', basis='def2tzvp', software='orca',
127+
args={'block': {'general': '%scf\nMaxIter 500\nend'}})
128+
renamed_level = rename_level(str(level))
129+
for char in renamed_level:
130+
self.assertIn(char, valid_chars)
131+
self.assertNotIn(os.sep, renamed_level)
132+
self.assertEqual(rename_level("a/b, keyword args: {'general': '$(rm -rf ~)'}"),
133+
'a_b,_keyword_args..___general_..___brm_-rf__b__')
134+
123135
@classmethod
124136
def tearDownClass(cls):
125137
"""

0 commit comments

Comments
 (0)