Skip to content

Stop a short method-type marker from matching inside an unrelated method name - #1050

Open
alongd wants to merge 1 commit into
mainfrom
method-type-marker-matching
Open

Stop a short method-type marker from matching inside an unrelated method name#1050
alongd wants to merge 1 commit into
mainfrom
method-type-marker-matching

Conversation

@alongd

@alongd alongd commented Sep 8, 2026

Copy link
Copy Markdown
Member

Fixes #1049.

Level.deduce_method_type() matches its markers as raw substrings, so 'am' fires inside every Coulomb-attenuated functional. cam-b3lyp and camb3lyp are both registered in data/ess_methods.yml, so an ordinary input file reaches this.

The type then decides how the job is written, and every consequence points the same way:

  • is_species_restricted() early-returns True for a semi-empirical method before it reads the multiplicity, so a radical ran restricted.
  • The Gaussian adapter gates its optimization keywords on the type, so a TS optimization emitted a bare opt — no ts, no calcfc, no noeigentest — and searched for a minimum instead of a saddle point.
  • fine silently dropped its integral=(grid=ultrafine) / scf=(tight,direct) block.
  • A dispersion correction on such a level raised ValueError.
  • Orca did not get its KS method class or DFT grid, and deduce_software() used the wrong ESS preference order.

Generated Gaussian route lines, same species and settings, before this PR:

b3lyp      #P opt=(calcfc,maxcycle=100,maxstep=5,tight)  guess=mix ub3lyp/def2tzvp  integral=(grid=ultrafine, Acc2E=12) ... scf=(direct,tight)
cam-b3lyp  #P opt  guess=mix cam-b3lyp/def2tzvp

b3lyp      #P opt=(calcfc,maxcycle=100,maxstep=5,noeigentest,tight,ts)  guess=mix b3lyp/def2tzvp  integral=(grid=ultrafine, Acc2E=12) ... scf=(direct,tight)   <- TS
cam-b3lyp  #P opt  guess=mix cam-b3lyp/def2tzvp                                                                                                              <- TS

and after:

cam-b3lyp  #P opt=(calcfc,maxcycle=100,maxstep=5,tight)  guess=mix ucam-b3lyp/def2tzvp  integral=(grid=ultrafine, Acc2E=12) ... scf=(direct,tight)
cam-b3lyp  #P opt=(calcfc,maxcycle=100,maxstep=5,noeigentest,tight,ts)  guess=mix cam-b3lyp/def2tzvp  integral=(grid=ultrafine, Acc2E=12) ... scf=(direct,tight)   <- TS

The change

Two lines of it, for two independent problems.

'am' and 'pm' are abbreviations of the methods they stand for, so they are spelled out: am1, pm3, pm6, pm7.

The wave function markers are the shortest and most promiscuous of the four sets (cc, ci, ri, ic, mr, bd, cp) and were matched first. Matching them last lets the longer, more specific force field and semi-empirical names win. That also repairs amber (typed semi-empirical, same 'am') and ghemical (typed as a wave function method, for the 'ic' in ghemICAl).

Matching on token boundaries instead was tried and rejected. It looks like the principled fix and it re-types six registered methods that carry their marker mid-token: BCCD, BCCD(T), QCISD, QCISD(T), FCI, LCCD, LCCSD, LT-DF-LCC2 all become dft, and torchani loses force_field. That would have been a worse bug than the one being fixed, and the sweep below is what caught it.

Verification

The verifier is exhaustive rather than sampled: every one of the 180 methods registered in data/ess_methods.yml is typed before and after. The complete diff is the four intended corrections and nothing else:

amber      semiempirical  -> force_field
cam-b3lyp  semiempirical  -> dft
camb3lyp   semiempirical  -> dft
ghemical   wavefunction   -> force_field

test_deduce_method_type is new and covers one method per type, the common cases, the false positives being fixed (cam-*, am05), and the mid-token matches that must keep working (bccd(t), qcisd(t), fci, lccsd, lt-df-lcc2, uccsd(t)-f12, rohf). It fails on main and passes here. It also asserts that every registered method types as something, so a future marker edit is checked against the whole registry.

