Fix TS guess execution times zeroing on restart, relative energies compounding on re-report, and align the TS guess report by calvinp0 · Pull Request #988 · ReactionMechanismGenerator/ARC

calvinp0 · 2026-08-16T17:08:31Z

Three defects in how ARC records, computes and reports TS guess execution times and relative energies. They are in one PR because they meet in the same two files, and the second cannot be fixed properly without the first.

1. Every TS guess's execution time is zeroed on restart

arc/common.py::timedelta_from_str() documents itself as the inverse of str(datetime.timedelta). It is not. It implements a '1hr2m3s' grammar:

regex = re.compile(r'((?P<hours>\d+?)hr)?((?P<minutes>\d+?)m)?((?P<seconds>\d+?)s)?')

Every group is optional, so the pattern also matches the empty string. re.match on a real duration matches zero characters, produces no groups, and returns timedelta(0). Measured on main:

'0:00:05.357294' -> timedelta(0)      '2 days, 3:00:00' -> timedelta(0)
'0:00:03.420000' -> timedelta(0)      ''                -> timedelta(0)
'0'              -> timedelta(0)      '1hr2m3s'         -> timedelta(seconds=3723)

TSGuess.from_dict is the sole production caller, and it is fed exactly the str(timedelta) form that TSGuess.as_dict and the gcn / goflow / kinbot / rits scripts write. So every restored TS guess reports zero execution time, silently, with no warning and no failed job.

Reproducible from ARC's own shipped test data: arc/testing/restart/5_TS1/restart.yml carries 7× execution_time: '0:00:05.357294', every one of which restores as zero.

Nothing in ARC has ever produced the 1hr2m3s form — the regex arrived with the function and never had a producer.

Why it was never caught. The test asserted only isinstance(result, datetime.timedelta), which timedelta(0) satisfies. It passed indefinitely and looked like coverage.

The fix

Anchored parse of the str(timedelta) grammar first (days, negative days, fractional seconds), with the compact hr/m/s grammar retained as an anchored secondary requiring at least one component. The two grammars are disjoint, so keeping the second costs nothing and avoids removing behaviour from a public helper.

Unparseable input returns None with a logged warning rather than a plausible-looking zero. None rather than raising, because the caller already treats None as first-class and arc/testing/restart/2_restart_rate/restart.yml persists a legacy execution_time: '0' that no duration grammar accepts — raising would abort restart of an otherwise valid project. Exact zero ('0:00:00') stays distinguishable from a parse failure.

2. The successful-TS-guess log block does not line up

Real output from a benchmark run:

TS guess  0 for TS0. Method: heuristics (also: crest), relative energy:    17.95 kJ/mol, guess ex time: 0:00:03.4.
TS guess  3 for TS0. Method: heuristics, relative energy:     1.73 kJ/mol, guess ex time: 0:00:03.4.
TS guess 36 for TS0. Method: autotst, relative energy:    17.94 kJ/mol, guess ex time: 0:00:18.1.

Method: runs 7–24 characters wide, so every later column starts at a different offset.

Two further latent defects in the same statement:

the index format spec hard-coded a width of 2, so an index of 100 or more shifts the line (the run above generated 40+ guesses);
f'{tsg.index:2}' raises TypeError outright when the index is None — and arc/testing/restart/2_restart_rate/restart.yml persists 4× index: null. That is a live crash in the reporting path on main:
```
>>> f'{None:2}'
TypeError: unsupported format string passed to NoneType.__format__
```

The energy width was also fixed at 8, simultaneously wider than real data and able to overflow.

The fix

TS Guess  Method                    Rel. Energy  Guess Time  Img Freq
                                       (kJ/mol)                (cm-1)
--------  ------------------------  -----------  ----------  --------
       0  heuristics (also: crest)         0.00       3.4 s
       3  heuristics                      16.22      16.8 s
      36  autotst                         17.94      18.1 s
     102  xtb_gsm (also: gcn)             35.80       1.1 h   -1204.5
     137  kinbot                         151.31       2.0 d

