Repository navigation
gpec.h5 schema: follow-ups from the #364 standardization audit #388
Description
Non-blocking items surfaced by the two independent reviews run against #364 (a whole-schema standardization audit and a devoted physics units review). Everything blocking was fixed in #364 itself; these were deliberately left out to keep that PR reviewable and because several are data-layout or out-of-scope-file changes.
Re-verified against develop at b3abe074 (#363 and #364 both merged). All items below still reproduce; the wording of items 1, 2 and 4 has been corrected where the original text was inaccurate.
Schema / data layout
1. ξ array axis order differs between integrator backends.
ForceFreeStates/Solutions/ForwardIntegration/xi_psi is (mode, solution, psi); ForceFreeStates/Solutions/GalerkinIntegration/Solution/xi_psi is (mode, psi, solution) — the same physical quantity, transposed. #364 unified the names (both are xi_psi/dxi_psidpsi now), each dims attribute is honest about its own order, and the Galerkin long_name now carries an explicit warning ("note the psi/solution axis order differs from ForwardIntegration"), so the trap is at least signposted in the file. Cross-backend tooling that reads one and then the other still silently gets a transpose. Unifying requires a permutedims at write plus updates to benchmarks/verify_gal_{solution,ideal}.jl and friends — a data change, not a naming one, so it wants its own PR and a regression run.
2. Tearing/PerSurface/ cannot be plotted against ψ.
PerSurface/ already writes m, n and rs per surface, so the resonant q = m/n is derivable — but rational_psi is absent, and Roots/ and LayerWidths/ carry only the opaque rational_index. Attach paths resolve relative to the annotated group, so cross-group dimension-scale attachment to SingularSurfaces/rational_psi is not mechanically possible — the fix is writing rational_psi (and rational_q for symmetry with the rest of the schema) directly into Tearing/PerSurface/. The file's own (now stale) header comment suggests that was the original intent. Would make Tearing results natively plottable vs ψ or q like SingularCoupling/ already is.
3. PE Response/ writes empty placeholder arrays where sibling code skips.
src/PerturbedEquilibrium/Utils.jl:143-148 unconditionally writes xi_psi, b_psi_area_weighted, Jb_theta, Jb_zeta, b_n, xi_n as ComplexF64[] when not computed (so a rank-1 empty array carries a rank-2 dims attribute), while the surrounding function and the main writer both use !isempty(...) && guards or zero-extent sentinels. Pick one pattern file-wide.
4. Optional completeness. EulerLagrangeMatrices/Kinetic/{D,E,H,F} are shared unchanged from Ideal/ and never re-emitted — table entries were trimmed accordingly, but writing the copies for symmetry is an option. (Equilibrium/b_norm was also listed here; it has since been folded into #389, which owns the decision to compute the field or delete it. Do not fix it in both places.)
5. Vocabulary watch item. dims uses both solution (arbitrary-amplitude basis index, e.g. ForwardIntegration/xi_psi) and eigenmode (normalized generalized eigenvector index, e.g. FreeBoundaryStability/eigenmode_energies). These look like genuinely distinct concepts, but a schema owner should confirm rather than leave it implicit. Similarly Tearing/PerSurface/lu could be S, the literature symbol for the Lundquist number — its long_name already reads "Lundquist number S per surface", so only the leaf name is out of step. Low value, listed for completeness.
Documentation defects found during the units review
6. Metric coefficient labels are dimensionally inverted. src/ForceFreeStates/Fourfit.jl:35-40 (the docstring listing "1. g^ψψ · J … 6. g^ψθ · J", and the MetricData.fs field docs) describes metric.fs[1..6] as g^ij·J. The code computes g_ij/J: v[i,:] = e_i/J (e.g. v[3,3] = 2π·R/jac, which is |e_ζ|/J, not ∇ζ = 1/(2πR)), then fs[k] = dot(v_i, v_j) * jac. Verified independently. Numbers are unaffected — only the label is wrong — but it will mislead the next person auditing units. The same wording is repeated in src/PerturbedEquilibrium/FieldReconstruction.jl:611, and a nearby block comments its covariant basis vectors as "Contravariant basis vectors WITHOUT /jac". Highest value-per-effort item here: a wrong dimensional label inside a physics kernel.
7. Two smaller comment corrections in FieldReconstruction.jl: the # b^ψ (not J-weighted) (:797) / # b^ψ (no Jacobian) (:191) comments describe a quantity that is J·b^ψ (χ₁ supplies the weight; what is absent is the jmat convolution); and the area = <J·|∇ψ|>_θ (Hamada: J is const over θ) caveat at :133 is unnecessary — the (J·b^ψ)_m/A = b̄_m identity is exact in any SFL working coordinate.
8. Stale agent memory. .claude/agent-memory/fortran-physics-reviewer/known_issues_perteq.md:7-9 states xss is a zero placeholder; it is now solved as -A⁻¹(B·xmp1 + C·xsp) in compute_clebsch_displacements. The MEMORY.md index line says the same.
Suggested split
Items 1–3 are the substantive ones and each wants its own PR with a regression run. Items 6–8 are documentation/comment-only and could land together in a single cheap pass. Items 4–5 are judgement calls for whoever owns the schema.
Context: #226 (schema overhaul), #363 and #364 (merged), #389 (equilibrium parameter cleanup, owns b_norm).
Activity
ALL - REFACTOR - Unify h5 coordinates and vector-component naming; au…
logan-nc commentedon Aug 15, 2026
Re-verified every item against
developatb3abe074(both #363 and #364 merged). Nothing here has been fixed in the interim — the commits that landed after the audit ran were the Clebsch rename, the Equilibrium scalar overhaul and thepsi_boundarylong_name fix, none of which touch these files.Three items were reworded because the original text was inaccurate:
xi_psilong_namenow carries an explicit "note the psi/solution axis order differs from ForwardIntegration" warning, so the transpose trap is signposted in the file even though it is not fixed.PerSurface/already writesm,nandrs, so the resonantq = m/nis derivable today. Onlyrational_psiis genuinely missing (andRoots//LayerWidths/still carry the bare index).b_normhalf is now owned by Equilibrium parameters: compute the stubbed flux bookkeeping fields, and collapse the duplicate psi-on-axis variables #389, which decides whether to compute the field or delete it. Removed from here so it does not get fixed twice.Also added line references throughout and a suggested split: items 1–3 each want their own PR with a regression run, items 6–8 are comment-only and could land as one cheap pass, items 4–5 are schema-owner judgement calls. Item 6 is the highest value per unit effort — a dimensionally inverted label (
g^ij·Jdocumented where the code computesg_ij/J) sitting in a physics kernel's docstring.logan-nc commentedon Aug 15, 2026
Assigning to @matt-pharr.
Re-verified against
developatb3abe074— all eight items still reproduce, and the body now carries line references so nothing needs re-hunting.Where I would start, if it helps: item 6 is the highest value per unit effort.
src/ForceFreeStates/Fourfit.jl:35-40documentsmetric.fs[1..6]asg^ij·J, but the code computesg_ij/J— the numbers are correct, only the label is inverted, and it sits in a physics kernel where it will mislead the next units audit. Items 7 and 8 are comment-only corrections in the same neighbourhood and could ride along in one cheap pass.Items 1–3 are the substantive ones and each wants its own PR plus a regression run; item 1 in particular is a data-layout change (
permutedimsat write) that also touchesbenchmarks/verify_gal_{solution,ideal}.jl. Items 4–5 are schema-owner judgement calls rather than defects.One coordination note:
Equilibrium/b_normwas originally listed under item 4 here but is now owned by #389 (@jhalpern30), which decides whether to compute the field or delete it outright. It has been removed from this issue so it does not get fixed in both places.matt-pharr commentedon Aug 15, 2026
Coordination note from the #393 side (ForceFreeStates integrator abstraction): we are incorporating items 1 and 2 into #393 as its final commit, and leaving the rest here for follow-ups.
Solutions/*/xi_*dataset shares the ForwardIntegration axis order (dimsattributes and the gal readers/benchmarks updated to match). Rationale for pulling it into FFS - REFACTOR - Publish solves as ForceFreeStatesResult; staged main; solve API (refactor plan interface PR) #393: that PR's whole contract is "same physics ⇒ same layout regardless of integrator" — it already unified the Δ′/PEST-3 payload into oneDeltaPrimeDataon sharedSingularSurfaces/paths — and doing the ξ half there means the gal regression cases re-baseline once instead of twice. The full unification (writing gal ξ through the sharedSolutionProfilesinterchange type that FFS - REFACTOR - Publish solves as ForceFreeStatesResult; staged main; solve API (refactor plan interface PR) #393 introduces) stays future work;SolutionProfilesis the natural vehicle for it when someone picks it up.Tearing/PerSurface/rational_psi/rational_q) — FFS - REFACTOR - Publish solves as ForceFreeStatesResult; staged main; solve API (refactor plan interface PR) #393 enables Galerkin-fed SLAYER, and gal's Δ′ indexes the in-domain subset of the singular surfaces, so tearing output must identify its own surfaces to be unambiguous. That makes this item practically owed by FFS - REFACTOR - Publish solves as ForceFreeStatesResult; staged main; solve API (refactor plan interface PR) #393 rather than optional.Staying here on the issue: item 3 (we'd suggest deciding the empty-vs-sentinel pattern once, aligned with #368's zero-extent sentinels, after both PRs land), items 4–5 (schema-owner calls), and items 6–8 (fit a comment-audit pass in the #354 style).
🤖 Generated with Claude Code
d-burg commentedon Aug 18, 2026
Adding a schema item from the #393 Δ′ unification, since @matt-pharr asked there whether Δ′ should
move into the per-integrator folders and this issue is the right home for it.
Recommendation: keep Δ′ where it is, and record the formalism in one field.
Against moving it: this issue's parent principle consolidates per-rational-surface results under
SingularSurfaces/regardless of which algorithm produced them, with provenance in the subgroupname. Δ′ is a property of the surface, not of the solver, and
SLAYER/GGJ/Tearingall want onecanonical path rather than dispatching on backend. Moving it also re-breaks the harness cases #393
just re-pointed.
For the single field: #393 removed
gal_flagand made"galerkin"a standalone integrator, soexactly one formalism now runs per invocation and a global field is unambiguous. That was not true
before —
gal_flag = truelayered gal onto riccati and both Δ′s coexisted in one file.Info/alreadyholds run metadata (
git_version,mpert,psilim), soInfo/integratorfits the existing pattern.One conventions wrinkle worth settling explicitly. "Inputs live only under
Input/" forbidsechoing control-struct values into output groups, and read naively,
Info/integratorlooks exactlylike that. The distinction is that a record of which code path executed is derived output — an
observation about the run — not an echo of the requested configuration. Worth one sentence in
hdf5-conventions.md, otherwise the next schema audit flags it, which is how this came up in thefirst place.
Why it matters beyond tidiness. After #393 the Galerkin and BVP Δ′ share
SingularSurfaces/Delta_prime_matrixand are both in the PEST-3 convention, so nothing in the filedistinguishes them. Flip a deck's
integratorand a golden value pinned from one formalism issilently compared against the other. In #397 I pin the deck's declared
integratorout ofInput/gpec_toml_rawas a stopgap, but that only works for TOML-driven runs —solve(prob, Galerkin())writes no input blob, so an API-produced file has no recoverable provenance at all. A written
Info/integratorfixes both, and records what actually ran rather than what was asked for; theharness pin would then read it instead.
Happy to take this one if nobody else has it — it is small, and #397 wants it.