Conversation
T3's reaction-uncertainty logic lived inline in reaction_requires_refinement, conflating two things: a dedup gate (has this reaction already been queued?) and the actual uncertainty semantics (library / exact-match rate rule => certain). Extract the uncertainty half into t3/utils/uncertainty.py so every refinement path -- species, reactions, and the upcoming PDep network selector -- shares one definition. The core operates on a bare kinetics-comment string because the network selector reads provenance straight out of an RMG pdep network file's reaction() blocks, where no T3Reaction object exists. Behavior is unchanged: a characterization test was written and confirmed green against the unmodified code first, pinning the permissive default that the legacy (vacuously true) `or '' in kinetics_comment` clause produced.
The shared uncertainty predicate previously recognized provenance only via an
explicit ``kinetics_method=LIBRARY`` argument or the exact phrase "Exact match
found for rate rule". Callers that hold only a bare kinetics comment - notably
the upcoming PDep network selector, which reads comments straight out of an RMG
``network.py`` - therefore misclassified real library and training reactions as
uncertain, and missed several RMG estimate forms entirely.
Provenance is now read from the comment text itself.
Certain (real data for *this* reaction):
* "Exact match found" (shortened from "... for rate rule": RMG wraps some
comments with a literal line continuation that splits "for rate\ rule",
and RMG's own parser uses the short form)
* "Matched reaction <n> ... in <Family>/training" - this reaction IS the
training reaction. Matched with a bounded regex, not a bare "/training"
test: RMG also emits "From training reaction <n> used <rule>", which means
the rate RULE was fitted from a training reaction and then applied by
analogy, and must stay uncertain.
* "Library reaction:", "... from reaction library ...", "Seed mechanism:"
Uncertain (an estimate, and these override a certain marker):
* "Estimated using ..." and "Estimated from node ..."
* "BM rule fitted to <n> training reactions" - an exact match to a fitted
tree node is not real data. Every such comment in tests/data reports
Total Standard Deviation in ln(k) between 1.09 and 24.6 (median 11.5).
* "... to match endothermicity of reaction" - RMG raised the barrier. Keyed
on the endothermicity clause rather than "Ea raised from", so the no-op
"Ea raised from -0.0 to 0 kJ/mol." form does not count.
Per-path-reaction "High-P limit: ..." PDep debug lines (rmgpy/chemkin.pyx) are
stripped before scanning, in both directions, so one library path reaction
cannot make a whole PDep net reaction certain.
Method strings are normalized case-insensitively with an alias map, and an
unrecognized string now falls through to comment analysis instead of being
coerced to UNKNOWN and thus uncertain. Only LIBRARY and TRAINING_SET short
circuit at the method level; QM and USER deliberately do not, so a failed,
placeholder, or stale QM/user reaction can still be re-queued.
BEHAVIOR CHANGE for existing T3 users: this also changes
``T3.reaction_requires_refinement``. Measured over the 16091 reaction comments
in tests/data, versus the previous predicate: 1576 reactions flip
uncertain -> certain (1372 library-in-text, 204 matched-training - these were
being sent to expensive QM despite having real data), and 430 flip
certain -> uncertain (368 BM tree-node rules, 62 Ea-raised-to-match-
endothermicity - genuinely uncertain rates that were being skipped).
The dedup gate stays in ``T3.reaction_requires_refinement``.
Add a `t3/pdep/` layer that decides which whole pressure-dependent reaction
networks are worth expensive QM, rather than only picking individual wells.
A network qualifies when both hold:
(a) an observable is sensitive to one of its reactions (established by T3's
existing top-SA machinery), and
(b) the network's own k(T,P) is in turn sensitive to a transition state whose
kinetics is merely an estimate, judged by the shared provenance predicate.
New modules:
- `parser.py` reads RMG `network*.py` with `ast.parse`. These files are
executable Arkane DSL and are never executed, imported, or `exec`ed; only
their declarative structure is read. A regression test proves a top-level
side effect in a network file is not run.
- `selector.py` the pure decision core, holding no T3 state and doing no I/O,
so the in-run path and the upcoming public API cannot disagree. Returns a
`PDepNetworkSelection` carrying the verdict, the evidence, the thresholds
used, and any warnings.
- `cache.py` a T3-owned sidecar that makes reuse of an existing Arkane
sensitivity analysis safe.
Numerics, both deliberate:
- The relative gate's denominator is over transition-state rows only. Well rows
and TS rows are different derivative coordinates, so a shared denominator
would compare unlike quantities and make the selected TS set move whenever an
unrelated thermo row moved.
- The relative gate alone cannot tell a real derivative from a denormal ~1e-18
structural zero, so an absolute floor is applied too. It is expressed as a
dimensionless rate response (the new `pdep_min_delta_ln_k`, default 1e-3) and
converted using the perturbation size, so it keeps its meaning if Arkane's
2 kcal/mol perturbation ever changes.
Behavior changes for existing users:
- Well selection now ranks by absolute value. It previously compared signed
coefficients against a signed maximum, so a strongly negative sensitivity --
just as real as a positive one -- was skipped. The same absolute floor is
applied to wells, which keeps denormal rows out.
- `run_arkane_job` no longer accepts any `.yml` under `sensitivity/` as proof of
success. A stale file from an earlier run is not evidence, so the target is
removed before Arkane is invoked and must exist afterwards.
Why the cache needs a sidecar: nothing in Arkane's own output identifies which
Arkane wrote it or which network it came from. An analysis whose TS perturbation
never reached the rate expression writes structurally dead TS rows that read
exactly like "nothing here is sensitive", so trusting one would drop the network
from QM with no visible symptom. The sidecar records the selector version, the
perturbation, and hashes of both the network file and the sensitivity output,
and additionally records the largest absolute TS coefficient present. A cache
whose TS rows carry no signal is rejected rather than believed, and a cache with
no sidecar is never trusted. The cost of that choice is real: against such an
Arkane the analysis is regenerated every iteration instead of cached. A loud,
expensive failure is preferred here to a quiet wrong answer, and the warning
names the likely cause.
The two directions of a reaction appear as separate sensitivity keys, so the
requested direction is preferred, falling back through label canonicalization to
the opposite direction. Measured on the checked-in fixture, forward and reverse
coefficients agree to 4.6e-11 relative for TS rows above the floor, so the
fallback answers the same question; a test pins that premise so a future change
fails loudly instead of the fallback going quietly wrong.
The sensitivity fixture is real patched-Arkane output for `network4_2`. It lives
under `tests/data/pdep_sa/` rather than mirroring the runtime layout, because
`tests/data/pdep_network/iteration_1/PDep_SA` is `t3.paths['PDep SA']` and is
deleted by an existing teardown -- which made the new tests order-dependent
until the fixture was moved.
Qualifying networks are recorded and logged but do not yet trigger work; the
master-equation and explorer adapters that consume the decision follow.
Add `t3/pdep/api.py`, the standalone entrypoint an external client imports to ask whether a PDep network deserves QM refinement without running T3. This is the trigger channel described in the design's D3/D4, kept deliberately separate from the diagnostics schema. It never reimplements the decision. Every answer comes from the same pure core the in-run path calls, so the two channels cannot drift apart, and the parity test compares the API's decision against a real `determine_species_from_pdep_network` run rather than against the core it already shares -- otherwise the test could not catch a divergence in defaults, which is the only way the two would realistically disagree. For the same reason the API's thresholds are read off `T3Sensitivity`'s own field defaults instead of being copied literals, with a test asserting they match. A copied default is exactly the kind of thing that drifts silently. Entry points: - `select_pdep_network()` accepts an RMG `network.py` path or an already-parsed network. Naming a reaction asks T3's narrower in-run question; omitting one asks whether the network is worth refining at all, aggregating over every reaction in the sensitivity output. - `rank_pdep_networks()` orders networks most-deserving first, by the largest ln(k) response among the uncertain path reactions. - `save_pdep_network_selections()` writes the result for an external reader. The design note this implements mentions accepting "a T3-standardized YAML" for the network. No such schema exists in this repository -- it was floated once and never built -- so only the real `network.py` form is supported, and the module docstring says so rather than inventing a format nothing produces. Decisions worth stating, since each rules out a quieter alternative: - A rejected cache returns a decision marked `not_evaluated`, not a plain "no". A standalone caller cannot regenerate the analysis, so "I could not evaluate this" and "this is safely not worth QM" must not look identical. Ranking returns those entries too, instead of dropping them. - `explore=True` raises rather than being accepted and ignored. The explorer adapter lands in a later commit; accepting the argument now and doing nothing would be a lie the caller cannot detect. The signature is stable so that commit only has to fill it in. - Sensitivity YAML is read through a restricted loader that understands the `!!python/tuple` condition keys Arkane emits and refuses every other non-safe tag. The unrestricted loader is fine for T3's own output but not for a public entrypoint reading a caller-supplied path. - Combining per-reaction decisions refuses to merge different networks, and drops the method and cache status to unset when components disagree, so an aggregate cannot claim more provenance than its parts had.
…iterion
Adds the first of the two adapter families from D5: an ME-solver family that solves a
GIVEN pressure-dependent network and returns k(T,P), with Arkane as the sole concrete
implementation this round. Mirrors the existing t3/simulate registry idiom (module-level
dict + a free register_*_adapter() called bare at the bottom of each concrete module +
eager imports in the package __init__), not a decorator.
The substantive work is the success criterion. T3's existing Arkane gate is SA-specific
(it requires sensitivity/sa_coefficients.yml), and a plain ME solve produces no such file,
so it needed its own. Three real Arkane runs established what that criterion has to be:
- Arkane creates output.py BEFORE any job runs (arkane/main.py:186-187), so a hard crash
leaves a 0-byte output.py behind. Existence proves nothing.
- A failing CSE solve exits 0 with EMPTY stderr and writes a syntactically perfect
pdepreaction(...) block whose Chebyshev coefficients are all None; chem.inp carries
literal nan. Arkane's own "Negative rate coefficient generated; rejecting result."
goes to stdout, not stderr.
So exit status, stderr, and file existence all pass a run that produced nothing usable.
Only parsing the numeric payload discriminates. No Arkane/RMG API reads output.py back
(pdepreaction is write-only and absent from the input DSL), so it is scraped with ast,
consistent with the never-exec rule and with T3 shedding its RMG-Py API dependencies.
check_arkane_me_success() therefore gates on the RATE PAYLOAD specifically -- the coeffs /
A,n,Ea leaves -- and not on every numeric leaf of the kinetics call. Tmin/Tmax/Pmin/Pmax
are themselves finite numbers, so a Chebyshev(coeffs=[], Tmin=..., Tmax=...) would satisfy
an "all leaves finite, at least one present" check while carrying no rate information at
all. The parser records separately which kinetics keywords were omitted because they could
not be literal-evaluated, so an unparseable coeffs is distinguishable from an absent one.
Coefficients are deliberately NOT required to be positive: Chebyshev coefficients are
log-space and legitimately negative; positivity is a property of k(T,P), not of the fit.
Two refactors make room for this, both behavior-preserving for their existing SA callers:
write_arkane_network_input_file() is extracted from write_pdep_network_file() with a flag
controlling the sensitivity directive, and run_arkane_job() gains a required_artifact
parameter defaulting to the current SA path. The artifact is deleted before the run and
required after, so it is confined to the job directory -- an absolute or traversing value
is rejected rather than deleting a file outside it.
Known and deliberate: expected_reactions is plumbed through but defaults to None, so output
completeness is not verified unless a caller supplies it; deriving it from network topology
is deferred, since the number of net reactions is not trivially the number of path
reactions and guessing wrong would reject good solves. set_up() runs from __init__ to match
the simulate adapters, so constructing an adapter writes input.py and output_directory must
be unique per instance. allow_ilt_complement is enforced in the factory only; that becomes
load-bearing when a non-Arkane adapter exists, and the check should move into the adapters
then.
Reviewed adversarially (spar round 8): 3 P1, 3 P2, 3 P3, all real, all fixed here.
Build the network input file that hands Arkane a QM transition state for the path reactions we refined while leaving the rest on ILT, so a single master equation solve mixes both sources. Two invariants make the result trustworthy rather than merely plausible: - Every QM'd path reaction loses its `kinetics = Arrhenius(...)` entry. Left in place, Arkane silently prefers it and falls back to ILT, and the caller believes it received QM when it did not. A `transitionState=` that is present but cannot be evaluated as a literal is refused outright for the same reason: we cannot tell whether that reaction needs its kinetics dropped, and guessing would break the invariant silently. - Artifacts are vendored, not referenced. ARC deletes and recreates `calcs/statmech/kinetics` on every rate pass, so a pointer into it is ephemeral. Vendoring stages into a sibling directory and atomically renames it into place, so a source that vanishes mid-copy leaves the previous good `qm/` intact instead of destroying it and leaving `input.py` aimed at a half-populated tree. Every path resolved out of a `Log(...)` call is confined to an explicit root with `realpath` + `commonpath` before it is copied. Vendoring copies these files, so an artifact naming an arbitrary absolute path would otherwise pull it into the run directory. This commit also hardens `rewrite_arkane_method_line`, because it introduces the helper's second call site and cannot be correct without it. `rewrite_arkane_method_line` returned the line untouched whenever it could not parse it, and both call sites gated on the literal substring `'method = '`. Three ordinary inputs therefore kept the source file's method silently: a double-quoted `method = "modified strong collision"`, an unspaced `method='CSE'`, and a network file carrying no method line at all. Since each network is solved once per ME method, a silent no-op makes both passes solve the same method while the SA cache records the one merely requested, and `validate_sa_cache` then vouches for that mismatch on every later run. The assignment is now matched with a regex accepting either quote style and any spacing, both call sites count the rewrites, and anything other than exactly one rewrite raises. A `method = ` line inside a triple-quoted string is still matched; line-based scanning cannot tell it from a real assignment, and that limitation is stated in the docstring.
…count `check_arkane_me_success` accepts an `expected_reactions` count to catch a truncated Arkane `output.py` -- a solve that died partway leaves fewer `pdepreaction(...)` entries, each of them syntactically perfect and numerically finite, so no per-entry check can see it. Nothing ever passed the argument, so the hole stayed open. This makes the count derivable from the network file. Wiring a production caller to pass it is the next commit. Arkane enumerates net reactions in `PressureDependenceJob.save()` as a double loop whose source index runs over the isomers and reactant channels only and whose destination index runs over those plus the product channels, skipping the diagonal, then comments out any entry that duplicates an already-printed one. No branch of that loop omits an entry -- a failed fit raises, and a rejected fit still writes its entry with empty coefficients -- so the live count is exact rather than an upper bound, which is what makes it usable as a gate. Two things make the count harder than it looks, and both were found by measurement rather than by reading the source. First, P. The parser read the `products` keyword literally, and no RMG-generated network file ever writes that keyword: Arkane derives the product channels from the path reactions instead. Every real fixture therefore reported P = 0, which would have predicted 3 net reactions for `network4_1` where Arkane writes 12, and REJECTED good complete solves -- a worse failure than the truncation being closed. `_derive_product_channels` now mirrors Arkane's derivation, and `PDepNetwork.product_channels` holds the resolved channels, with `product_channels_declared` recording which branch produced them. Second, the duplicate suppression is not a closed form. `S*(S-1)/2 + S*P` assumes duplicates arise only between the two directions of a source-source pair, but Arkane compares each candidate against EVERY previously printed reaction, by label and in either direction. One configuration can appear in both channel lists -- a declared unimolecular reactant channel is also derived as a product channel, since the derivation's unimolecular branch tests only against the isomers -- and then the entries reaching it are commented out as duplicates of the entries leaving it. `expected_net_reaction_count` therefore simulates the loop rather than reducing it. The derivation's two arity branches are asymmetric, and deliberately kept so: Arkane tests a unimolecular side against the isomers only and a multi-species side against the reactant channels only, despite its own comment claiming both are tested against both. Unifying them reads like a cleanup and silently drops channels, so `network_arity_asymmetry.py` pins both consequences. Verified against real Arkane MSC runs, not against the source alone. Its log named the three derived product channels of `network4_1` and its `output.py` holds exactly the predicted 12 live entries; a second run of the same network with one channel deliberately made to appear in both lists holds 15 live and 9 commented, where the closed form predicts 18. Both outputs are checked in under `tests/data/pdep_me/` (the previous ME fixtures all come from a degenerate one-isomer network with a single entry, which cannot distinguish these models from several others), and the end-to-end tests derive the count from the network file rather than restating it, so they fail if the derivation and Arkane ever drift apart. The derivation, the branch asymmetry and the duplicate suppression are each mutation-tested. Targeted subset (tests/test_pdep, tests/test_runners, tests/test_utils/ test_writer.py): 204 passed, up from 194.
…label
A PDep network file names its transition states in RMG's namespace (TS1, TS3,
...), local to one file; ARC names them in its own, from the position of the
reaction in ARC's reaction list. The two collide -- TS1 in network4_1.py is
almost never ARC's TS1 -- so finding the Arkane statmech .py ARC wrote for a
selected transition state needs an explicit, durable tie.
Make the tie by CHOOSING ARC's name rather than predicting it.
ARCReaction.ts_label is honored when already set: ARC only falls back to
f'TS{rxn.index}' when it is None (arc/scheduler.py:379). Unlike T3's own
t3_index, ts_label also survives ARCReaction.copy(), which is what
T3.add_reaction puts on the QM queue -- copy() is hardcoded to return an
ARCReaction, not type(self), so every T3-only field on the queued object is
dropped. So T3 assigns a deterministic namespaced label up front and the join
is known at queue time, without depending on ARC's indexing at all.
Labels are refused, never sanitized. Rewriting an unsafe character would let
two distinct networks (or transition states) collapse onto one ARC label and
silently join a network to another network's quantum chemistry. The same
one-to-one requirement is enforced in both directions, on write AND on read:
the sidecar sits on disk between two ARC-length steps, so the reader cannot
assume the writer's guarantees still hold.
This commit is the pure, standalone half of the work; nothing calls it yet.
Wiring it into t3/main.py follows separately.
…o ARC Wire t3.pdep.join into the in-run path. When a network qualifies for QM refinement, each of its uncertain transition states is queued to ARC as its own reaction, tagged up front with the deterministic ARC label that ties its eventual quantum chemistry back to the network transition state it came from. The join sidecar is written into the iteration's ARC project directory. The parsed network is label-only, so the species ARC needs are built from the sensitivity output's 'structures' mapping -- the one source that covers edge species as well as core ones. Every selected transition state produces a record, including the ones that could NOT be queued, because a silently dropped transition state is later indistinguishable from one whose quantum chemistry failed and the two call for opposite responses. Three cases are recorded rather than queued: - no path reaction declares the transition state; - several path reactions share it, so which reaction it refers to is ambiguous. Computing a TS for A <=> B and then using it for C <=> D is silently wrong chemistry, not an approximation, and there is no basis for picking one; - a species has no adjacency list, so the reaction cannot be built. A reaction T3 already knows is recorded as such with NO expected artifact path: add_reaction does not re-queue it, and if it was added in an earlier iteration its QM entry has since been cleared, so ARC will not recompute it. Claiming a path there would make a never-computed transition state look merely unfinished. The records are emptied at the start of each pass. They describe one iteration's queue and are written to that iteration's project directory, so a carried-over record would advertise a previous iteration's artifact path.
…able ARC does not converge every transition state T3 sends it. Across the real ARC projects on hand only 4 of 9 transition states ended up with a statmech artifact, so "the artifact is missing" is the common case rather than the exception, and the PDep layer needs an honest answer for it before a hybrid network is written. Discovery recomputes each ARC label and artifact path from the join sidecar rather than trusting the path recorded in it, confines the result to the ARC project directory, and calls a transition state usable only when ARC did not report it as failed, the artifact exists, and it parses with every referenced log resolvable. Existence alone is not convergence: a stale file can survive a previous pass and a referenced ESS log can be cleaned up, and preflighting here is what keeps the hybrid writer from raising late, after the policy already counted the state. Convergence is read from status.yml in preference to output.yml, which collapses "explicitly failed" and "not yet determined" into one value; when only the lossier source is available the verdict fails closed but says it could not tell the two apart. A project with neither file leaves convergence unknown, which stays usable if the artifact itself is sound. Acceptance is strict by default: a network is refined only when every selected transition state is usable. A partial result is still described, carrying its coverage and flagging when the state it is missing is the one the rate is most sensitive to, but taking it as the refined network is the caller's explicit choice. Nothing calls this yet; wiring it into t3/main.py follows separately.
Copy the transition-state artifacts ARC produced, and the logs they point at, into a capture directory T3 owns, so everything downstream reads a frozen tree instead of ARC's live one. ARC deletes and recreates `<project>/calcs/statmech/kinetics/` at the top of every high-pressure rate pass, so the pointer `.py` files there are ephemeral. Their `Log(...)` targets live in a sibling tree and do survive, but the pointer itself is not mere path indirection: it carries the `linear`/`spinMultiplicity`/rotor selections and the sp/freq bindings ARC rendered, none of which is recoverable from the logs without redoing that rendering. Capturing is therefore not an optimization. Discovery, capture, and the manifest write are one entry point rather than three composable ones. A public "remember to capture later" helper would only relocate the bug: if T3 dies after ARC terminates but before capture, a restart does not enter `handle_arc_restart()`, because ARC already looks terminated, and the iteration replays against a wiped pointer tree. Two things are frozen rather than referenced: - Convergence is resolved while ARC's state is still live and written into the manifest as a verdict. `output/status.yml` and `output/output.yml` are deliberately NOT copied: vendoring copies with `shutil.copyfile`, which does not preserve mtime, and discovery breaks a status/output disagreement by comparing their mtimes. Copying both would land them at the same instant and silently collapse exactly that discrimination. - The hashes are load-bearing, not provenance decoration. Captured bytes are verified against their sources immediately after the copy, and `verify_capture()` re-checks a capture against its own manifest so a consumer can validate one it did not create. The vendored `qm/` tree and the manifest are two objects written at different moments, so a crash between them leaves them disagreeing; that torn state is now detected and refused instead of silently consumed. The pointer file is exempt from byte-equality on purpose, since vendoring rewrites its `Log(...)` arguments to relative paths. It is verified structurally instead: re-reading the captured pointer must resolve the same number of logs, with the same set of hashes, as the source did. A capture directory is refused if it sits inside the ARC project directory, which would defeat the durability the capture exists for, and refused if it holds content this module did not put there, so the dedicated-directory requirement is enforced rather than left to the caller.
Call the capture boundary from T3's ARC finalization, and make a restart notice when finalization never happened. The gap this closes is narrow and silent. `restart()` derives `run_arc` from `rmg_terminated and not arc_began`, and `restart_arc` from `arc_began and not arc_terminated`. A crash after ARC wrote its own termination line but before T3 finalized leaves `arc_began` and `arc_terminated` both true, so neither flag fires, `handle_arc_restart()` is never entered, and the run falls through to "skipping RMG". The iteration then replays through ARC with the previous run's artifacts never captured -- and ARC deletes `calcs/statmech/kinetics/` at the top of its next rate pass, so by the time anything looks for them they are gone. Finalization is therefore gated on a completion marker T3 owns, rather than on ARC's own log. The marker sits at the iteration level, outside the ARC project directory, because anything stored inside it is exactly what ARC is liable to remove. The new branch fires only for iterations that left a P-dep join sidecar. An absent marker cannot by itself tell "crashed before finalizing" apart from "finalized by an earlier T3, which never wrote markers", so keying off the marker alone would re-finalize every project completed before this existed, re-appending library entries and advancing the iteration under them. The sidecar is the positive evidence that finalization has something to rescue; without one, behavior is exactly what it was. Capture runs first within the boundary. It is the only step whose inputs are ephemeral -- everything after it reads durable state -- so sequencing it after the ARC-info parse or the library append would let a raise there destroy artifacts that were still recoverable. The marker is written last, atomically, and its contents are checked when read. Any exception from capture propagates rather than being swallowed, so a failed capture leaves the marker absent and the work is retried; marking completion first would turn a recoverable crash into permanent data loss, and reading mere file existence as proof would let a truncated marker skip finalization forever. `process_arc_run()` consults the marker itself, so a redundant call is a no-op instead of appending the same results twice, and starting a new ARC run clears the marker its predecessor left behind. A missing sidecar means "none were queued" only if ARC also produced no T3-labelled artifacts. When such artifacts do exist with no sidecar naming them, the join is lost rather than empty, and finalization refuses instead of silently abandoning completed QM work. Capture is invoked from `process_arc_run()` alone, which all three call sites already funnel through, so the three sites are untouched. It reads the join sidecar rather than `self.qm`: the queue holds `reaction.copy()`, a plain `ARCReaction` on which only `ts_label` survives, so it cannot carry the provenance capture needs, and the sidecar is written before ARC starts. The marker and its helpers are named for ARC finalization, not for P-dep, because the flag gates finalization generally -- a P-dep name on a file that governs the restart path would misdirect the next reader.
A capture exists to outlive the ARC project directory it came from, but it could not describe the energy reference its own QM artifacts were computed on. ARC's per-transition-state statmech artifact carries only geometry, frequencies and Log(...) pointers; the model chemistry lives solely in the sibling calcs/statmech/<subdir>/input.py and output/output.yml, both inside the ARC directory. So write_hybrid_network_input_file(), which refuses a blank model_chemistry, could never be driven from a capture alone. Freeze it instead. read_arc_energy_settings() resolves the settings from ARC's own statmech directives at capture time and capture_ts_artifacts() records them under the manifest's new, AUTHORITATIVE 'energy_settings' key -- the same treatment the status/converged/reason verdicts already get, and deliberately unlike the inert 'provenance' archive, which stays unreadable as authority. Values are frozen, never lookup keys: ARC/data/AEC.yml is version-dependent, so storing only a model chemistry label would let a later edit shift a transition state's E0 while every capture hash still verified. input.py is parsed with ast.parse over module-level assignments only, never exec'd -- the file is executable Arkane DSL that also calls species() and reaction(). output.yml is read with yaml.safe_load. The two are cross-checked on the frequency scale factor and bond-correction type, and disagreement is fatal; their method and basis NAMES are deliberately not compared, because ARC de-hyphenates and year-suffixes them in input.py only. A null correction block means unknown, not absent: ARC computes those in a subprocess wrapped in a bare except. Two defects in QMEnergySettings surfaced while wiring this up: model_chemistry could not represent its only real data source. The validator rejected quotes and newlines, but ARC emits a bare LevelOfTheory(...) call, or a multi-line CompositeLevelOfTheory(...) -- both quoted, one multi-line, so both were refused outright. It now validates structurally via ast.parse, and _build_energy_header renders an object expression bare and a plain label quoted; emitting a quoted LevelOfTheory(...) would silently hand Arkane an inert string where it expects evaluable syntax. useAtomCorrections was never emitted at all, so the header's documented promise to fail loudly rather than silently disable atom corrections was unenforced. The field now exists and False raises, rather than producing a hybrid network whose QM'd transition state sits on a different energy scale than the RMG wells around it. The plain-label form is frozen as its string value, not its source segment. Freezing the segment kept the surrounding quotes, which the hybrid writer then refused -- two individually-correct behaviours composing into a hole that would only surface once the ARC directory was already gone.
Completes the capture contract. With the energy reference already frozen, two inputs to write_hybrid_network_input_file() were still unreachable once ARC finished: the original RMG network.py (no network_id-to-path mapping was ever persisted) and the master-equation method, which lived only in memory on self.pdep_network_selections and so was gone after any restart. Both are now frozen where they are actually known: at join-sidecar write time, inside determine_species_from_pdep_network, which already computes the network path and appends the selection. The sidecar gains a networks block carrying the source path, its hash_file() fingerprint, and the method, cross-referenced against the transition-state records in both directions -- a sidecar naming a transition state whose network has no source is unusable downstream, so it is refused rather than carried. The capture then re-verifies each network against that fingerprint and VENDORS the file into networks/<network_id>.py. Hashing alone would have been the cheaper change and the wrong one: a hash detects that RMG regenerated the network between queueing and capture, but it cannot supply the file once the pdep directory is gone, which is precisely the situation the capture exists to survive. A mismatch is fatal, because the frozen transition state labels may no longer name the same reactions. There is now one test that proves the whole point rather than its parts: with both the RMG pdep directory and the ARC project directory deleted, a capture alone still drives a complete hybrid network write. It uses the real network fixture deliberately. The stub written by the test helper hashes and vendors perfectly while having no reaction(...) entries at all, so it is unparseable by the writer that has to consume it -- byte-identity is not usability, and every existing vendoring assertion only checked bytes. The shared energy-settings fixture had set useAtomCorrections False for test convenience. ARC computes that flag as bool(model_chemistry or atom_energies) and so can never emit False alongside a modelChemistry line, making it an ARC-impossible state that the hybrid writer correctly refuses. The fixture is now faithful, which is what lets the end-to-end path be exercised at all. Wiring sensitivity_by_ts through to the manifest is deliberately not included. _capture_pdep_ts_artifacts also runs on the restart path, where the in-memory selections are empty, so the obvious wiring would silently record absent evidence exactly when a capture is replayed. Freezing the coefficients onto the join records at queue time is the real fix and belongs with its own tests.
A boolean Arkane directive that is not actually a bool silently inverts itself. The dangerous shape is a non-empty string: `useAtomCorrections = 'False'` is TRUTHY, so `if not energy_settings.use_atom_corrections` never fires and the guard added in 32c17e6 is bypassed entirely -- while `_build_energy_header`'s f-string renders it straight back out as a bare `useAtomCorrections = False` that Arkane evaluates as the boolean. The guard is defeated and the directive inverted in one step, and the hybrid network lands silently on the wrong energy scale. Enforce exact types at all three boundaries the value crosses, since each is independently reachable: * `read_arc_energy_settings` (producer): every `ast.literal_eval`'d directive is type-checked against `_LITERAL_FIELD_TYPES`. * `validate_frozen_energy_settings` (new, in energy_settings.py): the one authoritative definition of a frozen block's shape, called by `verify_capture` -- the manifest is a file, so it can be hand-edited into any shape between being written and being read back. * `write_hybrid_network_input_file` (consumer): `QMEnergySettings` is a plain dataclass whose annotations enforce nothing at runtime, and its values come from a YAML-loaded manifest. Types are compared by exact `type()` membership rather than `isinstance`: `isinstance(True, int)` is True in Python's numeric tower, so an isinstance check accepts `frequencyScaleFactor = True` and silently scales every frequency by 1.0. That distinction is mutation-tested -- swapping in isinstance kills only the `frequency_scale_factor=True` case, which is exactly the case that matters. Round-22 P1 #1. Eleventh fail-open defect on this branch, same shape as the other ten.
The AST-structural allowlist for `modelChemistry` was too permissive to be a validator for a value that is rendered BARE (unquoted, executable) into a generated Arkane input file that Arkane then executes. It accepted any keyword NAME and any `ast.Constant` type, so `LevelOfTheory(__class__='x')`, `LevelOfTheory(method=True)`, `LevelOfTheory(method=...)` (Ellipsis) and a vacuous `CompositeLevelOfTheory()` with no freq/energy at all all passed structural validation and failed only inside Arkane, i.e. after the file was written. Bind the allowlist to Arkane's actual constructor schema (arkane/modelchem.py:98-121, 173-191, read-only): `LevelOfTheory`'s fields are method (required) plus optional basis/auxiliary_basis/cabs/software/solvent/solvation_method/args, all str, with software_version the one genuinely non-str field (int|float|str); and `CompositeLevelOfTheory` takes exactly freq and energy, both required, both `LevelOfTheory`. Required fields are now enforced, keyword names are restricted to that field set, each value's type is checked by exact `type()` membership (so a bool cannot pass as an int), and duplicate keywords are rejected. Round-22 P1 #4.
Two defects that both let a capture be trusted when it should not be. Supersession was not transactional. `capture_ts_artifacts` wrote straight into `capture_dir`, so a run that died partway through left the previously good capture already destroyed and the new one incomplete, with no way back -- demonstrated with a probe test before the fix. The whole capture (qm/, networks/, provenance/, manifest) is now built in a sibling `.capture-staging-*` directory, self-checked with `verify_capture` while still staged, and only then swapped in: rename any existing capture_dir aside, `os.replace` the staged tree into place, and remove the renamed-aside tree only once the new one is confirmed. If the replace itself fails, the old tree is renamed straight back. `capture_dir` is therefore always either the complete old capture or the complete new one. `verify_capture` did not confine manifest-supplied paths. `captured_artifact_path`, every `captured_log_sha256` key, and the networks block's `captured_path` were joined onto `capture_dir` with no confinement, and `os.path.join(a, '/abs')` returns `/abs` -- so a tampered manifest could make verification "pass" against an external file that happened to match, rather than the capture's own vendored copy. All three now go through `_confine_to_capture_dir` (realpath + commonpath), mirroring `t3.pdep.discovery._confine_to_project`. Also fixes a vacuous test found while mutation-testing this: the pre-existing dotdot-escape test used two `..` components and overshot to a nonexistent path, so it passed on an unrelated "no longer exists" error whose message merely happened to contain the substring it matched on. It passed with confinement removed entirely. Round-22 P1 #2 and #3.
Establishes one rule: every correction the generated Arkane input turns ON must have its VALUES pinned in that same file. Anything else silently delegates the QM'd TS's energy reference to whatever Arkane database happens to be installed at solve time, so the TS drifts against the RMG wells around it whenever that database changes -- the exact failure this module family exists to prevent, and one no downstream test would catch. Two states are now refused: * `use_atom_corrections=True` with `atom_energies=None`. This was live and reachable from real ARC projects: `_resolve_atom_energies` may legitimately return None while the flag is True, and `_build_energy_header` emits `atomEnergies` only when it is not None -- so the generated file turned AEC on and pinned nothing. Twelfth fail-open on this branch. * `use_bond_corrections=True`, unconditionally. Arkane's input DSL has no directive to pin BAC values at all (arkane/input.py:670-700: `useBondCorrections` is a bool and `bondCorrectionType` is a KEY selecting a parameterization; `atomEnergies` is the only corrections-VALUES directive that exists), so a hybrid network with BACs on cannot be put on a verifiable energy scale from an input file. This is a limitation of Arkane's input language, not a T3 policy preference, and the error message says so. BACs are applied to transition states, not only thermo species, so the resulting TS-vs-well drift is directly material to a P-dep rate calculation. `use_atom_corrections=False` was already refused, so the only accepted state is now atom corrections on with values pinned, bond corrections off. Fixing this exposed that most of test_hybrid.py's fixtures were constructing exactly the unpinnable state -- `DEFAULT_ENERGY_SETTINGS` had `atom_energies=None`. The fixtures were given realistic ARC-shaped atom energies rather than the guard being weakened, and the end-to-end capture-drives-a-hybrid-write test now threads `atom_energies` from the frozen manifest through to the writer, which it previously dropped entirely. Also fixes the same bug class one line over: `_cross_validate_frequency_scale_factor` used `isinstance(output_value, (int, float))`, so a YAML `freq_scale_factor: true` in output.yml was accepted and frozen as 1.0, silently scaling every frequency by unity. The rest of this module already uses exact `type()` membership for that reason; this site was missed. Round-23 P1 #2 and P2 #5, and the BAC half of round-22 finding #5 (resolved as fail-closed after establishing that freeze-and-verify would require importing Arkane into T3, which is forbidden).
Two gaps left by 6e75abe's staging-and-swap. The swap was not crash-atomic. It renames the existing capture_dir aside and then os.replace's the staged tree into the now-vacant path; process death BETWEEN those two calls leaves capture_dir absent and the only good copy orphaned under a `.capture-old-*` name nothing looked for. The existing test raised an exception the in-process `except` already handled, so it never exercised that window at all. os.replace cannot atomically replace a non-empty directory, so a true single-syscall content swap is not available on POSIX. Instead, `_recover_capture_swap_state` runs at the top of every capture while holding the lock, and restores a valid orphaned `.capture-old-*` when capture_dir is absent or has no valid manifest. It refuses to guess when several ambiguous candidates exist. The guarantee this gives is stated plainly in the code: capture_dir is never permanently lost and self-heals on the next lock-holding call, but this is NOT real-time atomicity -- an outside observer can still transiently see capture_dir absent between the crash and that call. That window is fail-closed (an absent capture reads as "no capture yet", which costs an ARC re-run rather than producing a wrong answer). The crash test now forks a real child that monkeypatches os.rename to os._exit(1) immediately after the real rename completes -- genuine process death, no cleanup, no caught exception. There was also no single-writer guarantee: two T3 processes capturing the same network could interleave staging and swap. `_acquire_capture_lock` now uses os.open(O_CREAT|O_EXCL) with the owning PID written in for diagnosability, released in a finally. A lock is reclaimed only when its recorded PID is confirmed dead via os.kill(pid, 0); a PermissionError (alive under another uid) is treated as NOT stale. Contention fails closed with a loud RuntimeError rather than proceeding or skipping. Round-23 P1 #3 and #4.
`_pdep_capture_is_authoritative` compared only the ARC directory and the (network_id, network_ts_label) key set. So a capture stayed "authoritative" when the RMG network file had been regenerated or the master-equation method had changed, as long as the ARC directory and TS labels were untouched -- and T3 replayed frozen artifacts describing a different calculation, silently attributing the result to inputs that never produced it. Confirmed with a probe before fixing. `_pdep_capture_networks_match_current_join` now compares the manifest's frozen `networks` block against the current join sidecar's on both `source_sha256` and `method`, for every network the capture covers. It fails closed: a block that is missing, not a mapping, or missing an entry on either side counts as NOT matching, never as vacuously matching. A mismatch returns False rather than raising -- unlike the ARC-directory and TS-set checks, where reuse would be ambiguous, a network or method change unambiguously means the capture is stale, so a normal fresh capture should just proceed and overwrite it. Round-23 P1 #1 (and round-22 finding #6).
The frozen energy-settings shape was defined three times -- `_FROZEN_FIELD_TYPES`
(the manifest shape, in energy_settings.py), `_ENERGY_SETTINGS_FIELD_TYPES` (the
writer shape, in hybrid.py), and `QMEnergySettings`'s own dataclass fields -- and had
already drifted: `QMEnergySettings` had no `bond_additivity_corrections` field at all,
so the manifest froze that value and the writer dropped it on the floor, and threading
`atom_energies` from a manifest into the writer was something each caller had to
remember to do by hand.
`QMEnergySettings` is now the single definition. Each field carries its own
`metadata={'types', 'none_allowed', 'in_frozen_manifest'}`, read by two classmethods:
`field_types(frozen=...)` (the one type table, filterable to the fields that live in a
manifest block -- `tunneling` is write-time-only) and `from_frozen(...)` (the one place
a frozen dict becomes a validated object). Both former tables are deleted;
`_validate_energy_settings_types` and `validate_frozen_energy_settings` now consult
the dataclass. Adding a field is one edit instead of three plus tests.
`bond_additivity_corrections` becomes a real field, documented as provenance-only:
Arkane's input DSL cannot pin BAC values, and the writer refuses
`use_bond_corrections=True` unconditionally, so it is never rendered. That is now
explicit rather than a silent drop. The end-to-end capture-drives-a-write test uses
`QMEnergySettings.from_frozen(...)` instead of a hand-rolled seven-field constructor
call, making the manifest-to-writer mapping structural.
Mutation-testing the refactor surfaced two real test-coverage gaps, both now closed:
the `use_atom_corrections=False` refusal was only ever exercised together with the
adjacent `atom_energies is None` refusal, so it could have been deleted with no test
failing; and the fail-closed "missing required frozen key" path had no isolating test
at all. Same guard-composition shape this branch keeps producing, this time in the
tests rather than the code.
No behaviour change. Round-23 P2 #6.
…d TS A capture is supposed to let a replay decide, from the capture alone, whether it is partial and whether the dominant transition state is missing. It could not: the manifest recorded no sensitivity evidence at all. The obvious wiring was rejected earlier because `_capture_pdep_ts_artifacts` also runs on the RESTART path, where `self.pdep_network_selections` is empty in memory -- so reading the evidence at capture time would record it as ABSENT exactly when a capture is replayed. That deferral was wrong (adversarial review rejected it twice: absent evidence in the manifest actively endangers the very decisions the capture exists to support), but the hazard it identified was real. So the evidence is frozen where it is definitely in memory -- at QUEUE time, in `queue_pdep_transition_states`, onto the `TSJoinRecord` -- and flows from there into the durable join sidecar. `_capture_pdep_ts_artifacts` now reads it off the join records, never off `self`, so a restart has it for the same reason it has the records and the networks block: it was persisted, not re-derived. Several `uncertain_path_reactions` entries can share one `ts_label` (a shared TS reached via more than one path reaction, or evaluated under more than one condition); the largest |coefficient| wins, mirroring the selector's own documented ranking convention. Fails closed throughout, per this branch's rule: * `coefficient` and `delta_ln_k` describe one piece of evidence, so supplying exactly one of them is refused rather than half-persisted. * Both must be finite real numbers. `bool` is excluded despite being an `int` subtype, so a stray True/False cannot become a 1/0 sensitivity; NaN/inf are refused because YAML round-trips them but they carry no usable information. * `verify_capture` refuses a manifest entry that has a captured artifact but no valid evidence -- a captured TS with no recorded justification is malformed, not tolerable. * Queueing a TS reported uncertain but with no matching evidence entry raises rather than queueing it silently unjustified. No back-compat shim: a manifest predating this format raises, matching how the `networks` block was introduced. Round-22 finding #8.
The payload of this branch. `T3.process_arc_run()` now writes one hybrid Arkane P-dep
input per network -- QM/RRKM for the transition states whose captured artifact status
is USABLE, RMG/ILT for everything else -- built entirely from the capture directory.
The property that motivates the whole capture layer now holds on the PRODUCTION path,
not just in a hand-built test: the hybrid write succeeds with the ARC project
directory absent entirely. That is guaranteed structurally rather than only tested.
`_write_pdep_hybrid_network_inputs` reads exactly two `self.paths` keys ('PDep
capture' and the new 'PDep hybrid'); every path handed to the writer is joined onto
`capture_dir` from manifest-relative fields, the network source is the VENDORED copy
under `networks/`, the energy settings come from the frozen manifest block via
`QMEnergySettings.from_frozen(...)`, and `qm_artifacts_root=capture_dir` confines
`Log(...)` resolution inside the capture. `self.paths['ARC']` and
`self.paths['RMG PDep']` are never touched.
This also gives `evaluate_pdep_hybrid()` (t3/pdep/discovery.py) its first production
caller -- it was written as the per-network accept/refuse gate and had none, so the
partial-coverage decision was simply not being made. A network whose selected TS set
is not fully usable is now refused as a whole and stays entirely RMG/ILT, logged
loudly, rather than being silently written as a partial hybrid.
The ARC finalization marker is versioned (`ARC_FINALIZATION_MARKER_VERSION = 2`) via
one constant shared by writer and reader so they cannot drift. A legacy unversioned
marker (implicitly v1: written by finalizations that never produced hybrid inputs) and
any future-version marker both read as "not finalized", so finalization is redone
rather than silently skipped. That redo is safe because every step of
process_arc_run() is individually idempotent, each for its own reason -- not, as an
earlier draft of this message claimed, because of the authoritative-capture guard,
which only ever covered the capture step. The step that redo actually re-executes on
already-appended results is the RMG library append, and it is idempotent because
append_to_rmg_library() skips any source entry whose label is already present in the
destination. That was asserted rather than verified, so it is now tested directly, on
both the merge path and the create-then-redo path that a first iteration takes, for
thermo and kinetics alike. The trailing parenthesis in '(v2)' keeps the prefix check
collision-proof against a future '(v20)'.
This test has been dead on this branch. It hardcoded a 'T3/Projects/...' prefix, so it failed on the very first key in any worktree not literally named 'T3' (here 'T3-pdep-qm') -- long before reaching anything it was actually meant to check. Drop the worktree-name component from the expected substrings. Reviving it exposed what it had been unable to report: its expected dict never gained the 'PDep capture', 'PDep hybrid' or 'ARC finalization marker' keys added by this branch. Since the loop iterates the LIVE `t3.paths`, a key present in `set_paths()` but absent here would have surfaced as a bare KeyError. Add the three keys, and assert the key sets match up front so future drift reports which side is missing what instead of dying in a dict lookup.
_write_pdep_hybrid_network_inputs() never removed hybrid outputs left by a PRIOR call over the same iteration -- reachable in production because an unversioned or future-version finalization marker forces process_arc_run() to redo finalization (capture + hybrid write) over a directory that may already carry outputs from an earlier pass. Three leaks followed from this: a network refused on this pass (its selected TS set no longer fully usable) left the earlier pass's input.py in place; a network dropped from the manifest entirely between passes was never revisited because the write loop only iterates networks the CURRENT manifest references; and the zero-captured-artifact early return skipped straight to `return dict()`, leaving the whole prior 'PDep hybrid' tree untouched. Any of the three lets a downstream consumer read a hybrid QM/RRKM input for a network T3 no longer stands behind on this pass. Fixed with a new _prune_stale_pdep_hybrid_outputs() helper that removes every per-network directory under 'PDep hybrid' not in the set of network_ids accepted THIS pass. It is called from both the zero-artifact early return (with an empty accepted set, since nothing is being hybridized) and the main body, where the accept/refuse verdict for every network is now evaluated up front so pruning can happen BEFORE the write loop -- reversing that order would risk a refused network's stale directory surviving a raise partway through the loop, or an accepted network's fresh write being deleted by a prune that runs too late. The helper only ever joins names returned by os.listdir() onto the hybrid root -- never a manifest-supplied network_id -- so it is confinement-safe by construction, independent of the network_id confinement fix that follows in the next commit. It also fails closed on anything under the hybrid root that is not a plain directory (a file or a symlink), since that is not something this code would ever have written there itself. Tests added to TestWritePdepHybridNetworkInputsPruning in test_main_wiring.py cover all three leaks plus a no-over-deletion-regression check that an accepted network's output still gets written.
_write_pdep_hybrid_network_inputs() built dest_path with os.path.join(self.paths['PDep hybrid'], network_id, 'input.py') using network_id straight from the capture manifest -- untrusted for use as a raw filesystem path segment even though its CONTENTS are otherwise trusted. A manifest carrying network_id='../escaped' walks the write out of the hybrid root via os.path.join() join semantics; a manifest carrying an absolute network_id causes os.path.join() to discard the hybrid root entirely and write wherever the absolute path points. Both are demonstrated by the two new tests, which showed T3 actually writing "Wrote a hybrid P-dep network input for '<escaped path>'" to a location outside 'PDep hybrid' before this fix (confirmed via captured stdout, not merely a raised exception under mutation). Fixed with a realpath()+commonpath() confinement check immediately before dest_path is built, following the codebase's existing (deliberately per-module-duplicated) confinement idiom used in capture.py's _confine_to_capture_dir, discovery.py's _confine_to_project, and the inline checks in hybrid.py and rmg_runner.py. It deliberately does NOT reuse capture.py's private _confine_to_capture_dir (its error message talks about "capture directory" / "verify", which is a non-sequitur in this write path) and does NOT introduce a new shared helper (the codebase has never had one for this idiom -- four independent inline implementations already exist, and centralizing now for a single new caller would break that established pattern for no clear benefit). str.startswith() is deliberately avoided in favor of commonpath(), since a sibling directory whose name is a prefix of the hybrid root's name could defeat a naive startswith() check. t3/pdep/join.py's validate_networks_block (around line 406) was checked and does NOT need a matching guard: network_id there is used only as a dict key and interpolated into error-message text, never joined into a filesystem path. Tests added to TestWritePdepHybridNetworkInputsPruning in test_main_wiring.py cover both the '..'-relative and absolute-path escape vectors. The confinement is applied to every network_id in the manifest UP FRONT, before _prune_stale_pdep_hybrid_outputs() runs, rather than at the write site. Pruning is destructive and is driven by these same ids -- a directory whose name is not among them is removed -- so validating only at the write site let a manifest T3 was about to refuse still delete a previous pass's legitimate outputs on its way to being rejected. That failed closed (it raised, and no finalization marker was written), but a manifest that fails validation should not get to touch the output tree at all. Confirmed by test: with the check at the write site, the escaping-network_id test raised the correct ValueError and yet network4_2/input.py was already gone. The check itself is factored into _confine_to_pdep_hybrid_root(), local to main.py for the same reason capture.py and discovery.py each carry their own: capture.py's helper reports failures in terms of a capture directory and verification, which is a non-sequitur on this write path.
…ponent _confine_to_pdep_hybrid_root()'s realpath+commonpath check lets a network_id containing a path separator (e.g. 'a/b') resolve under the hybrid root and pass, but _prune_stale_pdep_hybrid_outputs() compares raw accepted network_ids against TOP-LEVEL os.listdir() names, so 'a/b' and a top-level 'a' entry collide there. Require network_id to be a single, safe filename component (not empty, '.', '..', absolute, or containing a path separator) before any path is ever built from it, closing the collision at its source.
…output _prune_stale_pdep_hybrid_outputs() previously validated and deleted entries in the same os.listdir() pass: a stale directory could be rmtree'd before a later entry was found to be unacceptable and raised, leaving a half-pruned tree behind. It also never checked that an ACCEPTED network's own destination under 'PDep hybrid' was still a plain directory (or absent) before writing into it -- a plain file or symlink left there (e.g. by external interference) would only be discovered after other, unrelated stale directories had already been deleted. Make pruning a genuine two-pass preflight-then-delete: first classify every entry under the hybrid root (accepted-and-a-plain-directory-or-absent is fine; anything else -- including a non-directory/symlink placed where an accepted network's own output belongs -- raises immediately with nothing yet removed), then delete only the entries confirmed stale in the first pass. Adds two tests under TestWritePdepHybridNetworkInputsPruning: - test_an_accepted_networks_destination_that_is_a_plain_file_is_refused - test_a_pre_existing_stale_directory_survives_when_a_later_entry_makes_pruning_raise Verified: -k "TestWritePdepHybridNetworkInputsPruning" -> 9 passed, 53 deselected in 226.72s.
… status/path consistency Two related defects in the capture re-verification boundary: 1. verify_capture() counted captured_artifact_count purely off "is captured_artifact_path non-None", never checking it against status. A manifest entry hand-edited (or corrupted) to status: usable with a null captured_artifact_path verified cleanly and then silently vanished from every downstream count, even though nothing had actually been captured for it. verify_capture() now fails closed: an unrecognized/missing status raises, a status in TS_ARTIFACT_STATUSES_REQUIRING_ARTIFACT_PATH (usable, unverified) with a null path raises, and a status in TS_ARTIFACT_STATUSES_REQUIRING_NO_ARTIFACT_PATH (missing, unusable, not_queued) with a non-null path raises. The two frozensets live in t3.pdep.discovery, read directly off discover_ts_artifacts()'s own branches, so verify_capture enforces exactly the pairing discovery itself would have produced -- not a second, independently-invented rule. 2. The hybrid-input writer (t3.main._write_pdep_hybrid_network_inputs) re-read the manifest itself after verify_capture() had already read and validated it. A second, untrusted read can observe a manifest mutated out from under the first read (e.g. a concurrent re-capture), reintroducing a check-then-use race that verify_capture()'s own re-verification exists to close. VerifyResult now carries a new ts_records: tuple field -- one TSArtifactRecord per transition-state manifest entry, built from exactly the fields verify_capture() itself just validated, with artifact_path already resolved to an absolute, confined path. The writer (fixed in a following commit) consumes this directly instead of re-reading the manifest. Adds 5 tests to tests/test_pdep/test_capture.py: - test_verify_capture_returns_the_verified_ts_records_for_the_writer_to_consume - test_verify_capture_refuses_a_usable_status_with_no_captured_artifact_path - test_verify_capture_refuses_a_non_null_captured_artifact_path_on_a_status_forbidding_one - test_verify_capture_refuses_an_unrecognized_status - test_verify_capture_refuses_a_missing_status_field Verified: tests/test_pdep/test_capture.py -> 82 passed in 1.60s.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…nguishable Round-41 adversarial review of the T-grid clamp (5555c56, be177e1). Four findings. A. The Tlist-drop equivalence claim was backed by an unrepresentative file. The commit verified the dropped Tlist against Arkane's 1/T-linspace branch, using the one network I happened to run -- which is interpolationModel 'pdeparrhenius'. But 300/300 real RMG network files are ('Chebyshev', N, M), where generate_T_list() takes a completely different branch (Gauss-Chebyshev nodes). So the evidence covered ~0% of the real population. Re-probed: the claim does hold for Chebyshev too -- a real Chebyshev network (Tmin=300, Tmax=5100, Tcount=10) has a Tlist equal to Arkane's Gauss-Chebyshev nodes to 0.005 K, which is written-file rounding. The fix is branch-agnostic BY CONSTRUCTION: it preserves interpolationModel and lets Arkane pick the family itself, so it cannot pick the wrong one. That property, not my evidence, is what made it correct. Now pinned by a test that asserts both the Tlist drop AND the survival of the interpolationModel line, with the node formula in the docstring. B. network_thermo_t_max() lied by omission: the docstring promised the ceiling at which EVERY species has valid thermo, while silently skipping species with missing, non-NASA, non-Kelvin or non-literal thermo. If a skipped species is the limiting one, it returns a too-high ceiling and the clamp under-protects -- partial evidence presented as a network-wide guarantee. Now returns a NetworkThermoCeiling carrying what was skipped; both writers log a WARNING naming the count and labels. Deliberately NOT fail-closed: exotic thermo is common and harmless, and over-refusal is this branch's recurring failure mode. Visibility is the fix, not refusal. C. The hybrid path fail-opened on any ValueError. A blanket `except ValueError` meant every error network_thermo_t_max could raise silently disabled BOTH the clamp and the Tlist drop. Only the "text does not parse" case should degrade (so the pipeline's own ast.parse self-check reports corruption); everything else is a defect signal and must propagate. Now raises and catches a purpose-named NetworkTextUnparseable rather than matching on message text. Worth recording HOW this was found: the surrounding comment already described the narrowed behaviour, and a previous report claimed it was implemented. It was not -- the comment described an intent the code never had. It surfaced only because a test written to pin already-"fixed" behaviour failed against the supposedly-correct code. A comment is not evidence that the code does what it says. D. Tlist parsing assumed a single-line tuple of Kelvin numbers. Now validates the parsed shape before comparing and refuses with a message naming the file rather than an opaque IndexError/TypeError -- but only when a clamp is in play; an odd Tlist on a network needing no clamp is not this code's business. Robustness, not a live bug: 0/300 real files are multiline or non-K. Mutation-tested throughout; the mutation for C is what exposed C. tests/test_pdep + tests/test_utils: 1162 passed.
Comment on lines
+25
to
+28
| from t3.utils.network_thermo import (NetworkTextUnparseable, # noqa: F401 | ||
| NetworkThermoCeiling, # noqa: F401 | ||
| format_skipped_species, # noqa: F401 | ||
| network_thermo_t_max) # noqa: F401 |
…cord
Round-41 finding: the T-grid clamp was recorded only as a logger.warning. The
sidecar already carried the network hash, SA hash, method and perturbation, but
nothing durable said the sensitivity analysis had been computed over a NARROWED
temperature domain. A saved PDepNetworkSelection could not be told apart from
one resting on the network's original grid -- and a coefficient measured over
[700, 3000] K is not the same measurement as one over [700, 3200] K. A log line
is gone the moment the run ends, so this violated the rule that a decision's
evidence must be reconstructible from the persisted record.
New TGridClampRecord (t3/utils/network_thermo.py) carries the requested Tmax,
the thermo ceiling, the Tmax actually written, whether the explicit Tlist was
dropped and its original highest entry, and which species were skipped when
computing the ceiling. The writers emit it, write_sa_cache_metadata persists it
in the SA sidecar, and it rides on PDepNetworkSelection through the YAML round
trip (dict-shaped, no tuples, per the settled round-trip constraint).
The distinction that matters is THREE-way, not two:
None -> provenance UNKNOWN (sidecar predates this, or
the SA was produced outside T3)
{'clamped': False, ...} -> positively known NOT to have been clamped
{'clamped': True, ...} -> clamped, with both the requested and the
written Tmax recoverable
Collapsing "unknown" into "not clamped" would silently reintroduce the exact
defect being fixed, so absence is never read as a negative. Unknown provenance is
a disclosure, not a disqualification: it does NOT make a decision not_evaluated
and does NOT refuse, since over-refusal is this branch's recurring failure mode.
SA_CACHE_CONTRACT_VERSION deliberately stays 1. The key is purely additive: a
sidecar written before this change is not MISREAD by the new reader, it simply
reads as unknown, which is a true statement about it. A bump would wrongly
invalidate every existing cache to record something no old cache ever claimed.
tests/test_pdep + tests/test_utils: 1185 passed.
tests/test_pdep/test_main_wiring.py: 66 passed (28 min).
Round-41 finding #1. A persisted PDepNetworkSelection decides whether T3 spends expensive QM on a network. `_selection_from_dict` type/enum-validated each field independently, so a hand-edited record with valid bools and enums, a matching hash and method, and empty or fabricated evidence still passed `explore_pdep_network`'s gate on `selection.qualified`. 5823c1c added a guard for the empty-evidence case in `reason()`, but that only makes the human-readable prose honest -- the record still gated exploration. Rejection belongs at load, where the record enters the system. Two invariants, each derived by reading the constructors rather than assumed: qualified=True requires non-empty uncertain_path_reactions select_from_sa_dict sets `qualified = bool(uncertain_path_reactions)`, and combine() unions qualified over evaluated components -- whose own evidence is part of the unioned result. No path produces a qualified record with no evidence. every uncertain_path_reactions entry is marked uncertain AND appears in selected_ts select_from_sa_dict builds it as `[e for e in selected_ts if e.uncertain]`, a filtered subset by construction; combine() only unions matching lists. Two candidate invariants were deliberately NOT enforced, because reading combine() shows real records violate them: qualified == bool(uncertain_path_reactions) as full equality -- combine() legitimately yields qualified=False with non-empty evidence, since it unions uncertain_path_reactions across ALL components but unions qualified over EVALUATED ones only, so a not-evaluated component's evidence rides along on a negative decision. any evaluation_status cross-check -- `qualified=True` with `not_evaluated` is the PARTIAL YES, which combine() constructs when one component qualified and another could not be evaluated. The branch's asymmetry is that a partial yes is supported; refusing it would be over-refusal, this branch's recurring failure mode and as real a defect as the hole being closed. Both over-refusal cases are pinned by tests built through the REAL constructors (select_from_sa_dict against the network4_2 fixture, and combine()), not by hand-written fixtures that would merely agree with the validator. The third invariant's mutation initially killed no test -- the gap was closed with a test rather than left standing. tests/test_pdep + tests/test_utils: 1191 passed.
Queueing a P-dep network for QM refinement was an immediate, per-network
decision taken inside determine_species_from_pdep_network()'s loop: the
`if selection.qualified:` branch queued the network's uncertain transition
states on the spot. Whether a network is worth its QM cost is a question
about the whole field of candidates, so it cannot be answered while
candidates are still being discovered -- ranking networks against one
another is impossible from inside that branch.
The branch now records a frozen PDepQueueCandidate instead, carrying the
three loop-local values the queueing step needs and that do not otherwise
survive the loop (the parsed network, the Arkane structures block, and the
file the network was parsed from). A second pass after the loop does the
queueing, in the order the candidates were found. No budget or ranking is
applied yet; every qualified network is still queued, so which networks are
refined is unchanged.
This is NOT a pure no-op, and the difference is worth stating plainly.
queue_pdep_transition_states() calls add_reaction(), which cascades into
add_species() for any of the reaction's species T3 does not already know.
The sensitive-well branch of the same loop also calls add_species(). So
deferring the queueing reorders species-key assignment between those two
paths, and since add_species() assigns `key = len(self.species)` and
get_species_with_qm_label() derives the ARC-facing QM label from that key,
species keys and QM labels differ from what the old interleaving produced.
Consequently species.yml, reactions.yml and restart state are not
bit-for-bit comparable across this change. No work is lost: the SET of
species queued for QM is the same, and every one of them is still computed.
One consequence is a contract question rather than mere numbering. A
species reachable from both paths gets a key back from whichever path
reaches it first and None from the second, and only the well path appends
to the returned species_keys. With queueing deferred, the well analysis now
always reaches such a species first, so a species justified by a sensitive
well is always reported -- which is the intended reading of a return value
documented as "species determined to be calculated based on SA". The second
test below pins that deliberately.
Both new tests were mutation-verified against the same mutation, putting
the queue call back inside the loop:
- test_queueing_is_deferred_until_every_network_has_been_evaluated
explores two entries and asserts every queue call saw both selections
already recorded ([2, 2]; the mutation yields [1, 2]).
- test_a_species_justified_by_a_sensitive_well_is_reported_even_when_a_
queued_ts_also_needs_it fails under the mutation because the queued
TS1 reaction claims C4rad(5) first.
Green: 1191 (tests/test_pdep + tests/test_utils, ignoring test_flux.py and
test_main_wiring.py), 67 (test_main_wiring.py, 29m47s), and
tests/test_main.py -k pdep.
Whether a network is worth its quantum chemistry is a question about the
whole field of candidates, and the previous answer was a per-network gate:
every qualified network was refined, however many there were and however
much they cost. This adds the other half -- rank the qualified networks
against one another and refine as many as the configured budget allows,
most deserving first.
Two knobs, both defaulting to None (no limit), so the budget is opt-in and
setting nothing refines exactly the networks T3 refined before:
t3.sensitivity.pdep_QM_max_transition_states -- the primary bound, since
ARC jobs are per-species and per-transition-state;
t3.sensitivity.pdep_QM_max_networks -- a secondary cap on whole networks.
Both are strict ints: bool is an int subclass, so without that a YAML
`pdep_QM_max_networks: true` would validate as 1 and silently cap the run
at a single network per iteration.
The ranking already existed in t3.pdep.api.rank_pdep_networks as a local
_rank_key. It is now t3.pdep.selector.selection_rank_key, so the in-run
queueing and the public ranking API cannot drift apart into two different
ideas of "most deserving".
t3/pdep/budget.py is pure: it reads decisions and returns indices. That is
not tidiness. Projecting a network's cost by actually queueing it would
mutate T3's species, reactions and QM state, which is precisely what a
budget has to be able to decide against before committing to it. Three
further properties are load-bearing and are argued in the module docstring:
It never slices a network. uncertain_ts_labels() is sorted by LABEL, not
by sensitivity, so taking "the first k" of them would choose quantum
chemistry alphabetically. A network that does not fit is skipped whole
and the walk continues, so a smaller, lower-ranked network can still be
admitted rather than leaving budget unspent.
Its cost is an upper bound. Some uncertain transition states will not
become ARC jobs (unsafe label, missing structures, a shared transition
state, a reaction T3 already knows), and they are charged anyway, so the
budget can only over-estimate the spend it authorizes.
A network offered several times in one iteration -- the normal case,
since several sensitive reactions can belong to one network -- is charged
once, for the UNION of what its offers name, and ranked by the best of
them. Two offers are not one decision reached twice: each answers "which
transition states is THIS observable reaction's rate sensitive to?", so
they legitimately name different transition states, and queueing both
produces their union.
A network no offer could evaluate is refused before it is ranked. It is not
a cheap candidate, it is an absent measurement: there is nothing to rank it
by and nothing to justify spending on it. A network whose cost exceeds the
entire budget is refused with a distinct reason naming the limit to raise,
because no remaining budget is ever larger than the whole of it and no
later iteration will change that on its own -- without that message it
would simply starve, quietly, forever.
Nothing is dropped silently. Every refusal is recorded in the returned
budget decision and logged as a WARNING naming the network, its cost and the bound that refused
it, because a run that quietly shrank its own work would be
indistinguishable in the logs from a run that had nothing left to do.
Note what this commit does NOT do. main.py still offers only QUALIFIED
networks to the budget, so the hard gate survives one level up, and
explore_pdep_network still refuses an evaluated-but-unqualified network
outright. Reframing that gate is a public-API change and is deliberately
left to its own commit. Also, a budget refusal is durable only in the log,
not on the persisted selection; recording it there bumps the selection
schema version and is likewise left separate.
Ten mutations were applied and all ten killed: charge the first offer
instead of the union; rank by the first offer instead of the best; group
unnamed selections together; delete the not-evaluated refusal; delete the
exceeds-the-whole-budget refusal; drop strict=True; hardcode the knobs to
None at the call site; queue every candidate ignoring admitted_indices;
drop the sort; turn skip-and-continue into stop-at-first-miss.
Green: 1247 (tests/test_pdep + tests/test_utils + tests/test_schema,
ignoring test_flux.py and test_main_wiring.py) and 70 (test_main_wiring.py,
30m23s).
…tion Third and last part of the gate -> ranking reframe. 354476f gave T3's in-run queueing the whole field to decide with; e43349d ranked that field against a budget. Both stopped at T3's own wiring: explore_pdep_network(), the public entry point, still treated selection.qualified as the sole admission authority, so a caller that had ranked a network and chosen to spend on it had no way to say so. It now takes a keyword-only admission_policy. Under 'caller_admitted' BOTH qualification checks stand aside -- the unqualified skip and the not-evaluated raise -- because both are about using `qualified` AS A GATE, and a caller that admitted the network elsewhere is not using it as one. Under the default, 'qualified_selection', nothing changes: with no external admission there is no positive evidence and no spend decision, so a missing evaluation is still a missing verdict rather than a negative one. What does NOT stand aside is provenance. method, network_id and network_source_hash stay unconditional, and evaluation_status is now checked for validity unconditionally too -- admitting a network is a budget statement, and no budget statement makes a stale or unreadable decision current. That is the whole difference from selection=None, which drops the binding along with the gate. Keyword-only because logger is the fourth positional parameter and an argument inserted before it would land on callers' logger. The result records what admitted it, since a 'succeeded' result carrying an unqualified selection is otherwise indistinguishable from a bypassed gate. A third value, 'ungated', covers selection=None: recording the argument default there would have the record assert that a qualified selection admitted a run for which no selection existed -- a false provenance claim, and the one an auditor of an expensive QM run would lean on. It is derived rather than requested, so a caller cannot claim 'ungated' while passing a selection, and 'skipped' is cross-checked against it because only the gate can decline a run. A results file written before this field is DERIVED, not refused and not blanket-defaulted: nothing predating the field could have been caller-admitted, so a record with a selection was gate-admitted and one without was ungated. That reconstructs the true value instead of guessing, and keeps EXPLORATION_RESULT_SCHEMA_VERSION 1 meaning one loadable shape. Public API change; PR #179's description needs it. T3's own path does not use the new policy yet -- t3.main still offers only qualified selections to the budget -- so in-run behaviour is unchanged. Also de-drifts prose that still called this a budget gate, including two claims in selector.py that stated flatly that explore_pdep_network refuses a selection that did not qualify. Verified: 1264 fast tests, and 70 in test_main_wiring.py against this exact tree. Fourteen mutations applied, fourteen killed, all against the final tree: the skip ignores the policy; the raise ignores it; the hash check becomes policy-conditional; the evaluation_status check becomes policy-conditional; the recorded value echoes the argument instead of being derived; 'ungated' becomes requestable; the loader defaults, or blanket-defaults, instead of deriving; the succeeded and failed sites drop the field; an unknown policy falls back to the default; caller_admitted with no selection is allowed as a no-op; the result skips policy validation; the skipped cross-check is removed; as_dict drops the field. Codex round 44 rejected the bare require_qualified bool this started as -- a flag naming what is not enforced hides what did admit the run -- and round 45 caught the false 'qualified_selection' on the selection-less path.
Member
Author
Update: the per-network qualification GATE is now a budget-limited RANKINGThree commits implement the reframe you asked for. Still DO NOT MERGE.
Two things to look at specifically. Behaviour is NOT preserved by
Design points worth your disagreement:
Verification: 1264 fast tests, 70 in |
Comment on lines
+40
to
+49
| from t3.pdep.explorer.result import (ADMISSION_POLICY_CALLER_ADMITTED, | ||
| ADMISSION_POLICY_QUALIFIED_SELECTION, | ||
| ADMISSION_POLICY_UNGATED, | ||
| EXPLORATION_RESULT_SCHEMA_VERSION, | ||
| EXPLORATION_STATUS_FAILED, | ||
| EXPLORATION_STATUS_SKIPPED, | ||
| EXPLORATION_STATUS_SUCCEEDED, | ||
| PDepExplorationResult, | ||
| VALID_ADMISSION_POLICIES, | ||
| ) |
A network that qualified for QM refinement and then went unrefined because the budget was spent left nothing behind but two log lines. That is the one outcome a long unattended campaign most needs to be able to reconstruct afterwards, and it was the only decision in this package that vanished with the process. The record covers admitted and refused networks alike, not refusals alone. Recording only refusals would collapse "no file", "empty file", "nothing was refused", "nothing was a candidate" and "no budget was configured" into a single ambiguous silence, and a reader could not tell a network that was admitted from one that was never considered. Refusal reasons gain stable codes alongside their prose. The prose is written to be read by a human in a log and is free to be reworded; anything that has to branch on why a network was refused needs a key that is not a sentence. The decision now carries the walk it already made -- the ranked identities, their offers, costs and remaining budget -- and the record is built from that alone. Deriving them a second time from the selections would have meant two independently maintained copies of the same grouping and ranking, and on drift the failure is not a crash but a record that confidently attributes the wrong refusal to the wrong network. Since the builder no longer takes the selections, handing it a decision and a field that never met is now unconstructible rather than merely unlikely. A network the budget identifies positionally because it has no id is recorded that way too, rather than as a repeated empty id the record then rejects; the budget accepts such networks, so refusing to describe them would have made the record fail on input the decision itself allows. Both a schema version and an algorithm version, following the split already in selector.py: this record's shape can stay fixed while what the budget means by ranking and refusal changes, and a reader has to be able to tell those apart.
Member
Author
Increment 32 — a budget refusal is now a durable record (2 commits, pushed)
Until now, a network that qualified for QM refinement and then went unrefined because the budget was The plan I inherited for this was built on a false premise, and probing killed it. It said to Reusing the existing TS-join sidecar was considered and rejected on review, for a reason I had not What landed instead: a Two defects worth naming, both found before the tests were green:
The builder originally re-derived the budget's grouping, ranking and remaining-budget bookkeeping. Verification. Fast suite 1317 passed; One caveat worth flagging: the record has no in-repo consumer today. Its intended reader is human Still DO NOT MERGE — this is for your review. |
| network_id = offers[0].network_id | ||
| cost = len({ts_label for selection in offers for ts_label in selection.uncertain_ts_labels()}) | ||
| remaining_before = remaining | ||
| unnamed_offer_index = indices[0] if not network_id else None |
The record type existed but nothing wrote it, and a save/load pair with no production caller is not durability -- it is the same silence in a different place, which is precisely why writing the outcome onto the selection was rejected: T3 never persists selections either. So t3/main.py now writes the record itself, into the iteration directory, alongside the other per-iteration sidecars. It is written whenever the budget runs, including when nothing was refused and when there was nothing to consider at all, so that the absence of the file carries a meaning that is actually true: the feature was off this iteration, not that it had nothing to say. For that to stay true across a re-run, a record left by an earlier run of the same iteration is cleared on the path where the feature is off, the way a superseded ARC finalization marker already is -- otherwise a stale file would go on describing a decision this run never took. It is written before the queueing loop rather than after. The record describes what the budget decided, not what queueing later managed to do, so it should outlive a crash in the step it authorizes. That only holds if the file is never half-written, since a truncated record can still parse and still validate while under-reporting what was refused, so it is staged beside its destination and moved into place, as the capture manifest already is. Replacing the path rather than writing through it also means a symlink sitting where the record belongs is replaced instead of followed. The loader is strict in the manner of the two loaders already here: an unversioned file, a version it does not recognize, a malformed envelope and a malformed record are all refused rather than guessed at. Versions are compared as integers, since Python would otherwise read `true` as version 1. The record carries its own schema and algorithm versions and is never nested inside another persisted structure, so those fields serve as the file's version markers and no second envelope-level copy is introduced to disagree with them. One existing wiring test had hand-built a budget decision that reported a refusal while carrying no account of it. That is exactly the incoherence the record refuses to render, so the test now builds a decision that is consistent with itself.
Member
Author
Follow-up on the comment above — adversarial review of the write path found six defects
Two of the six are worth calling out, because they were real and neither was caught by a green suite: The write followed a symlink. A symlink sitting at The write was not atomic, and a truncated record still validates. A partial file can parse and The other four: version gates compared with Verification. Fast suite 1324 passed; One disclosure: the Still DO NOT MERGE — for your review. |
…ysis The SA sidecar has always had somewhere to put the provenance of the RMG-Py that produced the sensitivity coefficients -- write_sa_cache_metadata accepts rmg_py_path and stores it alongside get_git_commit()'s answer. Nothing ever passed it. The only production caller, in determine_species_from_pdep_network, omitted the argument, so every sidecar a real T3 run has ever written records rmg_py_path: null and rmg_py_commit: null. The field was not unreliable; it was dead. That is not a cosmetic gap. A real PDep trial run was carried out against an Arkane predating the fix that makes sensitivity meaningful for ILT-based path reactions, so its transition-state coefficients were numerical noise nine orders of magnitude below the selection floor. The run looked merely negative rather than invalid, and it stayed that way for a week -- because the one field that would have identified the Arkane it came from was empty. Provenance earns its keep only when something fills it in. Resolve the path from sys.modules['arkane'] rather than from configuration. Arkane runs in-process (rmg_runner imports arc.statmech.arkane and calls it), so after a successful job the interpreter already holds the exact module that ran; its __file__ names the checkout with no guessing and no second source of truth to drift. A dict lookup is not an import, so T3 still never imports arkane or rmgpy. When Arkane has not run in this process there is nothing truthful to say and the answer is None, which is what the sidecar recorded before and what it will go on recording -- the difference is that null now means "no Arkane ran" instead of "nobody asked". The commit is deliberately recording-only. Rejecting a cache whose commit no longer matches the checked-out RMG-Py is a tempting next step and the wrong one: it would invalidate every cached network on any RMG-Py commit, including commits touching nothing this code depends on, and mass re-running master-equation jobs to satisfy a hash is the over-refusal failure this package keeps having to resist. A human reading the sidecar is the consumer here, as with the budget record. The test that matters is the wiring one. The fast suite passes with the new call-site argument deleted -- 1329 tests, all green, against code that had silently resumed writing nulls -- because the existing coverage exercises write_sa_cache_metadata directly and never the caller that was broken. The added test drives determine_species_from_pdep_network down the Arkane-succeeded path with a stub arkane module installed, and fails on that deletion.
…s process The previous commit resolved the RMG-Py behind a sensitivity analysis by looking up sys.modules['arkane'], on the stated premise that Arkane runs in-process. That premise is wrong. ARC's run_arkane builds a bash script and invokes micromamba run -n rmg_env python -m arkane input.py, so the work happens in a subprocess in a different conda environment, and nothing of the RMG-Py that did it is ever loaded here. T3 cannot import it either, by design and in practice alike. The resolver therefore returned None on every real run, and the sidecar went on recording the same null it recorded before -- the fix was ineffective rather than harmful, but it was ineffective everywhere it mattered. The mistake was not visible from the call site. rmg_runner imports run_arkane from arc.statmech.arkane, which reads as an in-process call and is in fact a wrapper around a shell-out. The tests did not catch it because they installed a stub arkane module in sys.modules and then confirmed that the mechanism worked against the stub. Mutation testing did not catch it either: deleting the argument at the call site does fail the test, so the wiring was genuinely covered. What went untested was the premise underneath the wiring, which no amount of testing the caller can reach. Arkane records its own provenance in the log it writes into the output directory T3 already owns, which makes that log the only first-hand witness to what ran. Read the commit from there. The value sits on the line after its label, and the line after that is a commit date, so only the first following non-empty line is considered and only if it actually looks like a hash; recording an arbitrary log line would be worse than recording nothing, since it would read as real provenance to whoever audits the sidecar later. write_sa_cache_metadata takes the commit directly now, keeping the derive-from-a-checkout path as the fallback for callers holding a checkout rather than a log. Checked against the runs themselves rather than only against fixtures. The two networks re-run today report e720866a, the fork tip carrying both ILT sensitivity fixes. The trial whose transition-state coefficients were nine orders of magnitude below the selection floor reports dfc4df86, which predates both. That is the distinction the field existed to make and had never once made.
The gate had never been tested against data it did not generate. Vendor two complete unmodified chains from a real run and assert what each decision MEANS, not just whether it qualified. The pair is chosen because it disagrees. network21_1 (CSE) responds five times more strongly than network799_1 (MSC) and still does not qualify, because all three of its path reactions come from a reaction library and none of its kinetics are uncertain; network799_1 qualifies with every one of its selected transition states uncertain. Qualification is sensitivity TIMES uncertainty, and a gate that ranked on sensitivity alone would invert both verdicts. A hand-built fixture would almost certainly make the two agree and would pass against that broken gate. Assertions are on the semantics -- the (transition state, path reaction, uncertainty) triples, the counts, the thresholds, the cache status, the absence of warnings -- because the boolean alone is nearly worthless here. Mutation testing, applied to production code and reverted: qualified ignores uncertainty killed by 2 tests absolute coefficient floor removed killed by 5 tests, BOTH BOOLEANS UNCHANGED path-reaction join shifted by one killed by 2 tests, BOTH BOOLEANS UNCHANGED reaction-library certainty marker removed killed by 2 tests The two mutations that leave `qualified` untouched are the argument for this shape: a fixture asserting only the verdict would have shipped green against both. The floor assertions are arithmetic rather than vibes: the raw SA files offer 36 transition-state rows each, of which 7 and 16 fall below the floor, and the selector keeps exactly 29 and 20. What survived is what was offered minus what the floor excluded, so a gate discarding rows for some other reason breaks the equality even if the survivor count coincides. Provenance is recorded rather than asserted into existence. Both arkane.logs name RMG-Py e720866ae, which carries the two ILT sensitivity fixes; without them these coefficients would be noise nine orders below the floor, which is exactly how an earlier experiment looked merely negative for a week. The t3_sa_cache.yml sidecars still record rmg_py_commit: null because they predate 2c91568 -- that null is now asserted by a test so nobody "fixes" it by inventing a commit. Nothing binds a log to the SA file beside it, and the test says so instead of overclaiming. A method-level branch of the uncertainty predicate survives mutation. That is structural, not a gap in the fixture: selector.py:1082 passes only kinetics_comment, and the network parser records kinetics_type (the callee name), never a kinetics method -- so no real-network fixture can reach it. Left to unit tests, where it belongs. Test-only; no production code changed. Fast suite 1331 -> 1345.
… qualified
Until now the only in-run trace of a PDep network's fate was appended at one site deep
inside the success path, so every fail-closed exit before it left the network with no
record at all. On a real 12-network trial that meant 7 networks -- the majority -- vanished
silently while the log's "3 qualified" read like an unqualified success. Fail-closed was
and remains correct; the defect is that refusing to answer looked identical to never having
been asked.
This is the first half of that work: the record type and the diagnosis that populates it.
The main.py funnel that produces the records follows separately.
PDepNetworkAssessment is a new OUTER type rather than a reused PDepNetworkSelection.
A selection is rankable, and selection_rank_key sorts a not_evaluated one into tier 1,
ahead of evaluated negatives -- right for a selection, wrong for a placeholder, since it
would make "we never found out" outrank "we checked and it does not qualify" for every
future consumer. The selection is kept as an optional NESTED payload instead.
The record's load-bearing property is that it cannot disagree with itself. Each of the 21
reason codes implies exactly one status and one rule about the nested selection, and
construction refuses any combination that violates either. A durable file is believed, so
one claiming a selector verdict for a network whose SA never parsed would mislead with the
full authority of provenance.
The vocabulary lives in its own module with no t3 imports. It is shared by a producer (the
selector, reporting which refusal site fired) and a consumer (the assessment, persisting
it), and putting it in either would force a dependency between them -- with the codes in
assessment, selector had to import persistence, which in turn stopped assessment from
importing PDepNetworkSelection to type-check its own nested payload. That left `selection`
typed as `object` and duck-checked, so any value at all passed construction and only failed
later at serialization. The neutral module lets the dependency run selector -> reason_codes
<- assessment, and the nested payload is now type-checked.
Two malformed-payload codes exist because one could not serve both callers: T3's funnel
inspects `structures` before it can build a network reaction string, so a non-mapping SA
payload is caught there and never reaches the selector, while the selector's public entry
points can be handed one directly. A single code would have forced the funnel to either
fabricate a selection or violate the type.
An internal error gets its own status rather than sharing not_evaluated. It is a bug, not
an outcome, and counting it alongside legitimately unevaluable networks would make a T3
defect read as the ordinary cost of doing business. It is also the one category for which a
nested selection is optional -- a crash after the selector returned should keep the evidence
already obtained.
Where two selector refusals both apply they are now BOTH recorded. Discarded rows and an
unassessed transition-state provenance are independent defects in the evidence -- a
transition state can fail to join a path reaction whether or not other rows were unreadable
-- so reporting only the first would undercount the second wherever these are tallied.
The record is frozen and the nested selection is deep-copied. Freezing alone would not be
enough: a selection is mutable, so a caller still holding the original could flip
`qualified` afterwards and leave a validated record quietly self-contradictory.
Verified by mutation, each failing at least one test: dropping the status/reason cross-check,
the selection-presence check, the selection type check, the deep-copy snapshot, the frozen
decorator, the bare-string guard (tuple('MSC') is ('M','S','C')), the bool guards on
iteration and schema_version, the first-cause suppression, moving sa_output_malformed
between categories, collapsing internal_error into not_evaluated, and swapping or inverting
reason codes at four selector sites.
Fast suite: 1906 passed. The 4 failures in tests/test_main.py and tests/test_functional.py
are pre-existing at d251414 -- confirmed by running them in a detached worktree at that
commit -- and are stale set_paths expectations from the 'PDep QM budget' key plus a cantera
environment issue, not regressions from this change.
Freezing the record turned out to freeze the reference and not the object behind it, so
every invariant above could be undone the instant construction returned -- and it was the
undone state that got serialized. The deep copy shut the caller's alias and was mistakenly
believed to shut the record's own copy too, which it hands out freely through
record.selection. The rendered snapshot taken at construction is what makes the claim true:
as_dict() serializes it, so no later mutation reaches the file, and it refuses outright if
the live selection has drifted rather than quietly persisting one story while the record in
memory tells another.
The nested selection must also now be about the SAME network as the record. Six fields are
carried by both, and nothing checked any of them, so a record could nest another network's
evidence -- exactly the misattribution t3/pdep/reason_codes.py's own docstring says the
category rules exist to prevent. network_id must match exactly, since that is the identity
claim itself; the other five need only not conflict, because absence is not disagreement and
the funnel legitimately knows things the selector never recorded. That distinction is not
cosmetic: requiring both halves to be populated made 26 tests fail, since it refuses
ordinary records.
Adds ASSESSMENT_ENVELOPE_SCHEMA_VERSION, used by the persistence commit that follows.
…use to read one that lies The record type from the previous commit had nowhere to live. This adds the two functions that put it on disk and take it back off, so the next commit's funnel has somewhere to write the fate of every network it looks at. The write is atomic and durable -- staged in a private directory beside the destination, flushed, renamed, and the destination directory flushed in turn -- which save_pdep_network_selections is not. The difference is not stylistic. The funnel rewrites this file once per network as an iteration progresses, so a crash or a full disk part-way through a write is an ordinary event rather than a theoretical one, and this is precisely the file whose job is to survive the failure that interrupted it. A truncated but still parseable record would be read back as authoritative and would under-report exactly the networks whose silent absence this increment exists to fix. The rename also closes the symlink write-through vector, as the budget record's write already did. Atomic is not the same as durable: os.replace orders the rename but says nothing about whether the bytes behind it ever reached the disk, so without the two flushes a power loss can leave the rename applied and the contents gone -- the file present, and empty or torn. Staging inside a fresh 0700 directory rather than beside the target (as the budget writer does) also closes the window between creating the staged file and reopening it by path to write, during which anything else with access to that directory could substitute a file at the staged path. The list is wrapped in a versioned envelope for the reason the other list-savers give: an iteration in which T3 finds no P-dep networks at all is an ordinary outcome, and an empty list has no record of its own to carry a version. The envelope's version is its OWN constant under its own key rather than a second copy of the per-record one. save_pdep_network_selections does reuse a single number for both roles, but that makes each version a hostage of the other: adding a field to a record would force the envelope to claim a change it never underwent, and renaming the list key would force every record ever written to be re-stamped. A version number that has to lie is worse than none. The loader checks the two separately, so a file cannot claim one shape at the top level and hold another underneath. The loader needs no companion cross-field validator, unlike _selection_from_dict. Every invariant that makes an assessment trustworthy already lives in PDepNetworkAssessment's __post_init__, so constructing one re-checks the whole record: a hand-edited file claiming a selector verdict for a network whose SA never parsed is refused by the same rule that refused the impossible combination at the site that would have written it. Construction is wrapped only so the refusal names the file and the entry -- the type's own message names the network, which suffices where it is written but not where it is read, with a dozen iterations on disk. The per-field helpers already name both and are left unwrapped rather than given a second prefix. An absent 'selection' key is refused rather than read as null. as_dict() always writes the key, rendering it null where the reason code forbids a nested selection, so absent and null are different claims: guessing null would quietly convert a corrupt record into a plausible "never evaluated" one, which is the misreading the file exists to prevent. cache_status is read as a free optional string even though the selection loader enum-restricts its own, because PDepNetworkAssessment accepts any string there. A loader stricter than the constructor that feeds it would make a record T3 legitimately wrote unreadable by T3 -- the one failure a durable record must never have. Strictness not shared with the writer is not safety; it is a bug with a delay on it. Nested selection versions are now bool-guarded like every other version on this branch: True == 1, so `selection_schema_version: true` was being read as correctly versioned. One gap is pinned by test rather than fixed, and deliberately. PDepNetworkSelection is mutable by design -- the selector builds it across some twenty assignments -- so it validates almost nothing, while _selection_from_dict is strict; a selection mutated into e.g. a bare-string `warnings` therefore saves and then cannot be loaded back. It is not reachable from T3's own paths, and validating at construction would not help since the mutation happens afterwards. The real fix is one validation contract shared by writer and loader, which is its own increment. The test exists so the gap stays visible and cannot silently widen. Also drops an unused dataclasses.field import left in assessment.py. 78 new tests. Beyond the round trips, the ones that discriminate rather than merely describe: a save whose underlying write raises leaves the previous record intact rather than truncated, a save onto a symlink replaces the link instead of writing through it, both flushes happen in the right order around the rename, and the staging directory is unreachable by anything else. Verified by mutation, each failing at least one test: dropping either fsync, staging beside the target instead of in a private directory, collapsing the envelope key back onto the record key, dropping either bool guard on the nested selection versions, and reading an absent 'selection' key as null. pdep suite: 1433 passed, up from 1355 at d251414.
Adds the 'PDep network assessments' iteration path key and clears a stale record when the
feature is off, so the funnel that follows has somewhere to write and cannot inherit another
run's answers. No funnel yet -- nothing writes this file at runtime.
It is a sibling of 'PDep QM budget' rather than part of it, because the two answer different
questions over different populations: the budget describes the networks that QUALIFIED --
which were admitted, which refused, and why -- while this describes every network that was
looked at at all. A network that failed before the selector ran never reaches the budget, and
those are precisely the ones that used to vanish without trace.
Stale-clearing mirrors _clear_pdep_budget_record and matters more here. This record's whole
purpose is to be believed about which networks were never evaluated, so a file left over from
an earlier run of the same iteration would not merely be stale, it would answer that question
with another run's networks. Clearing is the only thing standing between "no record" and "a
confident wrong record". It is narrow in the same way: the path is always the single
set_paths-derived key, never constructed or caller-supplied, and removal happens only after
os.path.isfile confirms a regular file.
Fixes the three long-standing failures in tests/test_main.py, which were failing before this
branch and are now green. Two of them turned out NOT to have the cause they had been recorded
as having. Only test_set_paths was stale on the path key ('PDep QM budget', added in
increment 32 and never added to the test's literal). test_as_dict and test_args_and_attributes
were stale on increment 32's SCHEMA additions instead -- pdep_QM_max_transition_states and
pdep_QM_max_networks were missing from the shared expected sensitivity dict. Same increment,
different omission; the diff says so plainly once read rather than assumed.
Fast suite: 1987 passed, 1 failed. The single remaining failure is
tests/test_functional.py::test_computing_thermo, a cantera environment issue unrelated to
this branch, down from 4 before this commit.
…ng one file end the run On a real twelve-network run, seven networks -- the majority -- left no trace of what T3 decided about them, because the only record was appended deep inside the success path. "Assessed and found not worth refining" and "never assessed at all" were the same silence afterwards, and the second is the one an operator has to act on. Four of the paths producing that silence were uncaught exceptions that ended a multi-day RMG+ARC campaign over a single unreadable file. Route every offer through one funnel, `_assess_pdep_network_candidate()`, which has no bare `continue`: every path out of it produces a `PDepNetworkAssessment`, so a network cannot be dropped by adding an exit site. The four crash paths become recorded outcomes -- writing the Arkane input, reading the sensitivity YAML (missing and malformed), and mapping species labels -- and the version arithmetic that used to raise on a stray `network4_backup.py` now ignores anything that is not a versioned network file rather than either dying or discarding the real network beside it. The catches are narrow and the line runs between DATA and CODE, not between convenient and inconvenient. A missing or unreadable network file is a fact about this run's data at either site that reads it, so the input writer's `OSError` is an outcome exactly as the parser's already was; which of the two happens to touch the file first must not decide whether the campaign survives. A `TypeError` is a fact about the code: it is recorded under its own status, never counted among the networks that legitimately could not be evaluated, and re-raised. Recording it is guarded, because this handler runs when something is already wrong enough that the record may be unbuildable, and a complaint about writing the diagnosis down must not replace the diagnosis. Two failures that were per-METHOD were being treated as per-network. The sensitivity artifact is now opened inside the master-equation loop: a method that reports success and leaves nothing readable behind says nothing about whether the next method would, and it no longer spends their turn. Where methods break in different ways those are independent defects in different artifacts, so all of them reach the record rather than whichever ran last. The record is written after every network and marked incomplete until the pass ends. A file holding four of twelve networks is not a smaller truth, it is a different claim, and it is the one corruption that cannot be detected by inspecting the records, since each of them is individually valid. The loader refuses an unfinished file unless asked for one. The pass also stakes its claim on the file before doing any work, so a `complete: true` record from an earlier run of the same iteration cannot survive a crash and answer "which networks were never evaluated?" with another run's networks. Both verdict lists now describe one iteration. Carrying them over was never a decision, and a network re-examined in three iterations was reported three times, against a model that had changed underneath each one.
Comment on lines
+45
to
+76
| from t3.pdep.reason_codes import (INTERNAL_ERROR_REASON_CODES, | ||
| PRE_SELECTOR_REASON_CODES, | ||
| REASON_CODE_STATUS, | ||
| REASON_EVALUATED_NO_UNCERTAIN_TS, | ||
| REASON_INTERNAL_ERROR, | ||
| REASON_NETWORK_DISCOVERY_FAILED, | ||
| REASON_NETWORK_INPUT_WRITE_FAILED, | ||
| REASON_NETWORK_PARSE_FAILED, | ||
| REASON_QUALIFIED_UNCERTAIN_TS, | ||
| REASON_SA_ALL_METHODS_FAILED, | ||
| REASON_SA_OUTPUT_MALFORMED, | ||
| REASON_SA_OUTPUT_MISSING, | ||
| REASON_SA_OUTPUT_UNREADABLE, | ||
| REASON_SA_STRUCTURES_MISSING, | ||
| REASON_SELECTOR_DIRECTION_ENTRY_MALFORMED, | ||
| REASON_SELECTOR_DIRECTION_UNRESOLVED, | ||
| REASON_SELECTOR_MALFORMED_CONDITIONS_NO_TS_ROWS, | ||
| REASON_SELECTOR_NEGATIVE_INCOMPLETE_DATA, | ||
| REASON_SELECTOR_NO_TS_ROWS, | ||
| REASON_SELECTOR_NO_USABLE_TS_ROWS, | ||
| REASON_SELECTOR_SA_PAYLOAD_MALFORMED, | ||
| REASON_SELECTOR_TS_PROVENANCE_UNASSESSED, | ||
| REASON_SELECTOR_TS_RESPONSE_BELOW_FLOOR, | ||
| REASON_SPECIES_LABEL_MAPPING_FAILED, | ||
| SELECTION_BEARING_REASON_CODES, | ||
| STATUS_EVALUATED_NEGATIVE, | ||
| STATUS_INTERNAL_ERROR, | ||
| STATUS_NOT_EVALUATED, | ||
| STATUS_QUALIFIED, | ||
| VALID_ASSESSMENT_REASON_CODES, | ||
| VALID_ASSESSMENT_STATUSES, | ||
| ) |
… back `_read_persisted_yaml_file` already asserted the invariant -- "any file containing [a Python object tag] is not a file T3 wrote" -- but that was an assertion about the writers made on the read side, and nothing enforced it on the write side. `arc.common.save_yaml_file` renders with a full representer, so a single non-plain value in a record (a `pathlib.Path` handed to a `str`-annotated field is the realistic way in) is written out as a `!!python/object/apply:` tag, the write reports success, and every loader here refuses that file from then on. The failure being prevented is TOTAL LOSS, not a bad record: `yaml.safe_load` fails before any per-record check runs, so one bad nested field costs the whole iteration's assessments and reports a YAML tag rather than the field at fault. `_refuse_content_that_would_not_parse_back` renders through `to_yaml` -- the same function the write uses, custom string representer included -- and parses that back. Checking with `yaml.safe_dump` instead would approve bytes other than the ones written, which is the shape of the bug itself. Called by all four writers, before staging in the two that stage, so a refusal leaves no droppings. It does NOT make the loaders accept the file: a `str` where a list belongs is good YAML, refused later per record with a message naming the field. `PDepNetworkSelection` is the only record type written here that does not type-check its own fields in `__post_init__`; closing that is its own work. The `except yaml.YAMLError` catch stays narrow on purpose. A `RecursionError` from a self-referential record is a defect in the code that built it, not a fact about the data, and filing it as a refusal would hide a bug inside a provenance record.
`PDepNetworkSelection` and `SensitiveTransitionState` were the only record types in `t3/pdep` that validated nothing, while `_selection_from_dict` and `_sensitive_transition_state_from_dict` have always been strict. The gap between a permissive constructor and a strict loader is a record that can be built and written but never read back -- and a selection is nested inside both an exploration result and a network assessment, so one bad field cost whichever file carried it. Both now hold the contract their loader enforces. The selection's lives in `validate()`, called from `__post_init__` AND from `as_dict()`, because a constructor check alone would be a guarantee in name only: the record is mutable and `select_from_sa_dict` builds it blank and fills it in across ~20 assignments, so construction sees an empty record. `as_dict()` is the one funnel every persisted copy passes through. Two things the suite proved wrong on the way: `network_id` is NOT required. `rank_pdep_networks` records a decision for an entry too malformed to name a network, and `t3.pdep.budget` counts two such records as two distinct networks rather than collapsing them. The LOADER was relaxed to match -- so a file written by `rank_pdep_networks` + `save_pdep_network_selections`, two public API functions that pair, is no longer refused outright by `load_pdep_network_selections`. The assessment record's `network_id` stays required: it is a statement about a named network. `combine()` then raised `TypeError` instead of its intended `ValueError` on a set mixing `None` with a label, because `sorted` cannot order them. Sorted by `repr` now -- the mixed case is exactly what that refusal is for. Also: `VALID_CACHE_STATUSES` is now shared with the loader rather than respelled there; the `network_id` annotation says `str | None` and its docstring records that distinctness of unnamed records is positional, not identity-based, so a consumer joining on it collapses them all.
…m a file `PDepNetworkSelection.t_grid_clamp` is documented as a `TGridClampRecord.as_dict()` rendering, and the only check anywhere was "a dict or None". It is also the one field whose value is read off disk -- `t3.pdep.api` calls `read_t_grid_clamp_record(sa_path)` unconditionally and copies whatever it returns into up to four live selections, which then persist it as their own provenance. Nothing in t3/ or tests/ reads a key back out of that dict. That makes a malformed value worse rather than more tolerable: there is no failing consumer to reveal it, so the only symptom is someone opening a saved decision much later and believing what it says about the solve behind it. `TGridClampRecord` now type-checks its own fields, and `t_grid_clamp_shape_error` asks the same question of a plain dict, returning a reason rather than raising because its callers need opposite outcomes: `read_t_grid_clamp_record` collapses a malformed sidecar dict to None (unknown provenance -- its documented contract is to disclose, never to gate), while `PDepNetworkSelection.validate()` and the selection loader refuse one. What makes that leniency safe is the other end: `write_sa_cache_metadata` now refuses to record provenance its own reader would discard, so a possibly-foreign file is read leniently and a file T3 writes is written strictly. Only `clamped` is required and unrecognized keys are carried. Sidecars and selection files outlive the version that wrote them in both directions, and refusing either an older writer's missing key or a newer writer's extra one would discard honest provenance -- the loss this record exists to prevent. Tolerating a key means saying nothing about its value, which is what leaves the write-time plain-YAML backstop its remaining live reach. Also from the round-61 review: `nan`/`inf` are refused as temperatures (a `nan` renders, reloads, and then compares unequal to itself, so a record carrying one can never be shown to have round-tripped); a `list` passed for `skipped_species` is normalized to a tuple, since keeping it would let a caller append after construction and render entries the contract never saw; and `combine()` no longer adopts the first component's provenance without comparing, matching what `method` and `cache_status` beside it already do. Cross-field invariants were considered and declined: both temperatures are read from the same parsed line, so the real writers cannot produce an incoherent record, and a semantic rule would fire only on a foreign one -- where, on the sidecar path, it would silently drop provenance. The docstring states that boundary and a test pins it.
…nothing can use `T3Sensitivity.check_me_methods` compared `entry.lower()` against the three method names and then returned the value unchanged, so `ME_methods: ['cse']` in a user's YAML validated. Nothing downstream of it is case-insensitive. `t3/main.py` reads the list straight out of `InputBase(...).model_dump()` and hands each entry to `write_pdep_network_file`, which reaches `METHOD_MAP[method]` in `rewrite_arkane_method_line` -- a bare `KeyError: 'cse'` that the `(OSError, ValueError)` handler around that call does not catch, so a lowercase entry ended the whole run with a traceback naming neither the input file nor the field. The same string is also used verbatim as the per-method output directory name, as the `method` written into and compared against the SA cache sidecar, and as `requested_me_methods` provenance. The validator now returns the canonical `METHOD_MAP` key. Case-insensitive acceptance is kept rather than tightened away: it is deliberate, it matches `global_observables` beside it whose consumers genuinely do read case-insensitively, and no working configuration exists today with lowercase -- it crashed. The repetition check moved after canonicalization, so `['CSE', 'cse']` is still refused as one method spelled two ways. `METHOD_MAP` moves from `t3/utils/writer.py` to `t3/common.py`. Validating input must not drag in the writer's dependency stack (Mako, ARC species perception, the generator, the thermo reader), and `t3/pdep/api.py` imports `T3Sensitivity` only for its defaults, so it would have inherited all of it. `t3.utils.writer` re-exports the constant, leaving every existing `from t3.utils.writer import METHOD_MAP` call site untouched. `write_arkane_network_input_file` now refuses an unknown method before it touches the disk. `rewrite_arkane_method_line` already failed on one, but only after the destination directory had been created and the source copied into it -- leaving a plausible `<network>/<bad-method>/input.py` still carrying the SOURCE file's method, a different solve from the one its own directory name claims. The four other sites that render a method already check up front; this one was the exception.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
⛔ DO NOT MERGE — for @alongd's review
Draft on purpose. This is a large, staged increment and I want your eyes on the design decisions
before anything lands. See Open questions for you at the bottom.
What this adds (I-008 / D-020)
A T3 layer that decides which whole pressure-dependent (PDep) reaction networks deserve
expensive QM (PES exploration + master-equation refinement), and then computes them. The selection
criterion is observable-sensitivity × uncertainty.
The key architectural choice: this reasoning is network-level (ME solver, PES explorer). ARC
stays per-species / per-reaction and never learns that networks exist. Nothing in ARC was
touched;
arc.*is consumed read-only.The end-to-end capability, and the property the whole capture layer exists to provide:
T3.process_arc_run()writes hybrid Arkane P-dep network inputs — QM/RRKM for transition statesthat were successfully computed, RMG/ILT for the rest — from the capture alone, with the ARC
project directory absent entirely.
New module
t3/pdep/:discovery,join,capture,hybrid,selector,parser,energy_settings,me_success,cache,yaml_safe,api, andmesolver/(adapter + Arkaneimplementation + factory).
Constraints held throughout
rmgpyand noarkaneimports in T3.arc.*only;arc.moleculereplacesrmgpy.molecule..pyfiles are parsed withast.parse, neverexec/eval. Parsing an RMGnetwork.pywould otherwise mean executing the RMG/Arkane DSL.Testing
Full suite: 1089 passed, 1 failed (51m37s). The single failure is
tests/test_functional.py::test_computing_thermo(CanteraError) — pre-existing onmain, notfrom this branch.
Roughly 1000 lines of the diff are tests. Beyond assertions, the new guards were mutation-tested:
each guard was individually neutered and a test confirmed to fail, then reverted. Where a mutation
was reported without evidence, it was re-run independently before being believed.
Review-driven hardening
This branch went through 25 adversarial review rounds. That process found — and this PR fixes —
thirteen instances of the same fail-open / vacuous-truth defect, where a guard silently passed
on the case it existed to catch. Representative examples:
verify_capture()never read thestatusfield at all, so a manifest entry claimingstatus: usablewith a null artifact path verified clean, counted zero artifacts, pruned theoutput tree, returned success, and let the finalization marker be written.
verify_capture()and then separately re-read the same manifest, so pathconfinement applied to one parse while the use applied to another.
use_atom_corrections='False'(a non-empty, therefore truthy, string) slipped past a falsy checkwhile the generated file rendered a bare
useAtomCorrections = False.os.replace'd whileinput.pywas a plainopen(...,'w')—the marker was more durable than the output it certifies, so a torn file could be certified
as complete.
Known limitation, recorded rather than hidden
The versioned finalization marker relies on every step of
process_arc_run()being idempotent. TheRMG library append's contribution to that is dedup-by-label: verified to add no duplicates, but
it would silently keep stale values if a source library ever carried the same labels with different
values. No path in this feature produces that, it is pre-existing behavior, and it is documented at
the claim site rather than left implicit.
Open questions for you
state is "no
input.pyplus a finalization marker" — indistinguishable later from not-selected,zero-artifact, manually deleted, or pruned. The reason exists only in logs. Review flagged this
twice. I did not design it unilaterally because it interacts with the prune preflight and I
think the shape should be your call.
t3/utils/libraries.pyhas two pre-existing defects I deliberately did not fix here, sincethey are outside I-008 and deserve their own PR:
description_to_appendis extracted from thedestination's own
longDescand checked against it, so it never consults the source and iseffectively a no-op; and the shared-library lock path is keyed by
library_name, so twoprojects writing the same
shared_library_nameunder different locallibrary_names takedifferent locks — a real concurrency hole in shared-library writes.
explore=Truestill raisesNotImplementedError(t3/pdep/api.py). TheArkaneExplorerAdapteris the remaining unbuilt piece and has no design yet.Related
Depends on RMG-Py PR ReactionMechanismGenerator/RMG-Py#2990 (open, awaiting review) for the
P-dep sensitivity / ILT support this consumes.