Column widths are derived from the guesses that are actually reported — those passing success and energy is not None — so a filtered-out guess cannot leave a permanently over-wide column. for TS0. is dropped from every row; it is already in the block header.

The Status column is conditional, appended only when a reported guess carries an error. tsg.errors is written only where success is False, which is exactly the set this block filters out, so an unconditional column would be blank essentially always.

Adds arc/common.py::format_table() and arc/common.py::format_duration().

3. Relative energies compound when a TS is re-reported

determine_most_likely_ts_conformer() converts each guess's absolute energy into a relative one in place, with tsg.energy -= e_min. e_min is computed over every guess that has an energy, but the subtraction is applied only to guesses that are success and energy is not None.

On a repeat invocation for the same label the two sets disagree: the successful guesses already hold relative energies while the unsuccessful ones still hold absolute ones. If the global minimum sits on an unsuccessful guess, e_min stays large and every reported energy shifts again:

guesses at +10 and +20 successful, one at -100 unsuccessful
after call 1   [110.0, 120.0, -100.0]
after call 2   [210.0, 220.0, -100.0]

Repeat invocation is reachable — switch_ts in the same file, and two call sites in arc/job/pipe/pipe_coordinator.py. Where the minimum happens to sit on a successful guess the conversion is idempotent, which is why this has gone unnoticed.

This is pre-existing on main and untouched by the two defects above; it is fixed here because it lives in the same function and the same commit.

The fix

The conversion is made idempotent without moving the reference point, so the energies reported on a first invocation are unchanged and a repeat invocation is a no-op. This constraint rules out the otherwise natural repair of computing e_min over only the successful guesses: that is idempotent by construction, but it silently changes every reported energy whenever the global minimum sits on an unsuccessful guess.

Covered by a test that invokes the report twice with the minimum on an unsuccessful guess — the arrangement under which the bug actually manifests — and by an assertion that first-invocation energies are unchanged, so a reference-point shift cannot pass unnoticed.

Why these are one PR

format_duration renders a duration for the table. Its input is the same str(timedelta) form that defect 1 could not parse, so on main it would have to carry its own private copy of that grammar — and a duplicate parser for one grammar in one module is how two copies drift. format_duration therefore delegates to the corrected timedelta_from_str, which is only possible once defect 1 is fixed.

An earlier iteration of this work did carry both parsers, and they had already diverged before merging: the formatter's (?P<days>\d+) rejected '-1 day, 23:59:59', which the parser accepted.

Defect 3 is in the same function as defect 2 and therefore the same commit.

Commits are split by concern so they can be read separately — the parser fix and helpers, then the report, then the restart regression test. Each file is touched by exactly one commit.

A note on the duration format

An earlier revision of this branch rendered durations as DD:HH:MM, rounded up to the minute. That collapsed every distinct timing in the source log to 00:00:01 — heuristics at 3.4 s and AutoTST at 18.1 s became the same cell, erasing a 5× per-method cost difference. Most TS guesses are sub-minute, so this was the common case rather than an edge case.

The current format uses the largest unit the duration fills, to one decimal (3.4 s, 16.8 s, 1.1 h, 2.0 d), so sub-second resolution survives and every cell names its own unit.

Checks

Every new and changed test proved to fail against main before the fix: the restart round-trip, the str(timedelta) round-trip, the table cases, format_duration, the "both entry points read a string identically" test, and the repeat-invocation energy test.

Parity, same command and environment, per touched file:

file	`main`	this branch
`arc/common_test.py`	60 passed, 0 failed	73 passed, 0 failed
`arc/scheduler_test.py`	44 passed, 0 failed	50 passed, 0 failed
`arc/species/species_test.py`	86 passed, 0 failed	87 passed, 0 failed

Full suite on the branch: 2765 passed, 36 skipped, 5 failed — the 5 are torch_ani_test.py and fail identically on main.

