Repository navigation
gpec.h5 schema: follow-ups from the #364 standardization audit #388
Description
Activity
- added a commit that references this issue
on 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:
- Item 1 understated the current state: the Galerkin
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. - Item 2 overstated it:
PerSurface/already writesm,nandrs, so the resonantq = m/nis derivable today. Onlyrational_psiis genuinely missing (andRoots//LayerWidths/still carry the bare index). - Item 4's
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.- Item 1 understated the current state: the Galerkin
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.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.
- Item 1 (ξ axis order) — in its minimal form: per-formalism groups stay, but the Galerkin writer transposes at write so every
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. - Item 2 (
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
- Item 1 (ξ axis order) — in its minimal form: per-formalism groups stay, but the Galerkin writer transposes at write so every
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 subgroup
name. Δ′ is a property of the surface, not of the solver, andSLAYER/GGJ/Tearingall want one
canonical 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, so
exactly 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/already
holds run metadata (git_version,mpert,psilim), soInfo/integratorfits the existing pattern.One conventions wrinkle worth settling explicitly. "Inputs live only under
Input/" forbids
echoing control-struct values into output groups, and read naively,Info/integratorlooks exactly
like 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 the
first 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 file
distinguishes them. Flip a deck'sintegratorand a golden value pinned from one formalism is
silently compared against the other. In #397 I pin the deck's declaredintegratorout of
Input/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; the
harness pin would then read it instead.Happy to take this one if nobody else has it — it is small, and #397 wants it.
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
developatb3abe074(#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_psiis(mode, solution, psi);ForceFreeStates/Solutions/GalerkinIntegration/Solution/xi_psiis(mode, psi, solution)— the same physical quantity, transposed. #364 unified the names (both arexi_psi/dxi_psidpsinow), eachdimsattribute is honest about its own order, and the Galerkinlong_namenow 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 apermutedimsat write plus updates tobenchmarks/verify_gal_{solution,ideal}.jland 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 writesm,nandrsper surface, so the resonantq = m/nis derivable — butrational_psiis absent, andRoots/andLayerWidths/carry only the opaquerational_index. Attach paths resolve relative to the annotated group, so cross-group dimension-scale attachment toSingularSurfaces/rational_psiis not mechanically possible — the fix is writingrational_psi(andrational_qfor symmetry with the rest of the schema) directly intoTearing/PerSurface/. The file's own (now stale) header comment suggests that was the original intent. Would make Tearing results natively plottable vs ψ or q likeSingularCoupling/already is.3. PE
Response/writes empty placeholder arrays where sibling code skips.src/PerturbedEquilibrium/Utils.jl:143-148unconditionally writesxi_psi,b_psi_area_weighted,Jb_theta,Jb_zeta,b_n,xi_nasComplexF64[]when not computed (so a rank-1 empty array carries a rank-2dimsattribute), 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 fromIdeal/and never re-emitted — table entries were trimmed accordingly, but writing the copies for symmetry is an option. (Equilibrium/b_normwas 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.
dimsuses bothsolution(arbitrary-amplitude basis index, e.g.ForwardIntegration/xi_psi) andeigenmode(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. SimilarlyTearing/PerSurface/lucould beS, the literature symbol for the Lundquist number — itslong_namealready 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 theMetricData.fsfield docs) describesmetric.fs[1..6]asg^ij·J. The code computesg_ij/J:v[i,:] = e_i/J(e.g.v[3,3] = 2π·R/jac, which is|e_ζ|/J, not∇ζ = 1/(2πR)), thenfs[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 insrc/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 isJ·b^ψ(χ₁ supplies the weight; what is absent is thejmatconvolution); and thearea = <J·|∇ψ|>_θ (Hamada: J is const over θ)caveat at:133is unnecessary — the(J·b^ψ)_m/A = b̄_midentity is exact in any SFL working coordinate.8. Stale agent memory.
.claude/agent-memory/fortran-physics-reviewer/known_issues_perteq.md:7-9statesxssis a zero placeholder; it is now solved as-A⁻¹(B·xmp1 + C·xsp)incompute_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).