Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The change is a straightforward, internally consistent data correction with valid YAML structure and updated provenance.
Pull request overview
This PR updates ARC’s frequency scale-factor reference data to reflect G4’s method-defined ZPE scaling constant, improving consistency with how other composite methods (e.g., CBS-QB3) are represented in the table.
Changes:
- Updates the
g4, software: gaussianfactor from0.994to0.9992(i.e.,0.9854 * 1.014). - Replaces the G4 entry’s provenance to cite the G4 paper and adds it as
source: 6. - Updates the note to reflect the method-defined ZPE scale factor basis.
File summaries
| File | Description |
|---|---|
data/freq_scale_factors.yml |
Corrects the G4 scale factor value and cites the primary G4 reference in the sources table. |
Review details
- Files reviewed: 1/1 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 Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1039 +/- ##
==========================================
- Coverage 66.29% 66.23% -0.07%
==========================================
Files 122 122
Lines 41825 41834 +9
Branches 10749 10750 +1
==========================================
- Hits 27729 27707 -22
- Misses 11053 11072 +19
- Partials 3043 3055 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
a29d6a3 to
c8a81c8
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Four moderate findings remain, including missing G4 regression/integration coverage and an unresolved scan-frequency mismatch.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
arc/main.py:1055
- Please add an integration assertion that constructing
ARC(..., composite_method='g4')producesfreq_level.simple() == 'b3lyp/6-31g(2df,p)'. The new tests exercise the resolver directly, while the existing ARC-level tests cover only CBS-QB3, so a break in this call path would go undetected.
self.freq_level = get_freq_level_for_composite_method(self.composite_method)
arc/settings/settings.py:247
- The new helper tests replace
freq_for_compositewith a hand-written mapping, so they never exercise this production default. A typo or omission in the G4 entry here would still leave the suite green; add an assertion against the repository defaults or anARC(..., composite_method='g4')integration test that verifiesfreq_levelresolves tob3lyp/6-31g(2df,p)while keeping the overlay-independent helper tests.
'freq_for_composite': {'cbs-qb3': 'B3LYP/CBSB7',
'cbs-qb3-paraskevas': 'B3LYP/CBSB7',
'g4': 'B3LYP/6-31G(2df,p)', # G4's prescribed level
},
arc/settings/settings.py:246
- Adding the G4-specific frequency level leaves the composite rotor-scan default at
B3LYP/CBSB7(main.py:1064) while G4 frequencies now useB3LYP/6-31G(2df,p). A default G4 run therefore uses different levels for scans and frequencies, contrary to the project's stated scan/frequency pairing used to project out rotors. Please make the scan default per-composite as well, or otherwise explicitly handle this mismatch before making the G4 default active.
'g4': 'B3LYP/6-31G(2df,p)', # G4's prescribed level
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
| factor: 0.9992 | ||
| source: 6 | ||
| note: '0.9854 * 1.014, the 0.9854 value is the ZPE scale factor of G4' |
c8a81c8 to
8a3fecf
Compare
…ale factor, and pair each composite with the frequency level it prescribes
The G4 entry added in #1004 carries 0.994, derived as 0.980 * 1.014 where 0.980 was refitted with Truhlar's method over the 15-species standard set. G4 is a composite method that defines its own ZPE scale factor: 0.9854, applied to the B3LYP/6-31G(2df,p) frequencies the method prescribes (Curtiss, Redfern and Raghavachari 2007). Refitting it produces a number that disagrees with the method's own definition. Use the method's factor in the field's units: 0.9854 * 1.014 = 0.9992. This also restores consistency with the neighbouring composite entry. cbs-qb3 stores its own method-defined ZPE factor the same way (0.99 * 1.014), so a refit for G4 alone made the two entries mean different things. Adds source 6 for the G4 paper; no existing source id covered it. Effect: relative to no entry at all, the ZPE shift is 0.02-0.10 kcal/mol. Relative to the 0.994 this replaces, values computed with the old entry are low by 0.0059 * ZPE_harm -- 0.15 kcal/mol for a small species, 0.6+ kcal/mol at ZPE ~ 120 kcal/mol. That correction is closed-form per species, so data already computed against 0.994 is recoverable without a re-run.
The frequency scale factor stored for a composite method in
data/freq_scale_factors.yml is that protocol's OWN prescribed ZPE scale
factor times 1.014 -- the notes say so explicitly:
cbs-qb3: '0.99 * 1.014, the 0.99 value is the ZPE scale factor of CBS-QB3'
g4: '0.9854 * 1.014, the 0.9854 value is the ZPE scale factor of G4'
But default_levels_of_theory['freq_for_composite'] was a single global
'B3LYP/CBSB7' applied to every composite, so for G4 ARC scaled CBSB7
frequencies by a factor Curtiss defines against B3LYP/6-31G(2df,p).
Keying the factor lookup on the composite method is therefore correct and
is left alone; what was wrong is the frequency level fed to it. This makes
freq_for_composite a {composite: level} mapping, so the pairing is right by
construction, and adds get_freq_level_for_composite_method() to resolve it.
An unmapped composite now raises rather than defaulting. The failure it
replaces is undetectable from the job's own output -- a mispaired factor
still converges, terminates normally and yields a plausible enthalpy -- so
a loud stop is worth more than a guess. `freq_level` in the input file
remains the override, and the error message says so.
A local ~/.arc/settings.py overriding default_levels_of_theory predates the
mapping and still holds a string; that is honored with a warning naming the
mispairing risk, rather than crashing the user's run.
Tests patch the mapping explicitly instead of reading it live, since
arc/imports.py overlays ~/.arc/settings.py over the repo defaults -- an
unpatched test would assert against whatever the developer's machine holds.
8a3fecf to
d3e988a
Compare
What
Two related corrections to how ARC scales harmonic frequencies for composite methods.
1. The G4 value was wrong.
data/freq_scale_factors.ymlcarried0.994, derived as0.980 * 1.014with the 0.980 fitted by Truhlar's method over the 15-species standard set. G4's prescribed ZPE scale factor is Curtiss's 0.9854, giving 0.9992.2. The value was being applied to the wrong frequencies. The factor stored for a composite is that protocol's own prescribed ZPE scale factor x 1.014 -- the existing notes say so:
But
default_levels_of_theory['freq_for_composite']was a single global'B3LYP/CBSB7'for every composite, while G4 prescribes B3LYP/6-31G(2df,p). So ARC scaled CBSB7 frequencies with a constant defined against a different basis.Keying the factor lookup on the composite method (
main.py) is therefore correct by design and is left untouched -- the defect is the frequency level handed to it.freq_for_compositebecomes a{composite: level}mapping, resolved by the newget_freq_level_for_composite_method().Behaviour change worth review
An unmapped composite now raises instead of defaulting to CBSB7. This affects the 14 composites ARC recognises that have no scale-factor entry (
cbs-4m,g3,w1bd, ...). The rationale: the failure it replaces cannot be seen from the output of the job it corrupts -- a mispaired factor still converges, terminates normally, passes ARC's sanity checks, and yields a plausible enthalpy for the wrong quantity. A loud stop beats a silent guess. Settingfreq_levelexplicitly in the input file remains the override, and the error message says so.Happy to soften this to a warning-and-default if maintainers prefer; it is the one judgment call here.
Compatibility
A local
~/.arc/settings.pythat overridesdefault_levels_of_theorypredates the mapping and still holds a string. That is honoured with a warning naming the mispairing risk, rather than crashing the user's run.Testing
New cases in
arc/level_test.py: G4 ->B3LYP/6-31G(2df,p), CBS-QB3 ->B3LYP/CBSB7unchanged,Leveland case-insensitive input, unmapped composites raise, and the legacy-string path warns and proceeds. They patch the mapping explicitly rather than reading it live, becausearc/imports.pyoverlays~/.arc/settings.pyover the repo defaults -- an unpatched test asserts against whatever the developer's machine happens to carry.Note for reviewers
scan_for_composite,irc_for_compositeandorbitals_for_compositehave the same single-global shape and arguably the same defect --'scan''s own comment says it "should be the same level as freq (to project out rotors)", which no longer holds for G4 once freq moves. Deliberately left out of scope here to keep this reviewable; happy to follow up.