ALL - IMPROVEMENT - Self-describing HDF5 metadata (issue #226, PR B)

[logan-nc]

Summary

PR B of the two stacked PRs closing #226 (stacked on #363 — review/merge that first; this diff is additive on top of it). Every gpec.h5 dataset now answers "what is this, in what units, plotted against what" without opening the source — netCDF-grade, natively readable by h5py/xarray/HDFView.

What every dataset gets

• long_name — plain-text physics description.
• units — SI strings; "1" for dimensionless (CF convention); normalized quantities state the normalization in the long_name.
• dims (rank ≥ 2) — greppable axis-name string in Julia (column-major) order, e.g. "(psi, mode)".
• HDF5 Dimension Scales — coordinate datasets (ψ_N grids, rational-surface ψ, geometry xs/ys, KF quadrature ψ) marked via h5ds_set_scale and attached per axis with labels: h5py .dims, xarray, and HDFView resolve axes natively (verified with h5py: DIMENSION_LIST + labels present).
• Root attributes — schema_version="2.0" (readers get a dispatch point for future breaking changes), Conventions="GPEC-HDF5-2.0", references, title, date_created (ISO 8601 UTC).

Exempt (whitelisted in the enforcement test): Input/** (raw rerun snapshot) and GalerkinIntegration/Match/** (debug-only, drop pending Galerkin-owner decision — no point hand-curating ~15 entries for it).

Mechanism — one deviation from the approved plan wording

The plan said "route all writers through a write_annotated! helper". Implemented instead as table-driven post-hoc annotation: each writer keeps a path => (; long_name, units, dims) table next to it and applies it with one annotate! call after its datasets are written (Utilities.HDF5Annotations + src/HDF5Schema.jl for the main writer). Same contract, enforced identically by the schema test — but the writer bodies are untouched, keeping this PR genuinely low-conflict with #354/#345/#339/#318 and handling dynamic writes (per-field equilibrium loop, per-method KF groups) naturally. ~340 annotation entries total.

Physics review

The tables were audited by the fortran-physics-reviewer agent against the layer/field-reconstruction sources and the Fortran gpout.f attribute crib; 27 corrections applied, including: J-weighted contravariant fields are T*m^2 (χ₁-weighted), not T; Tearing/PerSurface/tauk is the Q-normalization time S^⅓·τ_H, not a resistive-kink time; island_width_sq is the squared half-width; Roots/omega_Hz is actually an angular frequency (rad/s) — the dataset name is a misnomer, flagged here rather than renamed (renaming is a breaking change; candidate for a future schema bump); delta_n is the Δ′-normalization S^⅓/r_s in 1/m. Reviewers: the covariant-component units (T*m, m^2) were settled by dimensional analysis — worth a second look.

Enforcement

test/runtests_h5_schema.jl now asserts on a full-run output: long_name+units on every non-exempt dataset, dims on every rank ≥ 2 array, root attributes present, and the ψ_N scale attached (28/28 passing). Additional metadata walks were run on the DIIID SLAYER, Solovev kinetic-NTV, and DIIID gal-resistive decks — zero violations. When adding a dataset, add its table entry in the same commit or the test fails — this is the anti-drift teeth #226 asked for.

Notes

• One HDF5.jl wart worked around: the h5ds_set_label wrapper mis-types the C const char* as Ref{UInt8}; we pass a NUL-terminated byte buffer.
• Dates (stdlib) added to Project.toml for date_created.
• Known gap (pre-existing): LocalStability/di/dr have no stored coordinate grid to attach (their ψ grid isn't written); their dims attribute documents the axis name only.

Regression harness (mandatory gate) — zero movement

regress --cases <all 12> --refs develop,local: identical table to PR A — every surviving quantity OK at 0.0e+00 (48/21/15/16/10/4+4 pre-existing N/A/14/14/6/5/4/4). Attributes are invisible to the extractor and the rerun leaf-walk, as designed; the bit-for-bit rerun round-trip test passes with annotations present.

Other verification

Full schema test 28/28; SLAYER runner, coils, fullruns, rerun-from-h5 all pass; docs build clean (new HDF5Annotations autodocs covered); h5py external check confirms scales/attrs.

⚠️⚠️ NO MERGE WITHOUT THIRD-PARTY HUMAN REVIEW ⚠️⚠️

This PR must not be merged until a third-party human reviewer has approved it — non-negotiable project policy. Merge order: #363 first, then this.

🤖 Generated with Claude Code

https://claude.ai/code/session_0129rSTCmYJDBbcH9khHqYnz

[logan-nc]

Full test/runtests.jl suite (all 57 testsets) run on this branch @ 0ece9c4: all pass, zero failures/errors. Together with the harness table in the description (zero movement, identical to #363's), the approved plan's verification checklist is complete.

[logan-nc]

@d-burg please review your outputs carefully here. In particular, the PR overview points out:

Roots/omega_Hz is actually an angular frequency (rad/s) — the dataset name is a misnomer, flagged here rather than renamed (renaming is a breaking change; candidate for a future schema bump);

It looks like you also have a delta_n for the Δ′-normalization S^⅓/r_s in 1/m? This seems dangerously close to delta_mlow, delta_mhigh which means likely confusion when @jhalpern30 extends the code to full 3D (thus perhaps getting delta_nlow type settings? Maybe not exactly that, but still...). How about changing your output to something more unique like delta_prime_norm? In general, please be explicit about prime things - there are far too many deltas in GPEC to be loosey goosey with primes! Apply the fixes on this branch rather than elsewhere to minimize merge conflicts, since the h5 changes have a stack of PRs going.

[logan-nc]

@d-burg is voluntold to do the full diff review on this one since he has some specific things called out.
@matt-pharr is in charge of checking units and long names (feel free to do more if interested).
@jhalpern30 please concentrate on the general h5 structuring logic (feel free to do more if interested).

[d-burg]

Reviewed with a focus on the Tearing/ metadata table, since this PR's annotations become the machine-readable statement of record for the tearing outputs. Scope: I audited the Tearing/ table exhaustively against the writer bodies, the Fortran GPEC slayer/ sources, and Park 2022; I spot-checked the PerturbedEquilibrium, KineticForces and Galerkin tables; I did not audit the main HDF5Schema.jl table. A green review here should not be read as coverage of that.

The mechanism is sound — table-driven post-hoc annotation is the right call for low-conflict stacking, and the table is genuinely complete: I diffed it field-by-field against both _write_per_surface! methods, _write_roots!, _write_layer_widths!, the ragged Diagnostics encoding and both _write_single_scan! branches, and nothing is missing. Requesting changes on one physics error and one enforcement gap.

Blocking

1. PerSurface/tauk states the wrong sign convention (inline suggestion below)

The (Q = −τ_k·ω) parenthetical generalizes a diamagnetic-only minus sign onto the mode frequency. Fortran GPEC slayer/params.f:47-52 is unambiguous:

Qconv=lu**(1.0/3.0)*tau_h ! conversion to Qs based on Cole
Q=Qconv*omega
Q_e=-Qconv*omega_e
Q_i=-Qconv*omega_i

Q = +Qconv·ω for the mode; the minus on Q_e/Q_i exists only to turn the negative-by-convention omega_e (params.f:42) into Q_e > 0, exactly as LayerParameters.jl:228-241 already documents and scopes correctly. Park 2022 agrees independently: Q = S^(1/3)·ω_E·τ_H, positive, with the perturbation taken as ψ ∝ e^(iky − iωt).

As written, this annotation tells every downstream reader that Roots/omega_Hz carries the opposite sign from what GrowthRateExtraction.jl:749 actually produces. I traced the full extraction path — _run_analysis, the AMR contour tracer, the contour intersection code — looking for a compensating flip, and there is none; Im(Q) > 0 = unstable falls out of Q = +τ_k·ω directly. The code is right and the annotation is wrong.

2. The metadata contract never walks Tearing/

runtests_h5_schema.jl runs test_data/regression_solovev_ideal_example, which is ideal-only — and the Solovev equilibrium cannot host tearing at all (examples/Solovev_ideal_example/gpec.toml:47-50: "SLAYER tearing analysis is not run on the Solovev analytic equilibrium"). No committed test deck has a [tearing] section. So _collect_metadata_violations never sees a Tearing/ group, and the PR's "add the table entry in the same commit or the test fails" guarantee does not hold for this module. The DIIID-SLAYER walk cited in the description was a one-time manual run and won't catch drift.

No new fixture is needed. The existing write_slayer_hdf5!: round-trip structure testset (test/runtests_slayer_runner.jl:149) already builds a synthetic two-surface SLAYERResult with store_scan=true and writes it to a temp file — covering PerSurface/, Roots/, LayerWidths/, Diagnostics/ and Scan/ with no equilibrium solve. Three lines close the gap at ~zero runtime cost:

         mktemp() do path, io
             close(io)
             h5open(path, "w") do f
                 write_slayer_hdf5!(f, r)
             end
+
+            # Metadata contract must hold for the Tearing group too — the full-run
+            # schema test only exercises an ideal deck, which writes no Tearing/.
+            h5open(path, "r") do f
+                viol = _collect_metadata_violations(f)
+                isempty(viol) || @error "metadata contract violations in Tearing/" viol
+                @test isempty(viol)
+            end

with include("h5_metadata_check.jl") alongside the file's other includes (see the inline suggestion on runtests_h5_schema.jl for that shared helper). Note this covers the SLAYER branch only — PerSurface/taua/taur/v1 are reachable only through the GGJ writer, so if there's no separate GGJ writer testset those three stay unenforced.

The same pattern generalizes to write_galerkin! and KineticForces.write_to_hdf5!, which are pure writers too, but that's beyond what this PR needs to be safe.

Non-blocking, outside this diff

SLAYERParameters' docstring has two rows that this PR (correctly) contradicts. Worth fixing here so the file and the docstring don't ship disagreeing:

--- a/src/InnerLayer/SLAYER/LayerParameters.jl
+++ b/src/InnerLayer/SLAYER/LayerParameters.jl
@@ -33,7 +33,7 @@
-| `tauk`     | Q-conversion factor S^(1/3) · τ_H  [s] — multiplies ω to get Q    |
+| `tauk`     | Q-conversion factor S^(1/3) · τ_H  [s]: Q = tauk · ω (ω in rad/s) |
@@ -40,8 +40,8 @@
-| `dr_val`   | Radial width parameter at surface (input to dc_tmp)               |
-| `dgeo_val` | Geometric Δ (Shafranov shift factor)                              |
+| `dr_val`   | Resistive interchange index D_R = E + F + H² (input to dc_tmp)    |
+| `dgeo_val` | Connor-Hastie-Helander 2015 Eq. 59 geometric factor               |

dr_val/dgeo_val are settled by LayerInputs.jl:95-99, Control.jl:39-40 and GGJParameters.jl:56 — the PR's wording is the correct one, the docstring is what's stale.

Confirming the calls that are right

Several judgement calls in this PR are correct and I'd rather they not get relitigated alongside the one that isn't:

• Roots/omega_Hz as rad/s is right. Park's ω enters as e^(−iωt), so it is angular by construction, and Qconv is in seconds. The _Hz in the dataset name is the misnomer, not the units attribute.
• The rad/s vs 1/s asymmetry between ω and γ is right and principled — the cycles-vs-radians distinction only bites for oscillation; an e-folding rate is the same number either way.
• Not renaming omega_Hz/gamma_Hz here is right — that's a breaking output-schema change and belongs behind a schema_version bump.
• tauk = S^(1/3)·τ_H, delta_n = S^(1/3)/r_s in 1/m, tau_r = μ₀r_s²/η, c_beta = √(β/(1+β)), D_norm, sval_r, d_beta = c_β·d_i — all verified against params.f and LayerParameters.jl. Correct.
• The brute-force scan dims="(re_axis, im_axis)" matches ScanResult.Q::Matrix being nre × nim in Julia order. Correct.

Filed separately

The units audit surfaced a pre-existing 2π bug on develop: GrowthRateExtraction.jl:441 converts a rad/s quantity to "kHz" with only a 1e-3 factor and compares it against gap_kHz_threshold. Filed as #372 rather than loaded onto this PR — it only gates the :gap warning flag, so no value and no regression-harness quantity moves.

[d-burg]

A1 — blocking. See the review summary: Fortran slayer/params.f:50 is Q=Qconv*omega (plus sign); the minus belongs only to Q_e/Q_i on lines 51-52.

Suggested change

	"PerSurface/tauk" => (; long_name="Q-normalization time S^(1/3)·τ_H per surface (Q = −τ_k·ω)", units="s", dims=("surface",)),
	"PerSurface/tauk" => (; long_name="Q-normalization time S^(1/3)·τ_H per surface (Q = τ_k·ω; the diamagnetic inputs Q_e, Q_i carry the opposite sign by convention)", units="s", dims=("surface",)),

[d-burg]

A2. tau_r is written by the SLAYER _write_per_surface! (line 164) and taur by the GGJ one (line 185). They never coexist, but they currently carry byte-identical long_names, so the annotation can't tell a reader which branch produced their file.

Suggested change

	"PerSurface/tau_r" => (; long_name="resistive diffusion time τ_R per surface", units="s", dims=("surface",)),
	"PerSurface/tau_r" => (; long_name="resistive diffusion time τ_R = μ₀r_s²/η per surface (SLAYER layer parameters)", units="s", dims=("surface",)),

[d-burg]

A3. Same disambiguation on the GGJ side.

Suggested change

	"PerSurface/taua" => (; long_name="Alfvén time τ_A per surface", units="s", dims=("surface",)),
	"PerSurface/taur" => (; long_name="resistive diffusion time τ_R per surface", units="s", dims=("surface",)),
	"PerSurface/taua" => (; long_name="Alfvén time τ_A per surface (GGJ layer parameters)", units="s", dims=("surface",)),
	"PerSurface/taur" => (; long_name="resistive diffusion time τ_R per surface (GGJ layer parameters)", units="s", dims=("surface",)),

[d-burg]

A5. Roots/* are the only rank-1 tearing datasets without dims, which breaks the greppability the attribute exists for. (Length-1 in coupled mode, length-msing in uncoupled — surface is right for both.)

Suggested change

	"Roots/Q_root_real" => (; long_name="Re of the dispersion-root normalized frequency Q (NaN = no root)"),
	"Roots/Q_root_imag" => (; long_name="Im of the dispersion-root normalized frequency Q (NaN = no root)"),
	"Roots/Q_root_real" => (; long_name="Re of the dispersion-root normalized frequency Q (NaN = no root)", dims=("surface",)),
	"Roots/Q_root_imag" => (; long_name="Im of the dispersion-root normalized frequency Q (NaN = no root)", dims=("surface",)),

[d-burg]

A4. Both units calls here are correct — this just records why, so nobody "fixes" them later, and adds the one invariant a downstream reader most needs (positive = unstable), which the file currently states nowhere.

Suggested change

	"Roots/omega_Hz" => (; long_name="mode rotation angular frequency of each root", units="rad/s"),
	"Roots/gamma_Hz" => (; long_name="growth rate of each root", units="1/s"),
	"Roots/omega_Hz" => (; long_name="mode rotation angular frequency ω = Re(Q)/τ_k of each root (the dataset name is a misnomer: this is angular, not cycles/s)", units="rad/s", dims=("surface",)),
	"Roots/gamma_Hz" => (; long_name="growth rate γ = Im(Q)/τ_k of each root, positive = unstable (an e-folding rate, so no 2π distinction applies)", units="1/s", dims=("surface",)),

[d-burg]

A5 (cont.) — dims on the remaining Roots/ entry.

Suggested change

	"Roots/no_root" => (; long_name="flag: no usable dispersion root found (Q_root is NaN, ω/γ are placeholders)"),
	"Roots/no_root" => (; long_name="flag: no usable dispersion root found (Q_root is NaN, ω/γ are placeholders)", dims=("surface",)),

[d-burg]

A6 — dims for consistency with the rest of PerSurface/.

Suggested change

	"PerSurface/dc_type" => (; long_name="per-surface D_c prescription label"),
	"PerSurface/dc_type" => (; long_name="per-surface D_c prescription label", dims=("surface",)),

[d-burg]

A7. The SLAYER and GGJ _write_per_surface! methods write disjoint field sets, so today the only way to tell which model produced a file is to notice which PerSurface datasets exist. Inferring the model from the shape of the schema is exactly what self-describing metadata should eliminate — and it makes A2/A3 a convenience rather than the sole signal. A group attribute keeps this out of the dataset walk, so the schema test is unaffected.

Suggested change

	g = create_group(parent, "Tearing")
	g["enabled"] = Int(result.enabled)
	result.enabled || return g # nothing else to write
	if !result.enabled # nothing else to write
	g = create_group(parent, "Tearing")
	g["enabled"] = Int(result.enabled)
	# Which inner-layer model produced PerSurface/: the SLAYER and GGJ branches write
	# disjoint field sets, so readers must not have to infer it from the schema.
	attrs(g)["layer_model"] = result.enabled ? _layer_model_token(eltype(result.params)) : "none"
	if !result.enabled # nothing else to write

with the helper added next to the annotation tables (e.g. just above TEARING_H5_ANNOTATIONS):

# Token recorded in the Tearing group's `layer_model` attribute.
_layer_model_token(::Type{<:SLAYERParameters}) = "slayer"
_layer_model_token(::Type{<:GGJParameters}) = "ggj"
_layer_model_token(::Type) = "unknown"

GGJParameters is already in scope here via the _write_per_surface! dispatch, and empty_slayer_result builds SLAYERParameters[] even when disabled, hence the result.enabled guard.

[d-burg]

B2 — needed for the blocking enforcement fix. Lift this walk verbatim into a shared test/h5_metadata_check.jl so runtests_slayer_runner.jl can call it too (see the review summary). No behaviour change here.

Suggested change

	# Metadata contract (docs/development/hdf5-conventions.md): every dataset carries
	# long_name + units, and rank ≥ 2 datasets carry a dims axis-name attribute. Exempt:
	# the Input/ raw snapshot and the debug-only GalerkinIntegration Match/ group.
	_metadata_exempt(path) = startswith(path, "Input/") || occursin("/Match/", path)
	function _collect_metadata_violations(h5)
	bad = String[]
	function walk(node, prefix)
	for k in keys(node)
	child = node[k]
	full = isempty(prefix) ? k : prefix * "/" * k
	if child isa HDF5.Group
	walk(child, full)
	elseif !_metadata_exempt(full)
	a = attrs(child)
	haskey(a, "long_name") || push!(bad, "$full: missing long_name")
	haskey(a, "units") || push!(bad, "$full: missing units")
	ndims(child) >= 2 && !haskey(a, "dims") && push!(bad, "$full: missing dims")
	end
	end
	end
	walk(h5, "")
	return bad
	end
	include("h5_metadata_check.jl")

New file test/h5_metadata_check.jl:

# Metadata contract (docs/development/hdf5-conventions.md): every dataset carries
# long_name + units, and rank ≥ 2 datasets carry a dims axis-name attribute. Exempt:
# the Input/ raw snapshot and the debug-only GalerkinIntegration Match/ group.
_metadata_exempt(path) = startswith(path, "Input/") || occursin("/Match/", path)

function _collect_metadata_violations(h5)
    bad = String[]
    function walk(node, prefix)
        for k in keys(node)
            child = node[k]
            full = isempty(prefix) ? k : prefix * "/" * k
            if child isa HDF5.Group
                walk(child, full)
            elseif !_metadata_exempt(full)
                a = attrs(child)
                haskey(a, "long_name") || push!(bad, "$full: missing long_name")
                haskey(a, "units") || push!(bad, "$full: missing units")
                ndims(child) >= 2 && !haskey(a, "dims") && push!(bad, "$full: missing dims")
            end
        end
    end
    walk(h5, "")
    return bad
end

[d-burg]

Follow-up on the blocking enforcement gap — it now has a concrete instance rather than a hypothetical one.

#298 (FKR-width, resistive-layer-width sanity checks) adds two fields to LayerWidths:

δ_FKR   = p.rs * p.lu^(-1.0/3.0)
δ_visco = δ_FKR * p.P_perp^(1.0/6.0)

It doesn't touch src/Tearing/Runner/HDF5Output.jl, so nothing collides today — the fields are computed but never written. But the moment they're wired into _write_layer_widths! (which is the obvious next step for a diagnostic like this), they land in Tearing/LayerWidths/ with no entry in TEARING_H5_ANNOTATIONS, and runtests_h5_schema.jl still passes green — because the deck it runs is ideal-only and never produces a Tearing/ group at all.

That's the failure mode the three-line fix in runtests_slayer_runner.jl:149 prevents, and #298 is a live PR that would walk straight into it.

Two clarifications while I'm here, so nothing above reads as a criticism of work that's actually correct:

• The "all 57 testsets pass @ 0ece9c4" result is consistent with this finding, not a rebuttal of it. The suite passes precisely because nothing in it walks Tearing/. The annotations themselves are fine; it's the guard that has the hole.
• I trial-merged the stack against current develop (22 commits ahead). Five conflicts surface — benchmarks/gal_validation/* ×3 (modify/delete: 1d30dfba deleted them on develop), src/ForceFreeStates/EulerLagrange.jl, and src/GeneralizedPerturbedEquilibrium.jl. All five reproduce when merging develop into ALL - REFACTOR - Module-mirroring CamelCase HDF5 schema (issue #226, PR A) #363's base alone, so none of them are this PR's. The "genuinely low-conflict with PE - Removing AI slop from PerturbedEquilibrium module #354/VACUUM - Migrate surface current calculation into the VACUUM module #345/KineticForces - FEATURE! - Multi-main-ion NTV (summed ions + impurity + electrons, both paths) #339/Drift-kinetic island/layer solver #318" claim holds up under test — the rebase burden sits on ALL - REFACTOR - Module-mirroring CamelCase HDF5 schema (issue #226, PR A) #363. Noting it so the eventual rebase doesn't get misattributed here.

Also worth flagging for whoever rebases: #368's fix to the legacy haskey(f["singular"], ...) calls in the gal_validation scans is moot now that develop has deleted those files.

[logan-nc]

Branch updated 0ece9c4f..2bb72589: merged the remediated PR-A base (second develop sync + independent-review fixes + reviewer dispositions — see the #363 thread) and addressed this review point-by-point:

Blocking 1 (tauk sign) — applied your suggested wording verbatim (modulo a 3-word trim for the 180-char margin): Q = τ_k·ω, minus scoped to the diamagnetic inputs only. You were right and the "applied correction" was wrong — thank you for tracing the extraction path. Also fixed the two contradicted SLAYERParameters docstring rows (dr_val, dgeo_val) per your non-blocking diff.

Blocking 2 (Tearing/ never walked) — the walker now lives in shared test/h5_metadata_check.jl, included by both runtests_h5_schema.jl and runtests_slayer_runner.jl. The existing synthetic round-trip testset now walks its temp file (your 3-line suggestion, plus a layer_model assert), and a new synthetic GGJParameters write covers the GGJ-only rows (taua/taur/v1) you flagged as otherwise unenforced. SLAYER runner: 65/65.

A1–A7 — all applied: tau_r/taur disambiguated with branch labels; dims=("surface",) on all Roots/* and dc_type; the rad/s / 1/s rationale + positive = unstable invariant recorded; and the layer_model group attribute ("slayer"/"ggj"/"none") from your A7 suggestion, via _layer_model_token(eltype(result.params)).

Note the stack base has moved (#363 got a second develop sync + the legacy-map drop per review there) — the metadata mechanism here is untouched by that, but the diff base shifted. Full targeted suites green on this tip (schema 14/14 + 6/6, SLAYER 65/65).

Requesting re-review.

🤖 Generated with Claude Code

https://claude.ai/code/session_0129rSTCmYJDBbcH9khHqYnz

[logan-nc]

One more annotation correction pushed (ca870696), caught by @logan-nc: KineticForces/<method>/total_energy stores δW_k with the 2n already divided out (Compute.jl: imag(total_torque) / (2*ctrl.nn)) — the long_name wrongly claimed 2n·δW_k. The ψ-profiles (T_imag, dTdpsi_imag) are raw Im(T) and keep the 2n label; both long_names now state the scalar-vs-profile asymmetry explicitly so neither gets "corrected" to match the other later.

[logan-nc]

Finding lots of human intuition issues with the AI overhaul (as expected)... posting review of SLAYER and KF now so claude can knock 'em out while I review PE. Goal is to merge tonight

[logan-nc]

Is the 2n really here? Something reported as energy should not have the 2n in it. Something reported as Imaginary component of the torque should

[logan-nc]

I believe *_real and *_imag subscripts are not necessary and torque can be stored natively as complex. Is there a reason it is broken up here? This should be standardized throughout all h5 writes... add something to the h5 instruction md?

[logan-nc]

Most other codes report dT/dV the torque density [N/m^2]. Lets make sure we have dVdpsi in the outputs so this is easily available

[logan-nc]

@d-burg should confirm this meaningful change and suggest new names for this an other *_val SLAYER outputs. IMO the _val subscripts add nothing but are taking room. This could be Interchange_D_R or D_R_interchange for example.

[logan-nc]

Parenthetical is overkill - these are still meant to be names, not general annotations

[logan-nc]

could include the actual equation for l_eff here since it is short, which would be consistent with lambda

[logan-nc]

The docs should explain why there are complex torque and energy here but only a complex torque (with energy corresponding to the imaginary part) at the top level of KineticForces outputs.

[logan-nc]

seems confusing for new users? A re-review should think hard about if there is a cleaner way to organize these outputs in the h5

[logan-nc]

Users familiar with the papers will know these by names like A,B, etc. while the index numbers are just an artifact of the code's consolidated storage. Rename more intuitively.

[logan-nc]

Will repeated dims like (mode, mode) break interfaces we claim to be facilitating like xarray? Do we need to name them distinctly? That would introduce it's own possible confusion points. Is there an industry standard for this kind of thing?

[logan-nc]

Why are separate helper functions defined each time? It seems like its just to do manual make_scale calls or looped calls. Couldn't that be handed by the general annotate function is we just added a (optional?) scale entry to the *_H5_ANNOTATIONS tables?

[logan-nc]

Finished human review of Galerkin - plenty of issues for a new AI plan to address.
Now on to PE for real

[logan-nc]

why are we outputting such a fine grid q profile? That seems excessive as we already output the q spline information it comes directly from in the Equilibrium section of the h5

[logan-nc]

Do we need a documented standard for variable name syntax in cases like this where the variables are words? To me, the is_singular seems most natural despite using CamelCase for the section labels... but I guess I could be convinced of IsSingular if that conforms better to standards (i.e. if the CamelCase choice means we should be CamelCase through and through). We should standardize throughout either way

[logan-nc]

We used d*dpsi for derivatives in KineticForces. Fortran used *1 and we might have a few uses of that laying around too. We should again standardize throughout for a consistent h5 experience. I think d*dpsi is the least ambiguous (note simple *_psi is not available since we use that for the psi component of things). Again, standardization means changing everything now and adding a note to the h5 conventions md

[logan-nc]

I don't understand the ; RDCON Galerkin part of this. Remove?

[logan-nc]

This physical quantity is written multiple places. That is ok (it isn't large) but the name should be consistent throughout. Have claude check for other repeated variables while at it and make sure the names are consistent (again, add a note in the conventions so anyone adding to the h5 later is sure to follow suite).

[logan-nc]

another quantity written multiple places. Note that I prefer "rational" to "singular" since we have options to do kinetic and resistive runs where the solutions are not singular on the rationals.

[logan-nc]

Perfect example of mixed use of singular (sing_*) and rational. While I am at it, I will explicitly say the ordering of specifiers should be standardized as well... no good having sing_bloop and blip_sing in the same h5 output file! This goes for all specifiers (no using pest3_this here and that_pest3 somewhere else, for example)

[logan-nc]

lowercase and all one word is inconsistent with use of D_R elsewhere. This was inherited from bad formatting in lazy old fortran. We should modernize and make names intuitively match the standard literature → D_I here. Claude should spin a subagent to look for any similar opportunities to improve and standardize names.

[logan-nc]

Is the capitalization of this delta consistent with the literature?

[logan-nc]

Both review rounds addressed across two commits (ca870696..279e7b57), propagated to #368. Point-by-point:

KF/SLAYER round:

• 2n in total_energy — you were right; fixed in ca870696 (scalar stores δW_k = Im(T)/(2n); the ψ-profiles are raw Im(T) and both long_names now state the asymmetry).
• Native complex storage — done everywhere: KF dTdpsi/T/EnergyIntegrals arrays, Tearing Q_root/dels_db→delta_s_over_d_beta/delta_s/ragged flat/Scan Q,Delta, and the DpMatrix subgroup collapsed to one complex Delta_prime_matrix. Convention codified ("never *_real/*_imag pairs"); sole documented exception is the Input/RawInputs/ForcingTerms amplitude pair (external ingest-format mirror; converting breaks replay of existing snapshots). Harness got a complex-preserving first_3_complex extract for the tracked Q.
• dT/dV — dVdpsi is now written per method at the quadrature points (spline passed from the pipeline), so dTdpsi ./ dVdpsi gives the density directly; also in Equilibrium/Profiles/dVdpsi on the profile grid.
• matrix_1..6 — renamed to the Logan letters A,B,C,D,E,H with identities (A = W_Z†W_Z, …) and the energy normalization stated (verified: energy_factor = tnorm/(2in) in Torque.jl).
• Repeated dims — netCDF/CF permits them but xarray mangles them on load; industry practice is distinct names, so all square matrices now use (mode_row, mode_col)-style axes (documented).
• Scale helpers — folded into the tables: entries now carry optional scale=/attach= fields handled by annotate! itself; the per-writer imperative helpers are gone.
• leff — long_name carries ℓ_eff = ℓ + n·q (circulating) / ℓ (trapped).
• Ragged layout — kept (VLEN types have patchy cross-language support; Tearing/Diagnostics uses the same pattern), and docs/src/kinetic_forces.md now explains the complex-torque convention split and gives worked Julia/h5py offset-indexing examples.
• dr_val/dgeo_val — renamed D_R/D_geo (both are D's in GGJ/CHH usage — checked; @d-burg feel free to counter-propose).

Galerkin round (naming standardization, 279e7b57):

• Fine-grid Solution/q dropped (redundant with Equilibrium/Profiles).
• One rational-surface convention, specifier-first: rational_psi/rational_q/rational_m/rational_n everywhere (SingularSurfaces bare names, GDP sing_*, and PE rational_m_res all converge); msing/kmsing → rational_count; issing → is_rational; Tearing ising → rational_index.
• Literature caps: di/dr/di0 → D_I/D_R; full Delta family — Delta_prime, Delta_prime_matrix, Delta_prime_raw (GDP delta merges into this leaf — it is the same side-major object), Delta_coil, Delta_r, C_Delta_prime, Delta_prime_norm; tau_r/taur collision → tau_R with tau_A/tau_k following; dc_tmp/dc_type → D_c_offset/D_c_type; omega_Hz/gamma_Hz → omega/gamma (the Hz misnomer dies).
• Derivatives are d<x>dpsi: q1→dqdpsi, q1lim→dqdpsi_lim, dxi_psi→dxi_psidpsi, xi_deriv→dxidpsi, clebsch_psi1→dclebsch_psidpsi, p1_local/v1_local→dmu0pdpsi/dVdpsi (+ p_local→mu0p, same-leaf-as-Profiles rule).
• All five principles are now codified in docs/development/hdf5-conventions.md; readers/tests/benchmarks/harness cases updated in lockstep (quantity names preserved).
• "; RDCON Galerkin" — removed.

On superscripts for contravariant components: there is no HDF5/netCDF standard for marking super vs subscripts in names — the _ convention is universal (inherited exactly as you guessed), and CF handles typography via attributes, not names. Our long_names already carry the typeset form ("contravariant radial displacement ξ^ψ"); if you want it machine-readable we could add an optional symbol attribute (e.g. "\xi^\psi") to the metadata contract as a follow-up — cheap in the annotate! mechanism, ~340 entries of content.

Tests green at each step (schema 14/14 + 6/6 incl. metadata walk with the renamed tables, SLAYER 64/64, fullruns 17/17). Ready for your re-review — and per policy, your review is the merge gate tonight.

🤖 Generated with Claude Code

https://claude.ai/code/session_0129rSTCmYJDBbcH9khHqYnz

[logan-nc]

Final human comments done!

[logan-nc]

lets make sure the radial variable of normalized poloidal flux is consistently named throughout. It is psi_n here, but I think it might be just psi in ForceFreeStates → standardize throughout the h5

[logan-nc]

Consider if there is a h5 industry standard for subscripts - the use of _ here and elsewhere for contravariant components is potentially confusing.

[logan-nc]

Variable name reads as though J is a subscript. Other variables I believe put the J in front (like Jbgradpsi). Standardize throughout.

[logan-nc]

Is it confusing to have the J explicitly in the psi component but implicit here and in zeta?

[logan-nc]

If we find a better standard for distinguishing superscripts and subscripts, we can get rid of this clumsy _cova_ notation

[logan-nc]

I thought there was a big push to make all the weights of the fields be Tesla... was it just the psi components then? This deserves a second hard look from a devoted physics agent review.

[logan-nc]

Also spin a devoted agent review of these cova units

[logan-nc]

I believe we are conforming all Δ → Delta and reserving any delta for  δ type variables. This should conform as well.

[logan-nc]

how does "surface" work as a dimension? If it's the rational surface index, this deserves a second look. Any physicist would want things to natively plot vs rational q or rational psi. That being said, perhaps the index is necessary to properly prepare for the future expansion to full 3D with multiple n such that there can be multiple resonances per rational q... if that is the case, then ok. But perhaps add documentation somewhere of how to quickly plot things like the effective resonant field vs q

[logan-nc]

Should be Delta_prime

[logan-nc]

Final round pushed (279e7b57..d668f572). All 31 review comments are now addressed; follow-ups that were deliberately left out are tracked in #388.

PE round (Q1–Q11)

• Coordinate naming — one vocabulary now: the radial abscissa is psi and the poloidal one theta in every group. Response/psi_n, Profiles/xs, Geometry/xs/ys are all gone; documented as a rule.
• Super/subscripts — there is no HDF5 or netCDF standard for them (you guessed right: the flat _ convention is inherited from the Fortran netCDF era and is what every code uses). CF handles typography in attributes, not names. So: bare suffix = contravariant (xi_psi = ξ^ψ), _cov_ = covariant (b_cov_theta = b_θ), leading J = Jacobian-weighted (Jxi_psi, Jb_zeta) — J moved to the front exactly as you noted Jbgradpsi does, and the implicitly-weighted xi_theta/b_zeta/etc. are now explicitly Jxi_theta/Jb_zeta so nothing is implicit any more. The typeset form always appears in long_name. If you later want it machine-readable, an optional symbol attribute would slot into the annotate! mechanism cheaply.
• 1 derivatives — already gone in 279e7b57 (clebsch_psi1 → dclebsch_psidpsi, q1 → dqdpsi, q1lim → dqdpsi_lim, xi_deriv → dxi_psidpsi).
• Δ vs δ — conformed in 279e7b57; Delta_prime at the line you flagged, with the whole Delta family capitalized and δ-quantities (delta_s, d_beta) left lowercase.
• Tesla weights (devoted physics review, as requested) — verdict: current units are correct, no change. The tesla convention covers the normal-field representations (b, b̃, b̄, b_n, resonant/penetrated). Jb_theta/Jb_zeta are flux-like contravariant densities (T*m^2), coordinate-dependent by construction with no scalar area to make them invariant — rendering them "in tesla" would be a false promise. Worth knowing: the ψ component already is one of the tesla three — b_psi_area_weighted = (J·b^ψ)ₘ/A is exactly b̄, since J|∇ψ|·b_n = J·b^ψ pointwise. Its long_name now says so. Tesla-valued tangential quantities already exist as b_R/b_Z/b_phi.
• Covariant units (second devoted review) — confirmed, and now proven rather than dimensionally argued: metric.fs[1..6] are g_ij/J (I verified independently — v[3,3] = 2πR/jac is |e_ζ|/J, not ∇ζ), so the contraction gives ξ_i = ξ·e_i → m², and b_i → T·m. long_names now spell out the ξ·e_ψ form so the units are self-evident.
• surface as a dimension — your instinct was right that a physicist wants to plot vs q. The index stays (multi-n can put several resonances on one q, as you supposed), but rational_q is now a second dimension scale attached to that axis alongside rational_psi, in all three rational-surface groups — so plotting vs either is native in h5py/xarray. docs/src/perturbed_equilibrium.md has a worked snippet.

Independent standardization audit (your closing ask)

Two blocking finds, both real and fixed here:

1. LocalStability/ had a name collision hiding a missing coordinate. D_I, D_R and ballooning_Delta_prime are computed on the locstab grid — which was never written to the file — while LocalStability/psi was actually the unrelated ballooning α-scan grid. Nobody could plot D_I against anything. The profile grid is now written as psi (a scale, attached to all three) and the α scan as ballooning_psi with its own scale.
2. PerSurface/tau_R had two table entries with conflicting long_names after my τ unification. annotate! is last-wins, so every SLAYER run was being labelled "(GGJ layer parameters)". Merged into one neutral entry; the layer_model group attribute already tells you which branch ran.

Also fixed: SingularSurfaces/Kinetic/* had visibly thinner metadata than its sibling (no dims/scale/attach — now wired); resonant_area_weighted_field and dclebsch_psidpsi were each missing an attach their siblings had; dead b_norm and four unreachable Kinetic ELM table entries trimmed; Match/InnerParams taua/taur/v1 → tau_A/tau_R/dVdpsi (the project already had that exact mapping in Tearing); nn → n; mode_index → mode; and the conventions doc itself was violating two of its own rules in prose.

Audit also confirmed clean: units-string formatting is uniform across all five tables, scale names agree across all seven ψ-grid producers, no sing/psi_n/xs/_val/_tmp/*_real survivors, and no writer↔table drift.

Verification

Schema+metadata test 14/14 (+6/6 rule tests), SLAYER runner 64/64, fullruns, coils, rerun-from-h5 all green; package loads; margins/whitespace clean. The schema test earned its keep twice tonight — it caught an interpolated $fwd/dxi_psi writer/table mismatch and then my own _CachedVector write bug in the LocalStability fix.

🤖 Generated with Claude Code

https://claude.ai/code/session_0129rSTCmYJDBbcH9khHqYnz

[logan-nc]

Audit correction (0959bcc9): re-checking my own work against all 31 comments, one was only half-addressed. On the issing comment you asked two things — rename it and "we should standardize throughout either way" with "a documented standard for variable name syntax in cases like this where the variables are words". I did the rename (is_rational) but never wrote the standard down, and the file was in fact inconsistent: is_rational alongside bare enabled, truncated, no_root.

Now documented in the conventions md: multi-word dataset names are snake_case English (CamelCase is groups-only), and a boolean flag takes an is_ prefix only where the bare word would read as a noun or collide with a data family — which is exactly why is_rational needs one (bare rational would clash with the rational_* coordinate family) while enabled/truncated/no_root already read as predicates and stay as they are. That legitimizes the existing names under one stated rule rather than forcing three more renames at merge time.

Also verified in the same pass, for the record: Delta_coil is capitalized (your "is the capitalization of this delta consistent with the literature?"); the only lowercase delta datasets left are delta_s/delta_s_abs/delta_s_over_d_beta, which are genuinely δ_s in Fitzpatrick; and the only trailing-1 names left are betap1/delta1/li1, which are definition indices, not derivatives.

[logan-nc]

Equilibrium/ scalar naming pass (final standardization item)

The last group still carrying Fortran-era contractions was Equilibrium/ itself. Pushed as H5 - IMPROVEMENT - Give Equilibrium scalars literature names, applying the same rule the rest of the file now follows: variable first, qualifier as a trailing subscript, numbered literature definitions keep their number as the last subscript.

Family	Before → after
Axis / wall	ro/zo/b0/bt0/bwall → R_axis/Z_axis/B_axis/B_T_axis/B_T_wall
Geometry	rmean/amean/aratio → R_mean/a_mean/aspect_ratio; delta1/delta2 → delta_upper/delta_lower
Boundary shape	rsep → R_midplane, rext/zext → R_extremum/Z_extremum
Safety factor	q0/qa/q95/qmin/qmax → q_axis/q_edge/q_95/q_min/q_max; mextrema → q_extrema_count
Beta / inductance	betat/betan/betaj/betap1-3 → beta_t/beta_N/beta_j/beta_p_1-3; li1-3 → l_i_1-3
Other	crnt → I_p, bt_sign → B_T_sign, psio → psi_total

Descriptive _axis was chosen over literature _0 so the qualifier slot reads identically across _axis / _edge / _wall / _min / _upper, and so it matches the psi_axis / psi_boundary pair already in the file.

On rsep vs rext: the docstrings ("plasma boundary" / "plasma edge") were not distinguishing them. rsep solves θ + ν(θ) − ν₀ = 0 at ν₀ = 0 and 0.5, i.e. the inboard and outboard midplane crossings; rext/zext are at the Z extrema. zsep is assigned the identical value as zext one line apart, so it is dropped.

Seven datasets leave the file: four exact duplicates (psi0, psi_axis, psi_axis_norm, zsep — the first three are all the same number as psi_total, with psi_axis additionally mislabeled) and three control-flag echoes (verbose, diagnose_src, diagnose_maxima) that violate the "inputs live only under Input/" rule this PR codified.

EquilibriumParameters is untouched. A EQUIL_H5_NAMES map plus an EQUIL_H5_SKIP set in src/HDF5Schema.jl sit between the struct fields and the dataset names — the same indirection Tearing/Runner/HDF5Output.jl already uses. Readers (Analysis/{Equilibrium,ForceFreeStates,PerturbedEquilibrium,PerturbedEquilibriumModes}.jl), the Fortran-comparison benchmark, both LAR scan scripts, regression-harness/src/runner.jl, and 13 harness case h5paths follow. Harness quantity names are unchanged, so cached history still matches.

Follow-up filed as #389: while auditing the assignment sites I found that most of the flux bookkeeping (psi_boundary, the offsets, the signs, psi_boundary_zero) is hardcoded literals rather than computed from the ingest, and that ro/zo/psio/b_norm are never assigned on the struct at all. Per review direction those stay in the output for now and are handled there, not here.

Docs updated (docs/development/hdf5-conventions.md naming rules + schema table, docs/src/workflow.md). Schema, rerun-from-h5, and fullruns tests running now; I will report results.

Important

This PR still requires a third-party human review approval at its current head before merge. The branch has moved several commits past the earlier approval, and with dismiss_stale_reviews: false / require_last_push_approval: false on this repo an auto-merge label would merge on that stale approval. Not applying it.

[logan-nc]

Correction to my previous comment: the separatrix bracket solves θ + η(θ) − η₀ = 0 (the poloidal-angle offset rzphi_offset, written as Geometry/offset), not ν — ν is the toroidal offset φ − 2πζ in Geometry/nu. The conclusion is unchanged: rsep is the inboard/outboard midplane crossing, hence R_midplane.

Also pushed H5 - IMPROVEMENT - Make the psi_boundary long_names honest: Equilibrium/psi_boundary is assigned a hardcoded 1.0, so advertising it as the boundary poloidal flux in Wb/rad was flatly wrong. It now annotates as the boundary value in the internal normalized convention (1 by construction) and carries no Wb/rad unit; psi_boundary_norm says it is 1 by definition of ψ_N. Recomputing them properly is #389.

Stack propagated: #368 (bugfix/366-ca-uninitialized) and #385 (feature/h5-runtime-records) both have this head merged in and pushed. #368 had one conflict in src/HDF5Schema.jl, resolved to keep both improvements — its zero-extent ca_left/ca_right long_names and this branch's dual rational_psi+rational_q dimension scales on the GGJ coefficient rows.

Local runtests_h5_schema.jl: 14/14 pass. runtests_rerun_from_h5.jl + runtests_fullruns.jl still running; will report.