The delegation was checked at the seam rather than assumed: '0:00:00' renders 0.0 s while unparseable input renders '', so exact zero stays distinguishable from a parse failure. Fractional seconds are right-padded, so '0:00:03.4' is 3.4 s and not 3 s plus 4 microseconds. A brute-force sweep of 90,271 durations found no value rendered at or above its own unit limit.
One deliberate narrowing: format_duration('100:00:00') returns '', because the anchored grammar bounds hours at two digits. str(timedelta) never emits hours >= 24 — it splits days off. An earlier revision of this branch accepted it; main has no such function.
Two of the tests replaced here were of the kind that cannot fail for the right reason — one asserted a return type, the other an output format. Both are now assertions on the property the value carries.

Known and deliberately not addressed

time_lapse() renders '1.0 days, 06:00:00' for durations over a day, which the newly anchored parser rejects. Nothing feeds time_lapse output into timedelta_from_str, so this is latent; it is noted rather than fixed to keep this PR bounded.
format_table() measures width with len(), so a cell containing full-width or combining characters would misalign. The only cell that could carry one is Status, and main's prose line had the same property.

codecov · 2026-08-16T17:47:10Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.64%. Comparing base (45d73a0) to head (7b1ac74).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #988      +/-   ##
==========================================
+ Coverage   64.60%   64.64%   +0.03%     
==========================================
  Files         119      119              
  Lines       39785    39830      +45     
  Branches    10307    10322      +15     
==========================================
+ Hits        25703    25748      +45     
- Misses      11105    11111       +6     
+ Partials     2977     2971       -6

Flag	Coverage Δ
functionaltests	`64.64% <ø> (+0.03%)`	⬆️
unittests	`64.64% <ø> (+0.03%)`	⬆️

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.

Copilot

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

