Repository navigation
Export tool-neutral ARC results and parser evidence, and fix three Molpro defects - #917
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #917 +/- ##
==========================================
+ Coverage 66.29% 67.06% +0.77%
==========================================
Files 122 123 +1
Lines 41825 43597 +1772
Branches 10749 11143 +394
==========================================
+ Hits 27726 29240 +1514
- Misses 11056 11212 +156
- Partials 3043 3145 +102
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:
|
alongd
left a comment
There was a problem hiding this comment.
Thanks for this well-though of contribution! I left some minor comments
| _warn_missing_tckdb_package() | ||
| return | ||
| raise | ||
| from tckdb_arc.adapter import TCKDBAdapter |
There was a problem hiding this comment.
what's tckdb_arc? do we install it in our devscripts? or is it in the env?
There was a problem hiding this comment.
It's for https://github.com/calvinp0/tckdb-adapters/
I added in the devtools installation now but it's still a WIP. Splitting the adapter from ARC has somewhat been a challenge
https://github.com/ReactionMechanismGenerator/ARC/blob/56800ae93a52f93d20d8a1017b20413a5ca6629d/devtools/install_tckdb_arc.sh
45210de to
bb36bbe
Compare
bb36bbe to
bd2a0db
Compare
fd0ea74 to
bc7b020
Compare
alongd
left a comment
There was a problem hiding this comment.
Thanks! added some comments, specifically about parser tests and makefile.
How does CI work with this new installation?
06826b6 to
cbeb153
Compare
f095424 to
1fbdb47
Compare
| from arkane.encorr.data import atom_energies, pbac | ||
|
|
||
| from arkane_levels import lot_from_string | ||
| from common import read_yaml_file, save_yaml_file |
There was a problem hiding this comment.
this imports from ARC? does this imply that we have both Arkane (RMG) and ARC in the same env?
There was a problem hiding this comment.
it's a script so should be subprocess called with rmg env
d822bb1 to
ddc3081
Compare
alongd
left a comment
There was a problem hiding this comment.
Another review pass at ddc3081 (testing/mutation, schema contract, security, plus a cross-model adversarial pass). The test suite held up well: 9 targeted mutations (Molpro fixes, _get_torsions scan-key guard, Hessian source flag, parent-dir fsync, PBAC gate, ORCA Hessian values) were all caught, and the changed modules pass (655 tests). Previous-round points (duplicate doc rows, AEC/BAC unit sourcing, parent-dir fsync) are resolved.
Points to fix before merge. Each of these was reproduced by running the code:
-
Broken test for the
cp_data→thermo_pointsrename.arc/scripts/save_arkane_thermo_test.py:156still readscontent['P1']['cp_data']. The file isn't touched by this PR. Running it underrmg_envgivesKeyError: 'cp_data'. CI doesn't catch it because this test skips inarc_envand no workflow runs it inrmg_env. -
Gaussian constraint parser loses the target of the constraints ARC itself writes.
arc/job/adapters/gaussian.py:393emits two lines per constraint:B 1 2 =1.45 B, thenB 1 2 F._parse_gaussian_constraint_linereturnsNonefor the first line (Baction). It returnstarget_value: Nonefor the second:'B 1 2 =1.45 B' -> None 'B 1 2 F' -> {'coordinate_type': 'distance', 'atom_indices': [1, 2], 'target_value': None, ...}Carrying the value from a preceding
=x Bline on the same atoms into the matchingFwould fix it. Please add a test that feeds the parser exactly what the adapter writes. -
IRC reaction coordinate sign.
docs/output_yml_schema.md(~L250) saysreaction_coordinate_sqrt_amu_bohris "signed about the TS".parse_irc_pathcopies Gaussian's cumulative value unchanged, though, so the reverse branch is positive too:rxn_1_irc_2.outgives('reverse', 1, 0.07236), ('reverse', 2, 0.14469), .... Either negate fordirection == 'reverse'or change the doc to say it's unsigned and to usedirection.
Smaller points:
- TCKDB URL logged verbatim (
ARC.py~L64). If the URL carries userinfo (https://user:token@host), it lands inarc.log. Logging only the hostname would avoid that. - Where does
tckdb_arccome from? Commit d5353ca's message mentionsdevtools/install_tckdb_arc.shand amake install-tckdb-arctarget, but neither is in the commit. The warning text points users to a "tckdb-arc package", which invites a guessedpip install tckdb-arc. Please name a pinned, canonical install source, or say explicitly that it's not on PyPI. Also, the adapter call shape is tested only against mocks. _make_rel_path(arc/output.py~L514) returns things like../../../home/<user>/...for logs outside the project directory. That leaks the local directory layout into the exports meant for sharing. Consider emittingnull, or the basename plus an "outside project" flag.parse_1d_scan_full_resultdocstring says it never raises, but it raisesTypeErroron an empty log andFileNotFoundErroron a missing path. The only caller wraps it intry, so either fix the docstring or add the guard.- Silent skip on restart. The
tckdbblock is popped beforeARC()is built, so it never reachesrestart.yml, and a restarted project silently uploads nothing. A one-line INFO log or a doc note would help.
Worth checking. These came from the adversarial pass and were not reproduced:
arc/output.py~L205 computes per-species corrections from the globalbac_type. On a kinetics-only run (the kinetics adapter usesbac_type=None),output.ymlmight report a BAC that Arkane never applied.- GSM: if xTB-GSM writes all-zero stringfile energies to mean "no energy",
parser_evidence.py~L544 exports them as a flat 0 kcal/mol profile. _read_irc_geometry_tabledoesn't require the closing separator. A log truncated mid-table would export a point with only some of its atoms.- Gaussian IRC point energies come only from
SCF Done. For an MP2 or double-hybrid IRC,electronic_energy_hartreewould then be the SCF component.
Add Cartesian Hessian lower-triangle parsers for Gaussian, Orca, and QChem, a structured Gaussian IRC-path parser emitting one record per converged point, and GSM stringfile energy parsing, all exposed through the parser dispatch layer. parse_irc_path walks the log once and returns per-point energy, gradients, reaction coordinate, direction, and geometry; the TS seed carries no CURRENT STRUCTURE block and is deliberately not emitted. Both Gaussian IRC parsers read the same kind of Cartesian table, so that walk lives in one _read_irc_geometry_table helper instead of being written twice. parse_irc_traj reaches it from the Point Number line and parse_irc_path from the CURRENT STRUCTURE line; the emitted geometries are unchanged. The helper is bounds-checked throughout, which also removes an IndexError parse_irc_traj could raise on a log truncated immediately after a Point Number block. The Orca 1D-scan parser published column 2 of the "Calculated Surface using the 'Actual Energy'" table unconverted, so a method documented as returning the electronic energy in kJ/mol returned absolute Hartree. On arc/testing/rotor_scans/orca/cc.txt the 0.0181 Hartree spread that is a 47.5 kJ/mol torsional barrier was published as -205.45 to -205.43, and every consumer of the rotor barrier saw it. It now zeroes against the scan minimum and converts, as every other adapter already does, and implements parse_1d_scan_energies_hartree so the absolute curve and its zero reference stay recoverable rather than being the only thing available. Orca also tabulated its scan angles on its own -180-to-180 dihedral origin while Gaussian reports a displacement from the scan's first point, and a caller cannot hold both conventions at once: parse_1d_scan_energies_from_specific_angle adds an initial dihedral to whatever the adapter returns, which double-counts under Orca's convention. The normalisation Gaussian already applied inline is now a shared scan_angles_to_displacement helper that both adapters route through, so the displacement convention is stated once. parse_cartesian_hessian_geometry searched the whole log backwards for the last "Input orientation:" while parse_cartesian_hessian_lower_triangle took the last force-constants block, with nothing tying the two together. In 27 of the 58 Gaussian fixtures carrying a force-constants block the file's last input orientation follows that block; on arc/testing/rotor_scans/CH2OOH.out the two structures are 1.51 A apart, and on arc/testing/composite/C3H7/TS7.log they do not even have the same atom count. The geometry search now starts at the force-constants block and walks backwards, so the pair is anchored by construction, and a Hessian with no input orientation before it yields no geometry rather than a later, unrelated one. Constraint records now carry target_value_units. The value is reported in the source deck's own units and never converted, so a consumer had no way to tell an Angstrom from a Bohr. Gaussian ModRedundant coordinates are Angstrom and degrees; Orca reports degrees for angular constraints and null for lengths, because its '%geom Constraints' block does not echo the unit it was written in and a guess would be worse than a stated unknown.
Add ARC-native Hessian parser tests with an Orca H2O Hessian fixture, scan constraint parser tests, and IRC-path parser coverage including the forward/reverse direction and the failed-log path. Also assert that the two Gaussian IRC parsers agree: parse_irc_traj returns the TS seed followed by exactly the geometries parse_irc_path reports. That is the invariant which lets both share one table reader, so it is pinned rather than left implicit. Extend the Hessian tests to the frame-anchoring invariant. Rigid-body directions built from the geometry preceding the force-constants block are null modes of the mass-weighted Hessian (5.7 cm-1 on CH2OOH.out), while the same directions built from the log's last input orientation come out at 977 cm-1. No size, finiteness or symmetry check on the triangle can see that difference, so it is asserted physically rather than structurally, and it needs no frequencies from the ESS, which those scan logs do not report. The Orca scan-energy assertions enshrined the raw Hartree table as the expected output of a function documented to return kJ/mol; they now assert the relative curve, the absolute curve, and that the two agree through E_h_kJmol, with tolerances rather than byte-exact float goldens.
ddc3081 to
3f21edc
Compare
Consolidate ARC's results into a documented, tool-neutral output.yml: per-job calculation and rotor-scan provenance, thermo points and applied energy corrections, chosen-TS-guess attribution, spin diagnostics, and IRC/NEB/GSM log paths. ARC reports source facts, native indices, and explicit units; consumers own any database-specific enums or nesting. Torsions reference rotor-scan records through a single shared predicate, so a torsion can never name a scan_rotor_N record that was not emitted. _build_rotor_scan_entry is the only place that decides whether a rotor yields a record, so the consumer cannot drift from the producer and leave a dangling reference the way an independent os.path.isfile check did. Correction parameter tables are labelled with the native unit of the table they are read from rather than the per-species total's unit. Atom energies are Hartree and Petersson bond corrections are kcal/mol; these are properties of the Arkane data the tables come from, and are independent of the unit the per-species total happens to be expressed in, so cross-sourcing them would mislabel every parameter should the two ever diverge. Calculation results are emitted flat, with prefixed scalar fields rather than wrapped result objects, and the tests pin that shape along with the field names and units. Downstream uploaders that need a different nesting own the translation, which they can only perform safely against a stable contract. docs/output_yml_schema.md documents the emitted shape. freq_n_imag was a constant rather than a count: a converged TS was recorded as having exactly one imaginary frequency and a converged stable species as having none, whatever the freq job found. check_imaginary_frequencies accepts a TS carrying additional imaginary modes when only one falls in the major band (75-10000 cm-1), so a TS with a residual low-frequency mode was recorded as clean, and a stable species that is not a true minimum was recorded as one. freq_n_imag now counts them, and the frequencies themselves are reported in a new imaginary_frequencies_cm1 list. imag_freq_cm1 keeps its meaning as the most negative mode, and the existing source precedence is unchanged. A scan sample's angle is renamed angle_degrees -> scan_displacement_degrees, and the change is a rename rather than a conversion. The value never was a dihedral: Gaussian's runs 0 to 360 from the scan's first point while coordinate. requested_start is the absolute dihedral of the launch geometry on a 0-360 origin, so a consumer storing one as the other was wrong by the start offset, and requested_end (542.9 in the branch's own fixture) is not even in the sample range. Converting the samples to absolute dihedrals instead was rejected: requested_start is optional - it needs a requested_step_size, which only Gaussian's log echoes, and a computable dihedral - so a converted field would silently change origin depending on whether that offset happened to be available, which is a worse contract than one that is always a displacement. The name now says what the number is, and the docs state the arithmetic that recovers the dihedral from it. The samples table itself is new: docs had field tables for result and result.coordinate but none for result.samples, which is how the ambiguity survived review. conformer_energies items become number|null. scheduler.py pre-allocates the list per conformer and fills it one job at a time, so [None] and [1.2, None, 3.4] are ordinary mid-run states; every species in arc/testing/restart/2_restart_rate/restart.yml failed validation on it. Its unit is also stage-dependent - kcal/mol for force-field screen energies, kJ/mol after conformer optimization - which the schema, the docs and the writer's own comment now all say, the last of which had asserted kJ/mol unconditionally. A failed evidence build now removes any tckdb_evidence.json left by an earlier run. The document omitted the descriptor but the previous sidecar stayed on disk, so a stale pair had nothing to compare document ids against - defeating exactly the mechanism the docs offer for detecting one. output.yml is also fsynced and its directory entry synced, matching the sidecar it is paired with. ts_checks is exported. The contract's headline is transition-state provenance and the verdicts are the provenance, yet E0/e_elect/IRC/freq/NMD appeared nowhere in the schema while irc_converged - which is true once two IRC jobs completed, whatever their endpoints - sat there reading like a verdict. Both are now documented for what they are. kinetics gains T0_k, without which A and n in k = A (T/T0)^n exp(-Ea/RT) cannot be interpreted; it was parsed and dropped. dA is documented as the multiplicative factor Arkane reports (the band is [A/dA, A*dA], not A +/- dA) beside dn and dEa, which are additive - a consumer reading it as additive was off by the difference between +/-1.48 and x/1.48. A_units and Ea_units stay free text because they follow the reaction's molecularity and the fit's own unit, which is now said rather than left to be noticed. irc_log_directions becomes an enum of forward/reverse/null. It is the one field where a wrong value silently exchanges reactant and product. zpe_hartree and sp_energy_hartree now state that the first is unscaled and the second uncorrected, so the gap between sp + zpe and e0_kj_mol is documented rather than puzzling - the same asymmetry the docs already explained for frequencies. The new tests drive the real writer over shipped fixtures rather than hand-constructed dicts: an Orca rotor scan (every other scan case in the suite is Gaussian, and output_test.py mocks the Orca parser out) and a document built from restart.yml. That gap is what let the Orca unit bug and the null conformer energies through a green suite. freq_hessian_method records how the frequency job's Hessian was built, in TCKDB's closed HessianMethod vocabulary (analytic / finite_difference_gradient / finite_difference_energy). A consumer parsing ESS output can only record it where the program says so explicitly, and Gaussian never prints its analytic default, so every Gaussian record had to be judged at the conservative threshold. ARC is the producer and knows what it requested, so it states the method wherever that request plus the ESS's documented behaviour settles it - a Gaussian Freq=Numer or Freq=EnOnly route, the method class under a bare freq, an Orca NumFreq/NumGrad or AnFreq keyword line, the PySCF adapter's multiplicity branch - and null everywhere else, including composite levels, RO-DFT, the undocumented double hybrids, an Orca bare Freq over a method Orca documents no analytic Hessian for, and every other ESS. It is never guessed: a wrong token silently mis-scales a consumer's threshold, which is worse than no token. The constraints' target_value_units is documented as Angstrom for an Orca length in docs/output_yml_schema.md as well, matching the schema and the parser; the prose table had been left saying null. The Gaussian freq keyword is parsed free-format, as Gaussian reads its route: Freq = Numer, Freq =(Numer) and Freq = (NoRaman, Numer) are the same keyword with the same option list as Freq=Numer. The previous pattern required the '=' and the parenthesis to abut the keyword, so a spaced route parsed as a bare freq and a numerical Hessian was reported analytic. The pattern still refuses freqchk, anharmonicfreq and #Pfreq, and still leaves freq bare in front of a following keyword's own parentheses (freq scf=(numer), freq IOp(7/33=1), opt=calcnumerfc freq). A method name that misses the explicit Gaussian tables is now tested against a correlated-method pattern - mp3-mp9, cc, qci, ci, bd, cas and hf, optionally spin prefixed - before the DFT rule, and yields null when it matches. Gaussian's option syntax admits spellings the tables cannot enumerate (mp4(full), MP4(SDTQ,Full), mp4(sdq,full), mp4sdq(full), mp5(full), mp4(fc), mp4(dq,full), mp4(t)) and Level.deduce_method_type types every one of them as DFT, its wavefunction list stopping at mp3, so each was reported analytic - a wrong non-null token, which is the one outcome this field is built to avoid. The pattern is anchored so a DFT functional opening with the same letters is untouched: hfs and hfb (Slater and Becke88 exchange), hf3c, mpw3lyp, mpw2plyp and b2plyp all keep the DFT rule's answer. The Gaussian double-hybrid exclusion matches a substring rather than a whole name, as the Orca branch already did for the same family. An exact-match list caught dsdpbep86 and pbe0dh but not revdsdpbep86, dsd-blyp, pwpb95, b2gpplyp or wb97x-2. Gaussian implements none of them, so no run was mis-reported, but the two branches now answer the same question the same way. b2plyp, b2plypd3 and mpw2plyp, which Gaussian does implement with analytic second derivatives, match none of the substrings and stay analytic. The Orca route scan is bounded. It read every line of the log to collect keyword lines that only ever appear in the echoed input block at the head of the file, and the analytic-Hessian marker search then made a second full pass; production Orca logs run to hundreds of megabytes. The route scan now stops at the ****END OF INPUT**** line that closes the echoed block, and in any case after a few thousand lines, and the marker search - which only runs when no keyword line was found at all - is capped as well. The PySCF branch reads the multiplicity through arc.common.is_str_int rather than calling int() on it. Nothing between _get_freq_hessian_method and main.py catches the ValueError a malformed multiplicity would have raised, so one bad species would have cost the whole output.yml; it now contributes a null field. _parse_calc_constraints resolves a relative log_path against project_directory, where it previously resolved it against the process's working directory. ARC stores absolute paths in output_dict['paths'], so this changes no run in practice, but it makes the log follow the same rule as the input deck beside it and as every other path this module reads.
Build a deterministic, tool-neutral evidence document from output.yml plus the parser layer: Hessians, IRC trajectories, and GSM paths, each reported by kind with an explicit availability status and reason. Paths are run-relative and operational fields (servers, job ids, credentials) are excluded. The sidecar is written atomically: contents are fsynced, replaced into place, and the parent directory is then synced so a completed rename survives a crash. Platforms that cannot sync a directory log and skip rather than failing a write that already succeeded. _fsync_directory becomes fsync_directory: the output.yml writer needs the same durability guarantee as the sidecar, so the helper now has two callers rather than one.
ARC no longer implements TCKDB upload itself; it hands results to the standalone tckdb_arc adapter from https://github.com/calvinp0/tckdb-adapters. That project is not published on PyPI, so the missing-adapter warning names the repository it comes from and says so outright: a warning that points at a "tckdb-arc package" invites a guessed pip install that would fetch something unrelated. The hook is optional: a missing adapter logs one warning per process and continues, while a broken install re-raises rather than being misreported as absent. The upload hook could end a finished run non-zero. run_tckdb_upload called .get on whatever the input file put under its 'tckdb' key, so the obvious mistake of writing 'tckdb: true' raised AttributeError, and ARC.py called the hook unguarded, so an import error inside the adapter or a ConnectionError from the endpoint propagated out of main after the results were already on disk. The settings value is validated as a mapping, and the call is wrapped so an upload failure is logged against the project directory the results are in rather than replacing the run's exit status. The resolved destination is logged before any data leaves the machine, parsed with urllib.parse and reduced to its host and port. A URL written with userinfo carries a credential, and arc.log is copied into issue reports and shared project directories, so logging the endpoint verbatim would carry the token with it. The 'tckdb' block is consumed before ARC is constructed, because ARC neither accepts nor records it. That is what keeps any credential it carries out of restart.yml, and it also means a run restarted from restart.yml uploads nothing -- so the drop is announced at INFO rather than left to look like a failure with no trace.
Retain the stringfile and per-node outputs the GSM run produces so the evidence layer can report the path and its energies, and transfer queued artifacts back from the job directory. Isolate the tests across xdist workers so they no longer share a scratch directory.
Record which TS-search method produced the selected guess and keep the NEB and GSM log paths associated with it, so the result contract can attribute a TS to its originating adapter rather than inferring it after the fact.
Surface the per-species spin information the result contract reports, so consumers receive ARC's own diagnosis rather than re-deriving it from geometry and multiplicity.
Extract the atom-energy and bond-additivity corrections Arkane actually applied, along with the thermo points, through helper scripts run in the RMG environment, so the result contract can report the correction model, total, component decomposition, and the parameter table used.
The Molpro adapter prefixed `u` to the method name for every unrestricted
species. That is right only for the methods whose open-shell program Molpro
spells that way, and the manual gives three different answers.
**The `u` prefix is correct, and is not a UHF request.** This adapter's
template hardcodes a spin-restricted reference, `{hf; wf,spin=N,charge=C;}`,
and `HF`/`RHF` is Molpro's spin-restricted program. Of `UCCSD-F12` the manual
says "Open-shell unrestricted UCCSD-F12 approximations ... Restricted
open-shell Hartree-Fock (RHF) orbitals are used", and of the open-shell CC
programs generally, "In both cases a high-spin RHF reference wavefunction is
used". So `uccsd(t)-f12` after `{hf; wf,spin=2}` is RHF-UCCSD(T)-F12, which
`arc/testing/sp/TS_x118_sp_CCSD(T).out` confirms: that input pairs `wf,spin=2`
with `uccsd(t)-f12;` and the output reports `!RHF-UCCSD(T)-F12a energy`. UCCSD,
UCCSD(T), UCCSD-F12, UCCSD(T)-F12, UDCSD and UDCSD-F12 all exist and are what
an open-shell species needs.
**Open-shell MP2 has a different name.** Quickstart's open-shell table gives
"rmp2 !second-order Moeller-Plesset perturbation theory with RHF reference"
and notes that "all methods except ump2 use spin-restricted Hartree-Fock (RHF)
reference functions", `ump2` being the one built on a UHF reference this
adapter never writes. Explicitly correlated, the manual's command index lists
both `RMP2-F12` and `DF-RMP2-F12` -- "Spin-restricted open-shell DF-RMP2-F12
using ansatz 3C" -- and the CABS-singles section speaks of "the MP2-F12 or
RMP2-F12 program". There is no `UMP2-F12`, so ARC was writing a command with no
program behind it, and for `df-mp2-f12` the even more malformed `udf-mp2-f12`.
**F12c has no open-shell implementation.** "Currently CCSD-F12c is not
available for open-shell cases", and `CCSD(T)-F12c` is "Same as CCSD-F12c, but
perturbative triples are added".
**CASSCF takes no prefix.** It is the MULTI program -- "The program is invoked
by the command MULTI, MCSCF, CASSCF, or CASCI" -- and reads the spin from the
preceding `wf` card; there is no `UCASSCF`. This adapter's own multireference
branch already writes bare `{casscf; ... wf,spin=2,...}`.
ARC now looks the method up instead of prefixing unconditionally. `mp2`,
`df-mp2`, `mp2-f12` and `df-mp2-f12` are rewritten to their spin-restricted
open-shell spelling and `casscf` loses the prefix, each logged at warning level
naming both the command Molpro lacks and the one written; rewriting rather than
refusing is safe here because the substitute is the same theory for an
open-shell reference, and the substitution is density-fitting consistent on
both rows. Anything ending in `-f12c` raises NotImplementedError when the
species is open-shell, matching check_argument_consistency's refusal of Molpro
IRC, so the job fails at construction naming the reason instead of at the ESS;
matching the suffix rather than two exact names also covers `df-ccsd(t)-f12c`
and `mp2-f12c`. Every other method keeps the `u` prefix: it is what the
coupled-cluster and CI families need, and dropping it would silently turn a
correct `uccsd(t)` into the closed-shell program -- a worse failure than
today's. That matters because arc/job/trsh.py's molpro branch has no keyword
for an invalid command, so these inputs failed as "errored for an unknown
reason" and were retried.
Rendered method line for a triplet species, before -> after:
mp2 ump2; -> rmp2;
mp2-f12 ump2-f12; -> rmp2-f12;
df-mp2-f12 udf-mp2-f12; -> df-rmp2-f12;
casscf ucasscf; -> casscf;
ccsd(t)-f12c uccsd(t)-f12c; -> NotImplementedError
ccsd(t)-f12 uccsd(t)-f12; -> uccsd(t)-f12; (unchanged)
dcsd udcsd; -> udcsd; (unchanged)
Closed-shell species are unchanged in every case.
The `${restricted}` template slot is now always empty and is removed, along
with the multireference branch's clearing of it; that branch still builds its
own casscf/mrci blocks and is otherwise untouched, and the method lookup is
skipped for it so it can be neither substituted nor refused.
https://www.molpro.net/manual/doku.php?id=explicitly_correlated_methods
https://www.molpro.net/manual/doku.php?id=open-shell_coupled_cluster_theories
https://www.molpro.net/manual/doku.php?id=quickstart
https://www.molpro.net/manual/doku.php?id=the_mcscf_program_multi
Claude-Session: https://claude.ai/code/session_01MndwJm4BEs4DN3vJQsEwD3
… parsed
`test_get_ts_xyz_in_normal_mode_frame_falls_back_when_parsing_fails` pointed
at `arc/testing/freq/CH2O_freq_molpro.out` and reached the fallback only
because the Molpro geometry parser raised on every log it was handed -- it
iterated an already-consumed generator, and reported Bohr as Angstrom. That
defect is fixed on this branch, so the log now parses, and it parses into
formaldehyde's four atoms `('O', 'C', 'H', 'H')` against the six of the
CH4 + OH reaction the test builds. `get_ts_xyz_in_normal_mode_frame` then
takes its element-sequence-mismatch branch and returns `None`, which the test
subscripts.
Returning `None` on a symbol-sequence mismatch is the documented behaviour and
is not what this test is for: normal mode displacements are indexed by the
atom order of the output file, so displacing a geometry whose elements are in
a different order would move each atom along another atom's mode. That branch
already has its own coverage in
`test_get_ts_xyz_in_normal_mode_frame_returns_none_on_a_symbol_mismatch` and
`test_analyze_ts_nmd_returns_none_when_the_helper_yields_no_geometry`, and the
`log_xyz is None` fallback has coverage for the sub-path where the parser
returns nothing without raising, in the two tests that use the TeraChem log.
What this test alone covered is the other sub-path, where the parser raises
and the warning reports the exception -- so it now supplies a log that really
does make the parser raise.
The fixture is a Gaussian frequency output truncated in the middle of its
`Input orientation:` block, as a job killed while writing one leaves behind.
`determine_ess` identifies it as Gaussian and the adapter's reader walks off
the end of the file, so `parse_geometry` raises `IndexError`, which the test
asserts before exercising the helper. It is written to a `tempfile.mkdtemp()
directory released by `addCleanup` rather than into the read-only fixture
tree, and the test asserts on the `raised` wording that distinguishes this
warning from the one the other sub-path emits.
Claude-Session: https://claude.ai/code/session_01MndwJm4BEs4DN3vJQsEwD3
The Cp table the thermo export writes was renamed from cp_data to
thermo_points, widening it from Cp-only to Cp/H/S/G per temperature with no
alias kept. This test file was not part of that change and kept reading
content['P1']['cp_data'], so the assertion raised KeyError instead of checking
anything.
Nothing caught it. The file requires rmgpy and so skips under arc_env, which is
the only environment CI runs it in, and no workflow runs it under rmg_env. It
is a script-style test -- rmg_env's Python 3.9 cannot import the arc package
that pytest pulls in for a file under arc/scripts/ -- so it is run directly:
conda run -n rmg_env python arc/scripts/save_arkane_thermo_test.py
which now passes all 7 tests. The per-point check is also split out of the
all() generator so a failure names the temperature that failed rather than
reporting False.
The freeze directive of each constraint was written without a trailing newline, so a
deck holding two or more constraints ran the freeze line of one into the definition
line of the next (``B 1 2 FA 1 2 3 =104.50 B``), and a deck combining constraints
with a ``scan_trsh`` block ran the freeze line into that block's first directive.
Gaussian reads a ModRedundant section line by line, so both shapes are misread.
Every other directive the adapter writes into ``input_dict['scan']`` — the scan
branch's ``D ... S`` lines and ``ics_to_scan_constraints``' freeze lines — is already
newline-terminated, and the template closes the section with blank lines after
``${scan}${scan_trsh}${block}``. Terminating the last constraint therefore produces
exactly the deck shape a scan job already emits: the section closed by a blank line,
which is what ModRedundant requires, rather than one extra unterminated line.
The round-trip coverage in arc/parser/constraints_test.py writes one constraint per
deck by construction, so the two-constraint shape is pinned here instead.
3f21edc to
cefbbad
Compare
|
Thanks, all addressed at cefbbad.
Worth checking - all four reproduced, all four fixed:
Two other changes since ddc3081 that you'll see in the diff: the sidecar is renamed to parser_evidence.json, since it holds ARC's parsed facts and nothing TCKDB-specific. And the thermo block now records standard_state_pressure_pa (101325, i.e. 1 atm). It's derived from RMG's partition function rather than written in as a literal. |
What changed
Re-homes the ARC-owned result-production pieces from the historical TCKDB integration work onto clean ARC main, without bringing the TCKDB adapter back into ARC, and fixes Molpro and Gaussian defects in files this PR already touches.
output.ymlparser_evidence.jsonsidecar, schema 1.0, bound to output schema 1.1 by document IDtckdb-arcpackage, with disabled/missing-package no-op behaviorparse_geometryfor Molpro, which raised on every log and reported Bohr as Angstrom13 commits, 50 files, no file touched by more than one commit.
Why
The adapter is moving to its own repository, but ARC must continue to own parsing and publication of the scientific facts generated by ARC jobs. This creates that durable producer/consumer boundary without merging the old
tckdb-impbranch wholesale.The result contract
freq_hessian_methodoutput.ymlgains a per-species/TS key stating how the frequency job's Hessian was built:analytic,finite_difference_gradient,finite_difference_energy, or null.The archive consuming this record resolves its imaginary-mode noise floor from that fact, but records it only from an explicit keyword (Gaussian
Freq=Numer/Numerical/EnOnly, ORCANumFreq/AnFreq). Gaussian never states its analytic default, so the field was absent on every Gaussian record. ARC, as the producer, knows what it requested and which default applies, and can state it.The value is derived from ARC's own request — the frequency input deck in preference to the log's echoed route — combined with the documented behaviour of the ESS for that method class. Where the request does not settle the answer, the field is null. It is never inferred from anything weaker:
freq: analytic for HF (RHF/UHF), all DFT functionals including the documented double hybrids, MP2, CIS and CASSCF; gradient differencing for ROHF, MP3, MP4(SDQ), CCD, CCSD, CID, CISD, QCISD and BD; energy differencing for MP4 (which means MP4(SDTQ)), MP5, CCSD(T), QCISD(T), QCISD(TQ), BD(T) and BD(TQ). Composites, semi-empirical and force-field methods yield null, as does any method not on those lists.Freqis documented as an alias ofAnFreq, so it is analytic for HF and non-double-hybrid DFT without RI-JK, and null otherwise;NumFreqis gradient differencing, or energy differencing when combined withNumGrad.Provenance for the applied corrections
output.ymlrecords the atom-energy and bond-additivity corrections it applied, the level they were keyed to and thebac_type, but identified the software only asarkane_git_commit— a git HEAD read fromRMG_PATH. A pip or conda installed RMG-Py has no git repository there, so that key came back empty and the record then named no source at all for the corrections.A new top-level
arkane_versionsits beside it, mirroring the existingarc_version/arc_git_commitpair. Both fields resolve from one source so they cannot describe different installs: Arkane prints its version and the RMG-Py HEAD in its own log header, which ARC already produces undercalcs/statmech/, so that is read first, withrmgpy/version.pyunderRMG_PATHas the fallback for both together. Logs older than the run's start are ignored, since that directory survives between runs. Either key isnullrather than absent when nothing resolves.importlib.metadatais not a usable source here: RMG-Py ships no dist-info, so it raisesPackageNotFoundErroreven inside a workingrmg_env.The git-commit helper no longer catches every exception. A
RMG_PATHthat is set but is not a directory now logs a warning, so a misconfiguration is distinguishable from an ordinary non-git install.These name the Arkane that ARC invoked. They are not a claim about what it produced: a run that started and then failed reports the same identity as one that completed, and the correction tables can be empty while both fields are populated.
The evidence sidecar's
producerblock carries the same two values beside ARC's own, under the same names the output document uses, so a reader holding only the sidecar can still name the install that produced the corrections it describes. Both are always present andnullwhen unknown, following the block's existing convention forgit_commit.The level's
yearis carried through, and is part of this provenance rather than incidental to it: Arkane selects its atom-energy correction entry by year, so two levels identical in method and basis resolve to different correction tables when their years differ. A record that publishes the corrections without the key that chose them is incomplete.Which parameter block each correction came from
A consumer reading a corrected energy could see that a correction was applied but not which library block supplied it. Each per-species correction record reports
matched_arkane_key, the Arkane key ARC matched in the section that correction's parameters live in.The atom-energy and bond-additivity keys are matched independently and can differ, and the difference identifies the refit vintage. Against the current RMG-database — 68 atom-energy keys, 26 Petersson, 17 Melius — three pairs diverge exactly that way, for instance atom energies under
wb97mvagainst bond corrections underwb97mv2023. The bond-additivity record previously published the atom-energy key, so in those cases it named the wrong block. It now reports its own, andnullwhen nothing matched in that section rather than borrowing the other.One qualification the field's definition carries: the atom-energy key is the model chemistry ARC hands Arkane, so both totals are computed under it.
matched_arkane_keyon the bond-additivity record names the section ARC matched and the key the run-level table was read under, not a second key the numbers were computed from. Both facts are in the document and neither impersonates the other.The frequency scale factor now reports
freq_scale_factor_key, the entry it resolved to in ARC's ownfreq_scale_factors.yml. The resolved citation stays alongside it infreq_scale_factor_source: every entry in that file carries an explicit integer index into a top-levelsourcesmapping, written by the entry's author, so the attribution is a 1:1 pointer rather than an inference from neighbouring prose.The parser-evidence sidecar
The sidecar is named for what it holds — facts ARC's own parsers extracted from ESS logs — rather than for one consumer:
parser_evidence.json, schema namearc-parser-evidence, described inoutput.ymlunderparser_evidence, produced byarc/parser_evidence.py. It contains no TCKDB enums, payload shapes or upload requests.Standard-state pressure
Each species
thermoblock carriesstandard_state_pressure_pa, the pressures298_j_mol_k, everythermo_pointsentropy and free energy, and the NASA fit are referred to. It is not a literal:arc/scripts/save_arkane_thermo.py, which runs underrmg_env, recovers it by inverting RMG's own translational partition function, so it reports the pressure RMG actually divided by and follows any change there. Today that is 101325 Pa, i.e. 1 atm rather than 1 bar.It is per record rather than per file. Every thermodynamic quantity in
output.ymlcurrently comes from Arkane at one standard state, but the value enters ARC in the per-species record, and the paths that could add a second standard state are per species.What the parsed values guarantee
bac_typeis the correction requested; a species'bond_additivityrecord is emitted only when thermo was computed for it. The kinetics run applies none, and transition states never receive one.directionis retained.electronic_energy_hartreeis the post-SCF energy, taken by the same codeparse_e_electuses, not theSCF Donecomponent.null, so an export does not carry the local directory layout.=value Bline is carried onto the matchingFline.Versions
output.yml'sschema_versiongoes 1.0 to 1.1. That is an earned increment: main emits 1.0 today.Everything else introduced here starts at 1. The evidence sidecar is schema 1.0, and the parser-version strings are
arc-hessian-1,arc-irc-path-1andarc-gsm-stringfile-1. No consumer has received an earlier version of any of these.Molpro
Three independent defects.
Reading the version
output.ymlrecords the ESS version per job type, so a version that fails to parse is a silently incomplete record.arc/parser/adapters/molpro.py::parse_ess_versionmatched only aNAME : 2015.1.37header entry. Molpro 2015 emits that block; 2021 and 2022 do not, so ARC recorded no Molpro version at all for any modern run. Across the 11 Molpro fixtures in the repository, 6 returned nothing, including every 2021.x and 2022.x file.The banner line
Version 2022.3 linked Wed Nov 30 06:40:34 2022appears in all 11, so it is now the fallback. TheNAMEentry is still preferred where present, because it carries the patch level the banner truncates —2015.1.37against2015.1. In the 4 fixtures carrying both, the two agree andNAMEalways comes first.The scan is bounded to the first 200 lines, following the Gaussian adapter's precedent for a header field, but keeps the streaming read rather than Gaussian's
readlines()so a large log is never loaded whole. The deepest header among the fixtures is line 69.ORCA needed no change: it parses correctly across 4.1.2, 5.0.4 and 6.0.0.
The open-shell method name
arc/job/adapters/molpro.pyprefixeduto the method for any unrestricted species, unconditionally:u<method>is a Molpro command for some methods and not for others, so ARC could write an input Molpro has no command for.A per-method table decides the open-shell command, with a suffix rule for the methods Molpro does not implement open-shell at all.
ccsd-f12,ccsd(t)-f12,dcsduccsd(t)-f12mp2,df-mp2,mp2-f12,df-mp2-f12ump2-f12rmp2-f12UMP2-F12command exists; "all methods exceptump2use spin-restricted Hartree-Fock (RHF) reference functions"-f12cuccsd(t)-f12cNotImplementedErrorcasscfucasscfcasscfUCASSCFexistsu<method>u<method>A rewrite is logged at warning level naming both forms, since it changes the command from the one the user wrote. The substitution is the same theory:
RMP2-F12is open-shell MP2-F12, and the mapping is density-fitting consistent on both rows.Unrecognised methods keep the
uprefix deliberately rather than by omission. It is correct for the whole coupled-cluster and configuration-interaction family, and defaulting to no prefix would silently select Molpro's closed-shell program — a worse failure than an invalid command.The refusal keys on the
-f12csuffix rather than two exact names, sodf-ccsd(t)-f12candmp2-f12care covered too. It raisesNotImplementedErrorfrom job construction, matching the existing precedent for Molpro plus IRC incheck_argument_consistency.The multireference path is unaffected:
mrci/rs2already clears the prefix and builds its own command. A test fails if that guard is removed.Not a change: the
uprefix on coupled cluster is correctWorth stating, because it looks like the same class of problem and is not. Molpro's
HF/RHFis the spin-restricted program, and ARC's template hardcodes{hf; ... wf,spin=N,charge=C;}, so ARC never requests UHF orbitals. Theuselects the open-shell correlation ansatz on an RHF reference, givingRHF-UCCSD(T)-F12, which the manual requires for open-shell F12. ARC's own checked-in output confirms it:arc/testing/sp/TS_x118_sp_CCSD(T).outpairswf,spin=2withuccsd(t)-f12;and reports!RHF-UCCSD(T)-F12a energy. ARC's default single point,ccsd(t)-f12/cc-pvtz-f12, was correct before this change and is unchanged by it.Geometry parsing
parse_geometryiterated a generator that had already been consumed, so it raised on every Molpro log, and where it did return coordinates they were Molpro's Bohr values labelled as Angstrom. It now iterates a list and converts to Angstrom viabohr_to_angstrom.Gaussian and uploading
Gaussian constraint lines
The ModRedundant writer emitted each freeze directive without a trailing newline, so a deck with two constraints read
B 1 2 FA 1 2 3 =104.50 B, and a deck combining constraints with a troubleshooting freeze block readB 1 2 FB 3 4 F. Each directive now ends its line, and the section is closed by the blank line ModRedundant requires.Uploading
tckdb_arcis optional and imported only when the input'stckdbblock setsenabled: true. It is not on PyPI; it lives at https://github.com/calvinp0/tckdb-adapters. The upload destination is logged as host and port only, so credentials in the URL never reacharc.log. Thetckdbblock is not written torestart.yml, and a restarted run says so rather than skipping the upload silently.Validation
-n 4 --dist workstealarc/scripts/save_arkane_thermo_test.pyunderrmg_env: 10 tests, OK — it skips inarc_env, which is the only environment CI runs it inThe
freq_hessian_methodrule table was validated by a quantum-chemistry review against vendor documentation, then by an adversarial code review which mutation-tested it: forcing the rule to always-absolute, always-multiplicative, or with the donor and acceptor partners swapped each fails the tests, so they are not tautological.A later adversarial review of the Arkane provenance was answered the same way. Removing the schema test's resolver patch, or reverting the append-log guard, each fails a specific test — so both are load-bearing rather than decorative.
🤖 Generated with Claude Code
https://claude.ai/code/session_01JYTE9rn3gHi6jDc7ehuHkn