ALL - REFACTOR - Module-mirroring CamelCase HDF5 schema (issue #226, PR A)

[logan-nc]

Summary

PR A of two stacked PRs closing #226: physics-first restructure of the gpec.h5 schema into 10 CamelCase top-level groups, plus the input-echo flush. PR B (self-describing metadata: long_name/units/dimension scales) follows, stacked on this branch. The settled convention is codified in the new docs/development/hdf5-conventions.md and enforced by the new test/runtests_h5_schema.jl.

Governing principle: the schema must be intuitive to a plasma physicist who is not a developer of this code. Groups are CamelCase at all levels, datasets snake_case, data-driven tokens (coil sets, method names, scan indices) verbatim, and inputs live only under Input/ — every other group is derived output.

Schema map (old → new)

Old	New
info/, input/, equil/, locstab/	Info/, Input/ (RawInputs/{Equilibrium,ForcingTerms,Coils}), Equilibrium/, LocalStability/
splines/{profiles,rzphi}	Equilibrium/{Profiles,Geometry}
integration/	ForceFreeStates/Solutions/ForwardIntegration/
galerkin/solution/, galerkin/msing	ForceFreeStates/Solutions/GalerkinIntegration/{Solution/,msing}
galerkin/match/**	ForceFreeStates/Solutions/GalerkinIntegration/Match/** (kept — see below)
galerkin/{delta,pest3_*,sing_*,di,alpha,delta_coil}	SingularSurfaces/GalerkinDeltaPrime/* (Δ′ consolidates by physics next to delta_prime_matrix + GGJ coefficients)
matrices/{ideal,kinetic}	ForceFreeStates/EulerLagrangeMatrices/{Ideal,Kinetic}
FreeBoundaryStability/, EdgeScan/	ForceFreeStates/{FreeBoundaryStability,EdgeScan}/
singular/, singular/kinetic/	SingularSurfaces/, SingularSurfaces/Kinetic/
perturbed_equilibrium/*	PerturbedEquilibrium/{ForcingModes,Response,ResponseMatrices,SingularCoupling,Energies}
kinetic_forces/<m>/records, kinetic_forces/matrices_<m>	KineticForces/<m>/{EnergyIntegrals,KineticMatrices}
slayer/*	Tearing/{PerSurface(/DpMatrix),Roots,LayerWidths,Diagnostics/{ValidRoots,Poles,FilteredRoots},Scan/Surface_<k>}
kinetic/, slayer/settings/	deleted (input echoes; covered by Input/gpec_toml_raw + rerun)

Reserved for the upcoming integrator options (documented, not written): ForceFreeStates/Solutions/RiccatiIntegration/.

⚠️ Dropped / changed outputs — stakeholder sign-off requested

• kinetic/ and slayer/settings/ are deleted (pure input echoes; Rerun.jl reconstructs all control structs from Input/gpec_toml_raw).
• galerkin/match/ is KEPT (renamed to GalerkinIntegration/Match/), reversing the earlier "debug-only, drop it" decision: PR InnerLayer (GGJ) - NEW - rotated-ray collocation backend (:ray), robust to |Q| ~ 500 #323 deliberately added new datasets there days ago (Inner/b_<i>, InnerParams/{E,F,G,H,K,M,taua,taur,v1}, bpen, …). The drop — and the question of re-homing bpen/InnerParams — is deferred to its own PR after consulting the Galerkin owner.
• Regression tracking retired only kinetic_factor (×2 solovev_kinetic cases — group flushed). The 3 galerkin/match/* tracked quantities are repointed, not retired.
• Fixed 8 benchmark/example readers of the long-removed vacuum/ group (et/ep/ev/wt0 → ForceFreeStates/FreeBoundaryStability/*) — these were already broken on develop.

Back-compat

Clean break in src/ readers. The regression-harness extractor gains a LEGACY_PREFIX_MAP fallback (new→old, tried when the new path is missing) so --refs comparisons and --ref-range bisects keep working across the rename boundary — exercised by the develop side of the harness run below.

Regression harness (mandatory gate) — zero numerical movement

regress --cases <all 12> --refs develop,local: every surviving quantity OK with 0.0e+00 diff.

Case	Result
diiid_n1	48 unchanged
solovev_n1	21 unchanged
solovev_multi_n	15 unchanged
diiid_slayer_n1	16 unchanged
gal_resistive_diiid	10 unchanged
gal_resistive_pe	4 unchanged, 4 N/A (pre-existing stale *_flux/penetrated_field tracks — N/A on develop too, untouched by this PR)
solovev_kinetic_calculated	14 unchanged
solovev_kinetic_nuzero	14 unchanged
solovev_kinetic_ntv	6 unchanged
efit_fixedbdy_separatrix	5 unchanged
ggj_reference / ggj_ray_q500i	4 / 4 unchanged

Other verification

• New test/runtests_h5_schema.jl: full Solovev run → all group paths CamelCase/whitelisted, legacy top-level groups absent (20/20).
• runtests_rerun_from_h5.jl: bit-for-bit whole-schema source↔replay round-trip passes; runtests_slayer_runner.jl 62/62; runtests_coils.jl, runtests_fullruns.jl pass.
• Docs build clean; repo-wide grep sweep for old tokens clean (only the extractor legacy map remains, by design).

Coordination

This PR is mechanical but wide — it will conflict textually with open PRs touching writers (#354, #345, #339, #318). Please review + merge promptly and warn authors to rebase; PR B is additive/low-conflict.

Post-merge: re-run the harness once on develop to cache baselines at the merge commit; close #226 citing the conventions doc.

⚠️⚠️ 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.

🤖 Generated with Claude Code

https://claude.ai/code/session_0129rSTCmYJDBbcH9khHqYnz

[logan-nc]

Full test/runtests.jl suite (all 57 testsets) run on the stacked branch (refactor/hdf5-metadata @ 0ece9c4, which includes this branch): all pass, zero failures/errors. This completes the approved plan's verification checklist for both PRs (schema test, SLAYER/coils/fullruns/rerun, docs build, grep sweep, regression harness — zero numerical movement — and the full suite).

[logan-nc]

@matt-pharr please approve, remove or clean-up the ForceFreeStates/Solutions/GalerkinIntegration/Match/**. PRs landed mid-h5 cleanup that added a bunch of Match outputs that were important looking so I decided to keep that section and defer to you here.

[logan-nc]

@d-burg @jhalpern30 @priyanshlunia please at least skim the Schema map table above and submit a review confirming that you have. Of course, at least one full diff review is needed... any volunteers? Should be relatively easy to skim since there is no math to check.

[jhalpern30]

Only some minor comments. I like the new naming convention

Something I was thinking about is that I think right now, Slayer and Galerkin have their own HDF5 write functions (which take in the existing HDF5 file). I'm wondering if it would simplify some code elsewhere to write other parts of the outputs incrementally too? Would involve tracking down any data we are lugging around for the sole purpose of dumping to HDF5.

No idea if this would simplify things (and it doesn't need to go in this PR), but just a thought

[jhalpern30]

This seems like an annoying thing to carry around. IMO could just be a "hey developers, make sure you run your regressions with --force or whatever once to reset your outputs" and make GPEC v2.0.0 just assume this output tree

[jhalpern30]

I don't like this. Its a minor change, but just doing base = "PerturbedEquilibrium/SingularCoupling/" etc. was fine

[jhalpern30]

Not a fan of these consts either, but up to you, I could see it going either way. The whole "keep writer and rerun reader from drifting apart" could just be fixed by ctrl + f when changing things

[jhalpern30]

This is a weird test - if I'm getting this right, we run the updated version of the code, which has these names removed, and check if the names are still there? I think just having it do a quick run and check for anything not camel case in the HDF5 is ok

[logan-nc]

Something I was thinking about is that I think right now, Slayer and Galerkin have their own HDF5 write functions (which take in the existing HDF5 file). I'm wondering if it would simplify some code elsewhere to write other parts of the outputs incrementally too? Would involve tracking down any data we are lugging around for the sole purpose of dumping to HDF5.

Pros of consolidated h5 writing:

• As a general rule file IO is slow and shouldn't be done in loops, but I think IO after major module / calculations complete should be ok performance wise.
• It forces folks to make in-memory versions of all outputs (structs or whatever), which helps modularity / integration later on. Allowing lots of local writes invites folks to be lazy and just write outputs for their our post-processing instead of returning them nicely in clean memory objects that could then be used downstream or by fuse, tokamaker, etc. This was what happened in Fortran GPEC, for example. Any time I wanted submodule cross-talk I inevitably ended up having to make new module-wide variables in gpout and it became a mess. If the subroutines had natively been designed to return their results in memory this would have been much cleaner.
• Easy maintenance - If all the writing is in one (or a few) places it is easy to check and/or update conventions, organization, etc. The more spread out, the more likely some outputs won't look like others (esp. conventions for dims, units, long_names, etc.) or will have oddly similar names (see delta_n vs delta_mlow example from ALL - IMPROVEMENT - Self-describing HDF5 metadata (issue #226, PR B) #364).

Pros of distributed writing:

• Outputs are written as calculations finish, so failed runs do still have partial results available for analysis on disk.

Possible balance:

• Incremental by module: Equilibrium runs + writes, FFS runs + writes, Tearing runs + writes, PE runs+writes, etc.

Open question:

• Can we consolidate the FFS to a single write using the abstracted integrator refactor @matt-pharr is working on? I assume we should open an issue for this and leave it for a later PR?

Thoughts @matt-pharr @d-burg?

[d-burg]

I personally think I like the balanced compromise where components of the h5 write incrementally by module (eq, FFS, then tearing). This is something that would be much easier to do in h5 than it was in the fortran netcdf interface, and we will retain partial results in case of a crash

[priyanshlunia]

Looks good to me -- new names are generally more descriptive and easier to parse.

[d-burg]

Following up on my own comment about incremental per-module writes — I dug into where this actually stands, and it turns out we're most of the way there already. Filed as #374 rather than expanded here; summary of what I found:

Module-level staging already exists. PE opens "cw", KineticForces "cw", the coil snapshot "r+", and Tearing "r+" (falling back to "w" if no earlier stage wrote the file). The Tearing stage is already the model for what I was describing — _run_slayer_stage wraps itself in a try/catch whose comment says a failure "must not discard the equilibrium / stability / PE results already computed."

The gap is inside stage 1. The equilibrium solve, ODE integration, free_run!, Δ′ BVP and Galerkin solve all complete before write_outputs_to_HDF5 opens the file with "w". So the longest and most crash-prone stretch of a run is the one with nothing on disk behind it — a crash in the integration loses the equilibrium that finished minutes earlier. That's the piece worth doing.

Not proposing it for this PR. Two reasons beyond the obvious one that this PR is already approved and the diff is large enough:

• This PR's safety argument is "pure rename, zero regression-harness movement." A write-ordering change is a behaviour change, and that evidence doesn't cover it — it needs crash injection and partial-file read tests instead.
• The change needs a completion marker on the file, because once writes are incremental a crashed run leaves a readable-but-incomplete gpec.h5 containing a valid Input/gpec_toml_raw, which is exactly what Rerun.jl starts from. ALL - IMPROVEMENT - Self-describing HDF5 metadata (issue #226, PR B) #364's write_root_attrs! is the natural home for that marker, so the work wants to sit on top of ALL - IMPROVEMENT - Self-describing HDF5 metadata (issue #226, PR B) #364 rather than here.

Also worth flagging while I'm in this thread, since it affects the merge rather than the review:

This PR is currently CONFLICTING against develop (22 commits behind). A trial merge produces five conflicts — benchmarks/gal_validation/{gal_beta_scan,gal_epsilon_scan,pfac_study/gal_pfac_scan2d}.jl (modify/delete: 1d30dfba removed them from tracking on develop while this branch modified them in the f["singular"] sweep), plus content conflicts in src/ForceFreeStates/EulerLagrange.jl and src/GeneralizedPerturbedEquilibrium.jl.

The three gal_validation ones need a decision rather than a mechanical resolution — those files are gone from develop, so the legacy-path fixes this branch made to them are moot. Same applies to the equivalent fix in #368. Since resolving these changes what would actually land, it's worth a second look at the resolution before merging, even though the approvals are already in.

[d-burg]

Merged current develop into this branch (43727d68) so it stops being CONFLICTING and CI can run on the tree that would actually land. Documenting every resolution here, because the three existing approvals were given on f07c0799, which is not this tree — please re-confirm against the resolved version, @jhalpern30 @priyanshlunia @logan-nc.

Five conflicts: two content, three modify/delete.

---

1. src/GeneralizedPerturbedEquilibrium.jl — the only one with real substance

Neither side was correct on its own. Merge base (5b6ba34a) had:

if ctrl.local_stability_flag
    locstab_xs = intr.locstab.cache.x
    out_h5["locstab/di"] = intr.locstab.y[:, 1] ./ locstab_xs

• This branch changed only the names (locstab/ → LocalStability/, singular/di0 → SingularSurfaces/di0), keeping intr.locstab / ctrl.local_stability_flag.
• develop separately changed only the structure — intr.locstab → a bare locstab kwarg, ctrl.local_stability_flag → locstab !== nothing (0ad362d0, reduce-whole-struct-passing / freeze-locstab).

Resolved as: develop's structure + this branch's names.

if locstab !== nothing
    locstab_xs = locstab.cache.x
    out_h5["LocalStability/di"] = locstab.y[:, 1] ./ locstab_xs

Taking this branch's side wholesale would have left a dangling intr.locstab: the function signature (locstab::Union{FastInterpolations.CubicSeriesInterpolant,Nothing}=nothing) and its call site (locstab=locstab) both sit outside the conflict region and auto-merged to develop's kwarg form, so the intr.locstab references would no longer have resolved.

Correctness check worth repeating: after resolution, the diff between this file and develop is purely the rename — every structural line is develop's. That's exactly what this PR claims to be, so the resolution is self-verifying.

2. src/ForceFreeStates/EulerLagrange.jl — docstring only

This branch's sole edit to the file was euler.h5 → gpec.h5 inside the eulerlagrange_integration docstring. develop rewrote that docstring wholesale and deleted the sentence containing the filename. Took develop's version — this branch's change is subsumed, nothing lost.

3. benchmarks/gal_validation/{gal_beta_scan,gal_epsilon_scan,pfac_study/gal_pfac_scan2d}.jl — modify/delete ×3

develop untracked these in 1d30dfba (BENCHMARKS - CLEANUP). This branch had modified them as part of the f["singular"] legacy-path sweep. Accepted the deletion — the sweep edits are moot for files that no longer exist.

⚠️ This also moots the equivalent fix in #368, which states it "fixed a leftover legacy haskey(f["singular"], ...) in both scans that escaped the #363 sweep." Those scans are gone from develop; that part of #368 will need dropping when it rebases.

---

Deliberately not included

@jhalpern30's four review comments (the extractor legacy-name compat, the Analysis/PerturbedEquilibrium.jl base-path style, the H5_* consts, and the legacy-name test) are not addressed here. They're optional style calls for @logan-nc to make, and folding them into a conflict resolution would make this merge commit un-reviewable. Same for anything from my review on #364.

Verification

• Zero conflict markers remaining anywhere in src/, test/, docs/.
• Grep sweep confirms no legacy lowercase h5 paths (locstab/, singular/, perturbed_equilibrium/, kinetic_forces/, galerkin/, slayer/, f["singular"]) were reintroduced by the merge — this branch's rename still holds everywhere.
• Both resolved files parse under Julia 1.11.6; continuation-line alignment and the 180-char margin checked by hand on the lines I wrote.
• I did not run the test suite locally — no instantiated environment. CI on this push is the check. Please don't treat this comment as a substitute for it.

[logan-nc]

Branch updated: 43727d68..f76b31ce — a second develop sync plus fixes from a fresh independent review (three parallel reviewer agents over the full diff at 43727d68, plus this thread). Three commits, each reviewable alone:

1. Merge of current develop (0b196ed8) — resolutions documented (same recipe as 43727d6: develop's structure + this branch's names)

Two content conflicts, both in src/GeneralizedPerturbedEquilibrium.jl:

• ForwardIntegration block: develop's singular-coupling bugfix changed dxi_psi's content from du_store[:, :, 1, :] to the now-3-D du_store. Kept develop's expression under this branch's ForceFreeStates/Solutions/ForwardIntegration/ paths.
• FreeBoundaryStability block: VAC - REFACTOR - Give the Vacuum module its own output struct #358's vacuum-response-struct refactor replaced ctrl.vac_flag ? vac_data.* with free_energies !== nothing ? free_energies.*. Kept develop's guard and fields under this branch's ForceFreeStates/FreeBoundaryStability/ paths.

Post-merge grep confirms no legacy lowercase paths reintroduced.

2. Independent-review fixes

The review verdicts: numerics-neutrality audit PASS (every hunk is a path string/comment/sanctioned deletion; both merge resolutions verified self-consistent); zero missed executable paths repo-wide. What it did find, now fixed: a user-visible @info log still pointing at integration/xi_*; the populate_dense_xi docstring citing three dead path families; four permanently-dead tracked h5paths in gal_resistive_pe.toml (resonant_flux/penetrated_field/C_resonant_flux — stale since the Pharr field-representation rename, pre-existing on develop; now repointed to the *_area_weighted_field leaves so they actually track); direct unit tests for _group_name_ok's data-driven whitelist (the ideal-deck walk never exercised it); conventions-doc scope note (the harness's synthetic ggj/* fixture is exempt); two comment nits.

3. Review-comment dispositions (@jhalpern30)

• Legacy-path map: dropped, as you suggested. Renames are now clean breaks everywhere; the conventions doc's back-compat section now says exactly that (cached pre-rename quantities keep old comparisons alive by name; fresh cross-boundary extraction reports missing; re-baseline with --force).
• Analysis/PerturbedEquilibrium consts: reverted to the inline base = "..." style.
• H5_* writer↔rerun consts: dropped; paths inlined in both files with cross-reference comments marking the mirrored pair.
• Schema test: simplified — the legacy-name loop is gone (the CamelCase rule already forbids every lowercase name); two targeted asserts remain for the moved CamelCase groups (FreeBoundaryStability, EdgeScan).

Verification & the harness under the new policy

Targeted suites green on this tip: schema naming (incl. new whitelist unit tests), SLAYER runner 62/62, both rerun-from-h5 round-trips, fullruns, coils. CI runs the full suite on this push.

Harness note (policy consequence of dropping the map, please weigh in if this bothers you): a develop,local harness comparison is now impossible by design for renamed quantities — develop writes the old schema and there is no translation layer. The empirical develop-parity evidence for the rename is the all-OK table posted earlier on this PR (produced while the map existed); everything merged in since carries develop's own PR validation, and the branch-side commits since are path/comment/tooling-only (audited + tested above). Post-merge, develop re-baselines with --force at the merge commit.

Re-review needed

The three approvals predate 43727d68 and this push. @jhalpern30 @priyanshlunia @d-burg please re-confirm on f76b31ce. @matt-pharr — the GalerkinIntegration/Match/** keep-or-clean decision is still yours; nothing here touched it.

🤖 Generated with Claude Code

https://claude.ai/code/session_0129rSTCmYJDBbcH9khHqYnz