…ion time timedelta_from_str() claimed to invert str(datetime.timedelta) but implemented a '1hr2m3s' grammar instead. Every group in that regex was optional, so the pattern also matched the empty string: any input in the real str(timedelta) form matched zero characters and the function returned timedelta(0). TSGuess.from_dict() is the only caller, and it feeds it exactly that form - TSGuess.as_dict() writes str(self.execution_time), and the gcn, goflow, kinbot and rits scripts all write str(datetime.datetime.now() - t0). So every TS guess's execution time silently became zero on restart. ARC's own restart fixture arc/testing/restart/5_TS1/restart.yml stores '0:00:05.357294' for seven heuristics guesses, all of which restored as zero. TS guess cost per method is a reported benchmark quantity, so this corrupted data rather than only a log line. Both grammars are kept. Nothing in the repository or its history produces or persists the '1hr2m3s' form - the regex arrived with the function in bd168da and never had a producer - but the two grammars are disjoint (one uses colons, the other letters), so accepting both costs nothing and avoids silently removing behaviour from a public helper in arc/common.py. The str(timedelta) grammar is tried first and is anchored at both ends, which is what stops the empty match. Unparseable input now returns None with a logged warning rather than raising. The caller assigns the result straight to TSGuess.execution_time, where None is already a first-class value - as_dict() guards it with `is not None` and scheduler.py str()s it - whereas raising would abort restart of an otherwise valid project. That is not hypothetical: arc/testing/restart/2_restart_rate/ restart.yml persists a legacy execution_time of '0', which no duration grammar accepts. The warning is what keeps the failure visible; returning a plausible-looking zero is what hid this bug. An exact zero stays distinguishable from a parse failure: '0:00:00' parses to timedelta(0), unparseable input yields None. The existing test asserted only isinstance(result, datetime.timedelta), which timedelta(0) satisfies, so it passed forever while enshrining the bug. It now asserts parsed values, and a round-trip test covers sub-second, multi-day, zero and negative durations. Two rendering helpers are added here as well, for the TS guess report that the following commit rewrites as a table. format_table() sizes each column to its own widest entry, header or cell, and supports a multi-line header so a units line can sit under a title. It validates its inputs and raises InputError for a ragged row, an alignment character other than '<', '>' and '^', and a non-string cell, rather than letting those surface as a TypeError or a ValueError from inside the renderer; a column whose multi-line header is empty and which has no rows now renders at width zero instead of raising from max() on an empty sequence. Its docstring states that widths are counted in characters, which is the contract a caller needs in order to know that alignment holds for single-width text; no display-width measurement is implemented, because the only cell that could carry a full-width or combining character is Status, built from tsg.errors, which is ASCII as an ESS emits it. format_duration() reports a duration in the largest unit it fills, to one decimal place ('3.4 s', '47.2 m', '13.1 h', '2.1 d'). Most TS guesses finish in seconds and per-method guess cost is a reported benchmark metric, so the report has to resolve a 3.4 s heuristics guess from an 18.1 s AutoTST one, which a whole-minute format cannot. One decimal place in a self-naming unit keeps that resolution and spans seconds to days while staying short enough to sit in a column without a unit line of its own. The unit loop covers the three bounded units and days are the terminal return after it, rather than a fourth entry whose limit is a None sentinel: the sentinel entry always matched, so the function could only return from inside the loop even though it is annotated `-> str`, which is what CodeQL's py/mixed-returns flags. Structuring it as "three bounded units, then days as the unbounded fallback" removes the implicit fall-through without leaving a line after the loop that can never execute. time_lapse() is corrected in the same file. It computes its day count with divmod() on a float, so `str(d)` rendered it as '1.0' and the function returned '1.0 days, 06:00:00' - which matches neither the "D HH:MM:SS" format its own docstring claims nor Python's own str(timedelta). Formatting the count with '.0f' makes it a whole number. Sub-day output is untouched. This also removes an inconsistency inside the module this commit restructures: arc/common.py now holds one duration parser, and timedelta_from_str(), newly anchored, rejected the day form that time_lapse() produced. Nothing feeds one into the other today, so the defect was latent, which is also why no test caught it - there was no coverage above 24 hours at all. There is now, and it asserts the round trip through timedelta_from_str() as well as the rendered string. Searched arc/common.py, arc/plotter.py and the rest of arc/ for an existing column, table or padding helper (def .*(pad|align|column|width|table|format), ljust/rjust/center, tabulate/PrettyTable, "max(len(") and for a duration formatter (time_lapse, timedelta_from_str, convert_to_hours). No table helper exists; the only comparable code is the Arkane thermo table in arc/statmech/arkane.py, which hand-rolls its widths inline. Rather than add a second copy of that, the renderer goes in arc/common.py, which is where the duration formatter belongs too. arkane.py is left alone because converting it would change thermo log output this change is not otherwise concerned with; it is the obvious next caller. format_duration() does not carry a duration grammar of its own. It delegates the string case to timedelta_from_str(), so the module holds exactly one parser for the str(timedelta) form and the two cannot drift apart - which they already had: a parser written privately against the broken timedelta_from_str() allowed no sign on the days field and so failed on '-1 day, 23:59:59', which timedelta_from_str() reads. Delegating is only correct once timedelta_from_str() is fixed; against the old implementation every colon-form duration would have rendered as '0.0 s'. The contract seam is preserved in both directions. timedelta_from_str() returns None for unparseable input and format_duration() returns '' for it, so the composition still yields ''. An exact zero stays distinguishable from a failure: '0:00:00' renders '0.0 s', garbage renders ''. Negative durations now parse, where the private regex rejected them outright, and format_duration() still reports them as '' - a negative guess duration is not worth displaying, and that was the behaviour before. An empty or blank string is treated as an absent duration and short-circuits before the parser, so a missing execution time does not log a parse warning once per report row; a non-blank string that is genuinely malformed still warns, which is the signal worth keeping.

