Regression - FEATURE - Golden values: committed pins with derived tolerances - #397
Open
d-burg wants to merge 11 commits into
Open
Regression - FEATURE - Golden values: committed pins with derived tolerances#397d-burg wants to merge 11 commits into
d-burg wants to merge 11 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds a “golden values” mechanism to the regression harness, enabling CI gating against committed reference quantities with recorded/derived tolerances and provenance (instead of only differential --refs comparisons).
Changes:
- Introduces golden-file read/write, tolerance policy enforcement, and golden check/update reporting.
- Extends the CLI with
--check,--update-golden, and mandatory--reasonfor golden updates. - Clarifies documentation around SLAYER/tearing transport coefficients (
chi_perp,chi_tor) semantics.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Tearing/Runner/Control.jl | Doc clarification: chi_tor is momentum diffusivity (viscosity), not heat diffusivity. |
| src/InnerLayer/SLAYER/LayerParameters.jl | Doc clarification of P_tor and chi_tor physical meaning. |
| src/InnerLayer/SLAYER/LayerInputs.jl | Doc clarification: chi_tor is χ_φ momentum diffusivity; expanded explanation. |
| regression-harness/src/types.jl | Adds CLI option fields and documents new golden modes. |
| regression-harness/src/golden.jl | New module implementing golden file model, load/save, compare, check report, and update path. |
| regression-harness/regress.jl | Wires golden modes into CLI parsing, execution flow, and exit status logic. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+316
to
+320
| if info !== nothing && !info.success | ||
| println(" RUN FAILED — nothing to compare (this is a crash, not a tolerance failure):") | ||
| println(" $(_short_err(info.error_msg))") | ||
| return (n_pass=0, n_fail=1, n_untracked=0, n_informational=0) | ||
| end |
Comment on lines
+245
to
+246
| within = (x, gold) -> !isfinite(g.atol) || !isfinite(g.rtol) || | ||
| abs(x - gold) <= g.atol + g.rtol * abs(gold) |
Comment on lines
+277
to
+278
| """Worst-element comparison for array quantities, shared by the real and complex encodings.""" | ||
| function _compare_arrays(got, gold, g::GoldenValue, within) |
| a = _json_element_abs(gold[i]) | ||
| d = _json_element_diff(gold[i], got[i]) | ||
| rel = a == 0.0 ? d : d / a | ||
| ok = d <= g.atol + g.rtol * a |
| n_fail += 1 | ||
| continue | ||
| end | ||
| # SQLite NULLs surface as , which the === nothing guards in compare_to_golden |
Comment on lines
+229
to
+242
| # Resolve refs. Golden modes judge the working tree, so they default to it rather than | ||
| # requiring the caller to spell out --refs local. | ||
| resolved_refs = if opts.ref_range !== nothing | ||
| expand_ref_range(opts.ref_range, REPO_ROOT) | ||
| elseif !isempty(opts.refs) | ||
| [resolve_ref(ref, REPO_ROOT) for ref in opts.refs] | ||
| elseif golden_mode | ||
| [resolve_ref(LOCAL_REF, REPO_ROOT)] | ||
| else | ||
| error("Must specify --refs or --ref-range") | ||
| end | ||
| if golden_mode && length(resolved_refs) != 1 | ||
| error("Golden modes compare one ref against the committed values; got $(length(resolved_refs)) refs") | ||
| end |
matt-pharr
self-requested a review
August 15, 2026 21:54
d-burg
marked this pull request as draft
August 15, 2026 22:00
Collaborator
d-burg
force-pushed
the
feature/golden-values
branch
2 times, most recently
from
August 16, 2026 23:01
00417a3 to
3a17004
Compare
d-burg
changed the title
d-burg
force-pushed
the
feature/golden-values
branch
from
August 18, 2026 15:50
80019fb to
cdc3616
Compare
Contributor
|
This pull request is missing an assignee. If you are not ready to name them, mark this pull request as a draft. |
Three docstrings called chi_tor a 'toroidal heat diffusivity'. It is Fitzpatrick's chi_phi, the anomalous perpendicular ion MOMENTUM diffusivity: in the TJ four-field layer model (TJ/Documentation/Layer.tex) it enters the vorticity equation as chi_phi*grad^4(phi + iota_i*N) and the parallel-flow equation as chi_phi*grad^2(V) -- a viscosity in both places -- with tau_phi = r_s^2/chi_phi named the toroidal momentum confinement time and P_phi = tau_R/tau_phi a magnetic Prandtl number. The codebase already knew: the DIII-D example toml comments 'the kinetic file supplies chi_phi(psi)' for this knob, Control.jl maps it to the chi_phi profile dataset, and P_tor/P_perp printed by a run equals chi_tor/chi_perp exactly. Only the fallback-scalar docstrings were wrong. Found while trying to match M3D-C1's viscosity (amu) to SLAYER for the TJ benchmark: the mislabel made SLAYER appear to carry no momentum-viscosity input at all, which cost a day of analysis concluding the wrong thing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… tolerances Differential --refs comparison answers "did this change?" but not "is this right?": nothing is recorded in git and both sides come from the same code. Golden values close that gap — one tracked TOML per case under regression-harness/golden/, holding each quantity's value, the tolerance it must reproduce within (pass iff |x - gold| <= atol + rtol*|gold|), and the evidence behind that tolerance. Five tolerance classes set policy: topological (exact), equilibrium_scalar (tight), physics_converged (measured plateau drift + platform spread), diagnostic and unconverged (reported, never gating — a quantity with no plateau is visibly excluded rather than quietly given a wide bound). Checksums are excluded: a bitwise hash has no notion of close, so it cannot carry a tolerance. Tolerances are derived, never chosen: save_golden refuses an rtol tighter than the recorded platform spread, every entry carries tolerance_basis so a provisional class-default can never be mistaken for a measured bound, and regeneration carries prior evidence forward instead of resetting it. CLI: --check gates the working tree against the committed values (exit 1 on any gating failure); --update-golden regenerates them and requires --reason, because a golden change is a claim about physics a reviewer has to evaluate. Reports print the golden's provenance (commit, environment, reason) next to the current run's. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The check loop iterated only the case's quantities, so a golden entry whose quantity was renamed or dropped from the case TOML was never compared and never reported — the rename silently deleted its gate, with the new name appearing as merely "untracked". Found by adversarial review. Orphans now fail the check regardless of class, with a message pointing at golden regeneration for the reshaped case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…view Fifteen findings from a line-level adversarial review, the load-bearing ones: - A --check that finds no golden file for any requested case now exits 1: a green gate that checked nothing was indistinguishable from a passing one, so a deleted or typo-named golden file silently disabled CI. Mixed coverage warns per case. - --update-golden now prints a REMOVED line for every pin it drops: an h5 rename made extraction return missing, the entry silently vanished from the rewritten file, and the git diff read like intentional cleanup. Deleting a gate is now loud. - Golden provenance records "-dirty" when generated from an uncommitted tree; the commit field is what a reviewer uses to reproduce a disputed number, and it was recording HEAD for numbers HEAD cannot reproduce. - save_golden refuses valueless entries (a NaN scalar became a gating entry with a class and tolerance but no number — failing forever, with regeneration reproducing it identically). SLAYER gamma is legitimately NaN when no root is found, so this path is reachable. - The check path now normalizes SQLite missing before compare_to_golden (the update path already did); a NULL reaching the === nothing guards crashed the report with a non-boolean Missing TypeError instead of failing the quantity. - Tolerance validation runs on every load, not only at write: a hand edit or a merge taking the wrong side could otherwise ship a gating entry with no finite rtol (unsatisfiable) or an rtol below its recorded platform_spread (the exact quiet loosening this mechanism exists to prevent). - A value_type change resets carry-forward: a topological rtol=0 landing on a float gates at bit-exactness forever; a float tolerance landing on a count lets it drift. - A crashed run under --check reports as a crash, not as N tolerance failures. - Non-finite tolerances short-circuit within (Inf + NaN arithmetic made zero-valued diagnostic entries compare false); checksums no longer inflate the untracked count; sing_psi/sing_q reclassified physics_converged (root-found, not pure quadrature); nstep matched exactly instead of by prefix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nner-layer outputs Three distinct Δ' quantities exist and their labels no longer distinguish them poorly: - "Δ′ STRIDE-like BVP (PEST3)" — SingularSurfaces/Delta_prime_matrix from the riccati fundamental-matrix BVP; the tearing-stability Δ'. Pinned at source (riccati case) and now also as ingested by SLAYER, so a change in what SLAYER consumes is caught even when the source case is not run. - "Δ′ Galerkin RDCON (PEST3)" — the resistive-DCON Galerkin outer solution. Both outer solvers are arranged in the PEST3 convention; the labels now say so explicitly. - "PE driven-response Δ′ (SingularCoupling jump)" — the forced ∂b^ψ/∂ψ jump over 2π·χ₁ under applied flux: a driven-response diagnostic, not a tearing-stability Δ', and the case comment now says so. The per-surface ca-based Δ' (compute_delta_prime_from_ca!) is a stub — documented as not physically valid, never written to gpec.h5 — and is deliberately not pinned. The SLAYER case also gains the inner-layer solve's own per-surface outputs: complex layer thickness δ_s, δ_s/d_β, |δ_s| [m], and d_β [m]. The adaptive-scan Δ(Q) sample arrays are deliberately NOT pinned: AMR sample locations move under any refinement change, so they have no stable identity. Validated end-to-end: 21 quantities extract on the DIII-D SLAYER deck. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The AMR scan's Δ(Q) samples in gpec.h5 cannot carry golden values: adaptive sample locations move under any refinement change, so they have no stable identity to pin. This computed case pins the dispersion curve itself instead — Δ(Q) on a fixed 4×4 grid over Re(Q), Im(Q) ∈ [-10, 10], sixteen refinement-stable complex values. The layer parameters are the DIII-D-like SLAYER deck's own 2/1-surface values (extracted from Tearing/PerSurface of a deck run and quoted verbatim), so the case is self-contained and probes the solver alone; the parameter chain that produces those inputs is pinned separately by diiid_slayer_n1. Runs in ~1 s — fast-tier eligible alongside the GGJ references. All sixteen Δ values are finite across the grid, |Δ| spanning 0.21 to 1.52. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…parsing it as a link The clarified chi_tor docstrings wrote "diffusivity [m²/s] (a viscosity ...)", which CommonMark reads as the link [m²/s](a viscosity ...). Documenter then fails the InnerLayer autodocs page with :cross_references, invalid local link/image. Backticking the unit is the convention CLAUDE.md prescribes for exactly this trap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… provenance The Galerkin and STRIDE-like BVP Delta-prime are converging on one canonical SingularSurfaces/ path, so the numbers alone no longer say which formalism produced them: a deck switched from riccati to galerkin would be compared against the other formalism's gold with no signal. The schema forbids echoing control values into output groups, and the subgroup name that used to carry provenance is going away, so the pin reads the deck's own declaration out of the Input/gpec_toml_raw rerun snapshot -- the one group where control values belong. Adds a 'toml_key:<dotted.path>' extraction yielding a 'token' value type, compared by exact equality and classed topological, since no tolerance means anything between "riccati" and "galerkin". A key the deck leaves unset pins as "<unset>", so starting or stopping declaring one is itself a change. Pinned on the four Delta-prime-bearing cases. The two Galerkin cases pin integrator AND gal_flag, because the Galerkin Delta-prime is currently the additive gal_flag path over a riccati sweep rather than a standalone integrator; when the standalone integrator lands and gal_flag retires, these fail loudly, which is the intent -- a deliberate formalism change must regenerate the gold. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…retired The Galerkin cases pinned both integrator and gal_flag because the Galerkin Delta-prime used to be the additive path -- gal_flag layered onto a riccati sweep -- so the pair was what named the formalism. The standalone galerkin integrator has since landed and gal_flag is gone from the control struct, so the decks now declare integrator = "galerkin" and that pin alone identifies the formalism. The gal_flag pin would only ever read "<unset>". Also reattaches two section comments that the original insertion orphaned onto the provenance block. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d-burg
force-pushed
the
feature/golden-values
branch
from
August 18, 2026 18:00
cdc3616 to
e678651
Compare
…figs 6.2/6.3 Every other case in this suite pins GPEC against its own previous output, which answers "did this change?" but never "is this right?". This one pins it against published values: riccati_del_s integrates Fitzpatrick (IOP 2023) equations (6.29)/(6.30)/(6.38) directly and returns the plotted delta_s/d_beta, so prescribing the normalized parameters reproduces the figures' own axes. Verified at eight points inside the published contour bands, and the magnitudes match the book's table 6.2 reactor estimates at DIII-D-like parameters. Pure ODE -- no equilibrium, no geqdsk, no kinetic file -- so it is sub-second, fast-tier eligible, and carries no equilibrium dependence, meaning its golden values survive equilibrium-side re-baselining. Complements slayer_delta_probe, which pins the dispersion solve rather than the width solve. tau is not stated in the book's figure captions, so the reproduction's assumption (tau = 1, table 5.1) is pinned as a tracked quantity rather than left implicit. P_hat = 0 is excluded: it is a singular edge of the model, where alpha vanishes and F -> -i Q_hat at the origin, so pinning it would record a modelling boundary rather than a result. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tion Generated at af3e449 on a clean tree. Tolerances are the provisional class defaults: this is a deterministic ODE with no equilibrium input, so the measured platform spread should replace them before it is relied on as a gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Release note
Adds golden-value regression testing: committed reference numbers with derived tolerances and
tolerance classes, so the harness can answer "is this right?" and not only "did this change?".
The only
src/changes are docstring corrections (chi_toris a momentum diffusivity, not a heatdiffusivity); no computation is touched.
Regression report
regress --cases diiid_n1 --refs origin/develop,local, baseline develop @ 14a3c9d, manifest pinned.47 quantities, all unchanged — as expected, since the
src/diff is docstrings only.Notes for reviewers
Implements the golden-value mechanism for the regression harness: committed, reviewable reference numbers with derived tolerances, closing the gap the differential
--refsmode cannot — it answers "did this change?" but never "is this right?", because nothing is recorded in git.Mechanism
One tracked TOML per case under
regression-harness/golden/, holding each quantity's value, the tolerance it must reproduce within (|x − gold| ≤ atol + rtol·|gold|), the evidence behind that tolerance (plateau_drift,platform_spread,converged_at,tolerance_basis), and full provenance (commit, Julia, platform, Manifest hash, thread counts, and a mandatory human reason).Five tolerance classes set policy:
topological(exact),equilibrium_scalar(tight),physics_converged(measured),diagnosticandunconverged(reported, never gating — a quantity with no plateau is visibly excluded rather than quietly given a wide bound). Checksums are excluded: a bitwise hash has no notion of "close".Tolerances are derived, never chosen.
save_goldenrefuses an rtol tighter than the recorded platform spread; validation re-runs on every load so a hand edit or a bad merge cannot ship a quieter gate than the writer refused to write;tolerance_basisdistinguishes measured bounds from provisional class defaults, and the tool says so loudly when writing provisional ones.CLI:
regress --checkgates the working tree against the committed values (exit 1 on any gating failure, and exit 1 when no requested case had a golden file at all — a gate that checked nothing must not read as green).regress --update-golden --reason "..."regenerates, printing an old→new delta for every changed value and a REMOVED line for every dropped pin.Adversarial review, applied
The mechanism was reviewed adversarially before this PR opened (line-level agent pass; 17 findings). All confirmed defects are fixed in the last two commits, including: orphaned golden entries silently losing their gate on a case-TOML rename (now FAIL), silent pin deletion in the update path (now loud), dishonest provenance from dirty trees (now
-dirtysuffixed), valueless entries from NaN extractions failing forever (now refused at write — SLAYER γ is legitimately NaN when no root exists, so this path is reachable), aMissing-vs-nothingcrash in the check path, unsatisfiable NaN-rtol entries passing load validation, class carry-forward across avalue_typechange mis-gating floats at bit-exactness, and crashed runs reported as tolerance failures.Two review findings deliberately deferred, recorded in the plan document (WS10): the
atol = 0policy for structurally-zero array elements (needs measured atol values, which arrive with the convergence work), and the duplicate tolerance systems (casenoise_thresholdvs golden rtol — a consolidation for the docs/consolidation workstream).Deliberately not included
No golden files ship in this PR. A committed golden implies measured evidence; every tolerance the generator can write today is a provisional class default, and gating on unmeasured pins would be the exact failure mode this mechanism exists to prevent. Real golden files land with the convergence-scan work (converged values, measured plateau drift, feynman-referenced platform spread), per the overhaul plan.
Unit tests for the comparison logic are a follow-up commit on this branch before merge — synthetic-data tests for the array worst-element path, type-change handling, the load-validation refusals, and the zero-coverage exit. Flagging now so review can start on the mechanism.
What gets pinned (when golden files land)
The golden files themselves land with the convergence work; this is the inventory the mechanism will gate, by code section, aggregated across the 13 cases (~160 gating entries total). Classes as auto-inferred by
infer_class, overridable per entry at generation.Equilibrium (
equilibrium_scalar, rtol ~1e-9): q0, q95, βt, βn, βp1–3, βj, li1–3, volume, Ip, bt0, bwall, aspect ratio, κ, ψ₀ (separatrix case).ForceFreeStates / ideal stability (
physics_converged): δW eigenvalues — Re/Im et[1], Re ep[1], Re ev[1], and the full ep/ev/et spectra — plus the vacuum-matrix minimum eigenvalue. Pinned per case on Solovev n=1, Solovev multi-n (n=1..3), both DIII-D decks, and the two kinetic Solovev variants.Singular surfaces / tearing (
physics_converged+topological): msing (exact), singular ψ locations and q values (root-found, so measured-tolerance physics class, not the 1e-9 scalar class).Three distinct Δ′ quantities are pinned individually, and the labels now say which is which (both outer solvers are arranged in the PEST3 convention):
SingularSurfaces/Delta_prime_matrixfrom the riccati fundamental-matrix BVP; the tearing-stability Δ′. Pinned at source (riccati case: diagonal + raw side-major + edge coil response δ_coil) and as ingested by SLAYER, so a change in what SLAYER consumes is caught even when the source case isn't run.Deliberately not pinned: the per-surface ca-based Δ′ (
compute_delta_prime_from_ca!) — a documented stub, not physically valid, never written to gpec.h5.PerturbedEquilibrium (⚠️ PE toroidal torque enters as
physics_converged): plasma/vacuum/surface energies, island half-widths, Chirikov parameter, resonant area-weighted field b^r (norm and per-surface complex), penetrated field, per-surface PE Δ′ on the driven-resistive case.unconverged(non-gating) until #370 resolves — its sign flips under grid refinement, so the auto-inferred class will be overridden at generation.KineticForces / NTV (
physics_converged): FGAR total torque [N·m] and kinetic energy δW_k [J] (DIII-D case), complex NTV torque and root-area-weighted et[1] (Solovev NTV case).Tearing / SLAYER (
physics_converged): per-surface Q_root, γ [Hz], ω [Hz] for 2/1, 3/1, 4/1, plus the layer inputs they derive from (rs, shear, Lundquist S, D_norm, P_perp, τ_k, ι_e) and the no-root flags. Now also the inner-layer solve's own outputs: complex layer thickness δ_s, δ_s/d_β, |δ_s| [m], d_β [m], and the ingested Δ′ BVP diagonal. The adaptive-scan Δ(Q) sample arrays are deliberately not pinned — AMR sample locations move under any refinement change, so they have no stable identity.InnerLayer / GGJ (
physics_converged): Δ_odd and Δ_even (Re/Im) on the Glasser–Wang Eq. 55 galerkin reference and the q=4 rotated-ray case at Q=500i.Galerkin / resistive DCON (
physics_converged): PEST3 Δ diagonal, ‖Δ′ matrix‖, ‖Δ_coil block‖, per-surface D_I and α, inner-layer match residuals.Excluded by construction: runtimes and ODE step counts (
diagnostic, reported never gating) and profile checksums (no tolerance semantics; they remain differential-mode tools).Validation
ggj_reference:--update-goldenwrites 5 quantities (with the provisional-tolerance warning),--checkthen reports 4 pass / 1 informational (runtime moved 6.8e-3 and correctly did not gate), exit 0.--update-goldenwithout--reasonrefuses;--check --update-goldentogether refuses;--checkagainst no golden file exits 1 with "nothing was actually gated"; mixed coverage warns per case.Δ′ provenance: the declared integrator is pinned
The Galerkin and STRIDE-like BVP Δ′ are converging onto one canonical
SingularSurfaces/path (#393 moves the Galerkin result offGalerkinDeltaPrime/ontoDelta_prime_matrix). Both are in the PEST-3 convention, so once they share a path the numbers alone no longer say which formalism produced them — a deck switched fromriccatitogalerkinwould be silently compared against the other formalism's gold.The schema forbids echoing control values into output groups, and the subgroup name that used to carry provenance is what's going away, so the pin reads the deck's own declaration out of the
Input/gpec_toml_rawrerun snapshot — the one group where control values legitimately live.toml_key:<dotted.path>extraction →tokenvalue type, compared by exact equality, classedtopological. No tolerance means anything between"riccati"and"galerkin"."<unset>", so a deck starting or stopping declaring one is itself a change.integratorandgal_flag, because the Galerkin Δ′ is today the additivegal_flagpath over a riccati sweep rather than a standalone integrator. When FFS - REFACTOR - Publish solves as ForceFreeStatesResult; staged main; solve API (refactor plan interface PR) #393's standaloneintegrator = "galerkin"lands andgal_flagretires, these pins fail loudly — which is the intent: a deliberate formalism change must regenerate the gold, not silently re-point it at different physics.Closes #228 (
--update-goldenis the selective update script that issue asks for: it takes a--reason, records the commit, and refuses to run against a dirty tree).Two in-flight physics corrections re-baseline most of what this PR pins, so the mechanism is what is up for review, not the numbers:
b0exp→ physical B_T) movesdiiid_slayer_n1by up to +94.5% (Lundquist S). It also invalidates the provenance ofslayer_delta_probe, whose layer parameters are quoted verbatim from a pre-fix run — the Δ(Q) physics it pins stays valid, but the parameters must be re-quoted.et[1]0.10% ondiiid_n1, 98% onsolovev_n1, and one BVP Δ′ diagonal element 16.73%.Regenerating once, after both land, rather than three times.
🤖 Generated with Claude Code