Full unit suite: 3295 passed, 12 skipped (the skips are uma_env and rmg_env guards, unrelated).

Note for #1014

Found while reviewing #1014, whose broken-symmetry adoption gate reads the same typing: before this PR a CAM functional reaches that gate as REFERENCE_AGNOSTIC and is denied a broken-symmetry reference it should be offered. That direction is conservative, it is not a defect in #1014, and this PR corrects it at the source. The two PRs touch disjoint files.

…hod name

`deduce_method_type()` matches its markers as raw substrings, so `'am'` fired
inside every Coulomb-attenuated functional and typed `cam-b3lyp`, `camb3lyp`,
`camh-b3lyp`, `cam-qtp00` and `lc-camb3lyp` as semi-empirical. Both `cam-b3lyp`
and `camb3lyp` are registered in `data/ess_methods.yml`, so an ordinary input
file reaches this.

The type then decides how the job is written. `is_species_restricted()`
early-returns `True` for a semi-empirical method before it reads the
multiplicity, so a radical ran restricted; the Gaussian adapter gates its
optimization keywords on the type, so a TS optimization emitted a bare `opt`
and searched for a minimum instead of a saddle point, and `fine` dropped its
ultrafine grid. A dispersion correction on such a level raised `ValueError`.

Two changes fix it. `'am'` and `'pm'` are abbreviations of the methods they
stand for, so they are spelled out as `am1`, `pm3`, `pm6` and `pm7`. The wave
function markers are the shortest and most promiscuous of the four sets, and
they were matched first; matching them last lets the longer force field and
semi-empirical names win, which also repairs `amber` (semi-empirical, for the
same `'am'`) and `ghemical` (a wave function method, for `'ic'`).

Matching on token boundaries rather than substrings was tried and rejected:
`BCCD`, `QCISD`, `FCI`, `LCCSD`, `LT-DF-LCC2` and `torchani` all carry their
marker mid-token and a token-start rule re-types all six.

Typing all 180 methods registered in `data/ess_methods.yml` before and after
shows exactly four changes, all of them the intended corrections:

    amber      semiempirical  -> force_field
    cam-b3lyp  semiempirical  -> dft
    camb3lyp   semiempirical  -> dft
    ghemical   wavefunction   -> force_field

Fixes #1049

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.

🟢 Approval recommended

The change is narrowly scoped to method-type deduction, and the added tests cover both the reported regressions and broad registry-wide behavior.

Pull request overview

This PR fixes false-positive substring matches in Level.deduce_method_type() that caused short semi-empirical / wavefunction markers (e.g., am, ic) to match inside unrelated method names (e.g., cam-b3lyp, ghemical), leading to incorrect method typing and downstream job-generation errors.

Changes:

  • Replace semi-empirical markers am/pm with explicit method strings (am1, pm3, pm6, pm7) to avoid substring collisions.
  • Reorder marker matching so force-field and semi-empirical matches take precedence over the more promiscuous wavefunction markers.
  • Add a unit test that covers common typing cases, the reported false positives, mid-token wavefunction markers that must remain supported, and a full sweep over registered methods in data/ess_methods.yml.
File summaries
File Description
arc/level.py Adjusts marker lists and matching order in deduce_method_type() to prevent incorrect substring-based typing.
arc/level_test.py Adds test_deduce_method_type to validate correct typing for key cases and across the full registered-method registry.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.58%. Comparing base (484c678) to head (78e8866).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1050      +/-   ##
==========================================
+ Coverage   65.55%   65.58%   +0.02%     
==========================================
  Files         121      121              
  Lines       41073    41073              
  Branches    10566    10566              
==========================================
+ Hits        26927    26936       +9     
+ Misses      11117    11111       -6     
+ Partials     3029     3026       -3     
Flag Coverage Δ
functionaltests 65.58% <ø> (+0.02%) ⬆️
unittests 65.58% <ø> (+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.

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.

deduce_method_type() substring-matches 'am' inside 'cam-b3lyp': every CAM functional types as semi-empirical

2 participants