The per-guess lines logged by determine_most_likely_ts_conformer() were prose with embedded labels, and they did not line up: three of the four fields ahead of the free-text tail were rendered at their natural width. The method string is 7-24 characters, longer still when a clustered guess adds an "(also: ...)" suffix; the execution time varies; and the index format spec hard-coded a width of 2, so an index of 100 or more shifted the whole line. Only the relative energy was padded, at a fixed width of 8 that was both wider than the data needs and able to overflow. The block is now a table with a header, a rule, and one row per guess: TS Guess, Method, Rel. Energy (kJ/mol), Guess Time, Img Freq (cm-1), and Status. Each column is sized to its own widest entry, computed over the guesses that are actually reported, i.e. those that pass the "success and energy is not None" filter, so a guess that is filtered out cannot leave a permanently over-wide column. Sizing them needs the rendered cells before the first line is emitted, so the guesses are collected in the existing loop, which already mutates tsg.energy into a relative energy, and logged in a second pass. The whole table is emitted before any structure is drawn. plotter.draw_3d() opens with an unconditional logger.debug('not drawing 3D!'), so drawing each guess right after its own row put that line between the rows and destroyed the alignment whenever ARC runs at DEBUG - and verbose is a documented user-facing ARC input, persisted to the restart dict when it is not INFO, so DEBUG is a supported mode rather than a developer-only one. Splitting the loop keeps the table contiguous. The draws themselves are unchanged: one per reported guess, in row order, still with method='draw_3d', because for a TS only draw_3d() may be used - show_sticks() infers connectivity and gets it wrong for a structure with partial bonds. The guess time cell comes from format_duration(), which reports the duration in the largest unit it fills to one decimal place. Most TS guesses finish in seconds and per-method guess cost is a reported benchmark metric, so the column has to resolve a 3.4 s heuristics guess from an 18.1 s AutoTST one; the previous code truncated str(tsg.execution_time) at one decimal place, which was legible only because it happened to be sub-minute, and a whole-minute format would collapse every row of a real block to one value. The Status column carries tsg.errors, which was previously appended to the end of the prose line, and is added only when a reported guess actually has an error: errors are recorded exclusively on guesses with success False (see the loop at the end of troubleshoot_ess), which this block filters out, so the column would otherwise always be blank. The imaginary frequencies, previously spelled out mid-sentence, are now just the Img Freq column. The relative-energy conversion is also made idempotent, which is a pre-existing defect this commit fixes rather than one it introduces - the same subtraction, with the same asymmetry, is on main. e_min is the lowest energy of ANY guess, successful or not, but `tsg.energy -= e_min` was applied only to the successful ones. A second invocation for the same label therefore saw a set in which the successful guesses already held relative energies while the unsuccessful ones still held absolute ones; if the global minimum sat on an unsuccessful guess, e_min stayed at that absolute value and every successful energy shifted again. Measured on main, for guesses at +10 and +20 that succeeded and one at -100 that did not: [110.0, 120.0, -100.0] after the first call, [210.0, 220.0, -100.0] after the second. Repeat invocation is reachable from switch_ts() and from two call sites in arc/job/pipe/pipe_coordinator.py. The subtraction is now applied to every guess that has an energy. This keeps the reference point exactly where it was - the lowest energy of any guess - so no energy reported on a first invocation changes, which matters because those numbers are physical and feed the selection. It also makes the set minimum exactly zero, so a second invocation subtracts zero and is a no-op. Computing e_min over only the successful guesses would have been idempotent too, but it moves the reference point whenever the global minimum sits on an unsuccessful guess, and so changes reported energies; that was rejected for exactly that reason. The values newly mutated are the energies of unsuccessful guesses. Two things read them. plotter.save_conformers_file() takes the energies of all guesses and re-baselines them against their own minimum, so a uniform offset leaves its output unchanged; it was previously handed a mixture of relative and absolute values, and now receives one consistent baseline. TSGuess.almost_equal_tsgs() compares two energies with isclose(abs_tol=0.1), a difference, which a uniform offset also leaves unchanged. as_dict() persists the energy, so a restart now restores an already-relative set whose minimum is zero, on which the conversion is again a no-op.

TSGuess.from_dict() parsed ts_dict['execution_time'] near the top and then, for a 'user guess' method, overwrote the result with timedelta(seconds=0) at the bottom. Now that an unparseable duration is reported rather than silently read as zero, that ordering makes ARC warn about values it throws away: loading arc/testing/restart/2_restart_rate/restart.yml, whose four TS guesses are all 'user guess' entries carrying a legacy execution_time of '0', emitted three "Could not interpret '0' as a time delta" warnings per restart for a field whose restored value cannot depend on it. The method is now resolved before the execution time, and the execution time is not parsed at all when the method is a user guess, since the later branch is the authority on that value. Nothing else moves, and the warning is untouched for every guess whose execution time is actually kept - an unparseable duration on a kinbot or heuristics guess is a real loss of benchmark data and still says so. TSGuess.as_dict() writes str(self.execution_time) and from_dict() reads it back, so that pair is the path on which every TS guess's execution time was silently reset to zero on restart. Nothing tested the pair itself: the parser had a unit test that asserted only isinstance(result, datetime.timedelta), which timedelta(0) satisfies, so the defect survived a green suite. The new TestTSGuess cases round-trip a sub-second duration, a multi-day one and an exact zero through as_dict()/from_dict() and assert the restored value equals the original, load the restart fixture and assert it produces no warning, and assert that a non-user-guess entry with the same unparseable value still warns. That pins the behaviour at the level a restart actually exercises rather than at the level of the regex.

github-actions Bot added the Module: Scheduler label Aug 16, 2026

github-advanced-security Bot found potential problems Aug 16, 2026

View reviewed changes

Comment thread arc/common.py Fixed

calvinp0 changed the title ~~Fix TS guess execution times zeroing on restart, and align the TS guess report~~ Fix TS guess execution times zeroing on restart, relative energies compounding on re-report, and align the TS guess report Aug 16, 2026

calvinp0 force-pushed the fix_ts_guess_time_reporting branch from 2320126 to e41a070 Compare August 16, 2026 18:02

github-actions Bot added the Module: Species label Aug 16, 2026

calvinp0 force-pushed the fix_ts_guess_time_reporting branch 2 times, most recently from d52b1c5 to 5f281e5 Compare August 22, 2026 06:45

calvinp0 marked this pull request as ready for review August 22, 2026 08:53

Copilot AI lite review requested due to automatic review settings August 22, 2026 08:53

Copilot AI reviewed Aug 22, 2026

calvinp0 force-pushed the fix_ts_guess_time_reporting branch from 5f281e5 to f806aab Compare August 23, 2026 08:55

calvinp0 added 3 commits August 23, 2026 15:13

calvinp0 force-pushed the fix_ts_guess_time_reporting branch from f806aab to 7b1ac74 Compare August 23, 2026 12:14

Navigation Menu

Uh oh!

Fix TS guess execution times zeroing on restart, relative energies compounding on re-report, and align the TS guess report - #988

Fix TS guess execution times zeroing on restart, relative energies compounding on re-report, and align the TS guess report#988
calvinp0 wants to merge 3 commits into
mainfrom
fix_ts_guess_time_reporting

calvinp0 commented Aug 16, 2026 •

edited

Loading

Uh oh!

Uh oh!

codecov Bot commented Aug 16, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

calvinp0 commented Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Every TS guess's execution time is zeroed on restart

The fix

2. The successful-TS-guess log block does not line up

The fix

3. Relative energies compound when a TS is re-reported

The fix

Why these are one PR

A note on the duration format

Checks

Known and deliberately not addressed

Uh oh!

Uh oh!

codecov Bot commented Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

calvinp0 commented Aug 16, 2026 •

edited

Loading

codecov Bot commented Aug 16, 2026 •

edited

Loading