GPEC - IMPROVEMENT - Make all input control structs immutable

[jhalpern30]

Continues issue #139, following the already-merged ForceFreeStatesControl freeze. (#358 has merged; this now targets develop directly.)

Why

A control struct built from the input TOML should be built once and then left alone, so a reader at a call site knows nothing behind it changes.

What

PerturbedEquilibriumControl, KineticForcesControl and ForcingTermsControl are now struct rather than mutable struct. The first two had no writers; ForcingTermsControl was filled in two steps because [[ForcingTerms.coil_set]] parses to a Vector{Dict} that cannot ride in the scalar kwarg splat — coil_sets_raw is now an explicit keyword after the splat.

EquilibriumConfig: the one that needed real work

setup_equilibrium read the EFIT g-file, binary-searched for the outermost closed flux surface, and wrote the result back into the config:

eq_input.config.psihigh = psihigh_safe

That write was load-bearing through aliasing — PlasmaEquilibrium.configisraw_profile.config, which is the only reason Sing.jl saw the clamped value — so it could not simply be deleted.

Fix: the resolved value moves onto the run-input structs, which are already mutable and already carry read-time-resolved values (bt_sign is the same pattern).

• DirectRunInput/InverseRunInput gain psihigh_resolved, defaulted to config.psihigh by an outer constructor, so all existing construction sites are untouched.
• resolve_psihigh! replaces the two duplicated clamp blocks; the solvers form their ψ grid from psihigh_resolved, and the efit_by_inversion mid-solve InverseRunInput rebuild now forwards it as an explicit argument instead of relying on the config alias.
• EquilibriumParameters.psihigh_resolved carries it onto the finished equilibrium for sing_lim! and the driver.

config.psihigh is now the user's request and is never written. Two other config writes had to go: eq_filename path resolution folds into the dict before construction, and the rerun path clears eq_filename on a dict copy (the caller's inputs is re-serialized into input/gpec_toml_raw). EquilibriumConfig(path::String) now delegates to the dict constructor.

setup_equilibrium's signature is unchanged, so no caller, benchmark or harness entry point moves.

Trade-off

config.psihigh is no longer authoritative — code wanting the plasma edge must read params.psihigh_resolved. The distinct field name makes a wrong read look wrong at the call site, and a new test asserts psihigh_resolved == rzphi_xs[end] <= config.psihigh across all three efit solvers plus that a psihigh=1.0 request survives unmodified on the config.

Also

reform_eq_with_psilim removed — it only ever gated a warning saying the reform was not implemented, so it could not affect a result.

gpec.h5 is unchanged: the config struct is never serialized, so the stored psihigh is the request and every replay re-clamps, as before.

Testing

No intended numerical change. Passing: runtests_equil 271/271 · runtests_kinetic 277/277 · runtests_rerun_from_h5 70/70 · runtests_sing 76/76 · runtests_fullruns 17/17 · runtests_coils · runtests_grid_refinement · runtests_parallel_integration · runtests_imas.

Regression harness: no change. local vs origin/develop @ 0b196ed8, identical env (julia 1.12.6, manifest pinned):

Case	Result
diiid_n1	48 unchanged, 0 changed
efit_fixedbdy_separatrix	5 unchanged, 0 changed
solovev_n1	21 unchanged, 0 changed

Every compared quantity is bit-for-bit identical (diff 0.0e+00, profile checksums identical), including the efit_fixedbdy_separatrix case that exercises the separatrix/psihigh path this PR reworks.

[jhalpern30]

@logan-nc I think the main thing to look at here is the logic/naming for psihigh_resolved - is there a better name/convention? I tried to make it clear that psihigh = input from TOML, psihigh_resolved = max resolved psi on the efit grid, and psilim is the maximum integration psi

[jhalpern30]

Regression harness: no change.local vs origin/develop @ 0b196ed8 — same env (julia 1.12.6, manifest pinned, 1 thread/6 BLAS).

Case	Result
diiid_n1	48 unchanged, 0 changed
efit_fixedbdy_separatrix	5 unchanged, 0 changed
solovev_n1	21 unchanged, 0 changed

Every compared quantity is bit-for-bit identical (diff 0.0e+00; profile checksums identical). efit_fixedbdy_separatrix was included deliberately — it exercises the separatrix / psihigh path this PR reworks.

[logan-nc]

Review

The design is right, and I'd have pointed at the same precedent you did. Moving the clamped psihigh off the config is the correct fix for that aliasing write, and bt_sign is the right model for it. I traced the whole path and found no gaps: every EquilibriumParameters construction site sets psihigh_resolved, nothing else reassigns .params, equilibrium_solver_by_inversion forwards the clamped value rather than leaning on the alias, the two-pass auto grid re-resolves consistently (with _validate_psi_nodes as a loud backstop), and no stale config.psihigh read survives anywhere in src/. Clearing eq_filename on a dict copy in Rerun.jl is a real latent-bug fix too, since inputs is re-serialized into Input/gpec_toml_raw.

I've updated the branch with develop (49 commits, clean merge) and pushed fixes for four things I found. Details below.

On the naming question

Keep psihigh_resolved. The request → resolved → psilim triple is coherent, it matches the read-time-resolution precedent already on those structs (bt_sign), and it reads consistently next to the Info/psilim / Info/qlim datasets. The distinct name is doing real work: a call site reading config.psihigh when it wanted the plasma edge now looks wrong, which is the whole point of the change. I considered renaming to psi_edge to match the h5 scalar naming standard and talked myself out of it — Info/psilim sets the precedent that this family keeps its spelling.

What I pushed

• newq0 was broken, and the freeze is not the whole story.DirectEquilibrium.jl:549 did equil_params.newq0 = -q0 where equil_paramsisraw_profile.config, so newq0 = -1 now throws on any direct-solver run. Digging in, the line was already dead before this PR: newq0::Int against -q0::Float64 threw InexactError for any non-integer q0. The real defect is the field type — newq0 is a target q(0) (the solver forms f0fac = f0^2*((newq0/q0)^2 - 1)), so Int rejects every realistic request like 1.05 at construction. Fixed both: newq0::Float64 = 0.0, and the sentinel now resolves into a local, matching InverseEquilibrium.jl:298-307. Docstring and the 18 example/test TOML comments follow.
• benchmark_q_vs_iota_edge.jl still mutated a frozen config — the scan loop kept deepcopy(eq_config) + cfg.mpsi = N; only reference_q() had been converted. Not covered by CI, so it was silently broken.
• Deleting reform_eq_with_psilim breaks existing decks.ForceFreeStatesControl is built by splatting the whole [ForceFreeStates] table as kwargs, so any user gpec.toml still carrying the key dies with an unknown-keyword MethodError — as does replaying any older gpec.h5, whose stored TOML blob goes through the same path. Added it to _DEPRECATED_FFS_KEYS; _drop_deprecated_keys! sits in main_from_inputs, so one entry covers both.
• gpec.h5 is not unchanged. True of EquilibriumConfig, which is never serialized — but EquilibriumParameters is dumped generically by field name, so psihigh_resolved silently becomes a new dataset. Harmless on the old base; after the develop merge it lands as Equilibrium/psihigh_resolved with no metadata and fails the new h5_metadata_check.jl walk. Fixed with one MAIN_H5_ANNOTATIONS entry — no rename and no EQUIL_H5_NAMES needed, since the writer falls back to the field name and annotate! defaults units to "1". Worth a line in the PR body: it is a schema addition.
• Tests.newq0 != 0 had zero coverage, which is exactly why the broken override stayed green. Added the -1 sentinel case (where f0fac vanishes, so ffac is exactly -1 and the revised profile is the negated baseline — asserted exactly), a non-integer target, psihigh_resolved on the CHEASE inverse path, and a deprecated-key parse test.

Two notes, neither blocking

• IMAS is never clamped."imas" is not in EFIT_KINDS, so resolve_psihigh! never runs for it and the field name promises a resolution that did not happen. Pre-existing, not a regression — I added a docstring sentence rather than change behaviour inside a freeze PR.
• The freeze is shallow.ForcingTermsControl.coil_sets_raw::Vector{Dict} can still be push!ed through the immutable struct, so "nothing behind it changes" is a little stronger than what the types now guarantee.

Verification

Tests: runtests_equil 283/283 (was 271) · runtests_sing 76/76 · runtests_kinetic 277/277 · runtests_grid_refinement 59/59 · runtests_h5_schema 14/14 · runtests_rerun_from_h5 · runtests_fullruns · runtests_coils — all green. benchmark_q_vs_iota_edge.jl runs and converges normally. I also confirmed against a real gpec.h5 that Equilibrium/psihigh_resolved is genuinely written (0.9995) with long_name and units=1 — annotate! skips missing paths, so the metadata test would otherwise have passed vacuously.

Regression harness — no change.local vs origin/develop @ b3abe074 (current tip, post-merge), identical env (julia 1.11.7, manifest 7027ae20 pinned, 1 thread/1 BLAS):

Case	Result
diiid_n1	48 unchanged, 0 changed
efit_fixedbdy_separatrix	5 unchanged, 0 changed
solovev_n1	21 unchanged, 0 changed

Every compared quantity is bit-for-bit identical (diff 0.0e+00, profile checksums identical), including efit_fixedbdy_separatrix, which exercises the separatrix/psihigh path this PR reworks. Re-run post-merge because the table in the description was taken against 0b196ed8, 49 commits back.

Verdict

The code is in good shape — the four defects are fixed, the suite and harness are green on the merged state, and I have no outstanding objections. But this still needs a third-party human reviewer's approval before it merges: I authored the fixes on this branch, so my sign-off cannot stand in for an independent review of them.

[matt-pharr]

@jhalpern30@logan-nc I will suggest we rebase this once #393 is done, it will be easier since that is when I will have finished the integrator abstraction work. #394 details a follow-up pass I want to do where we move around a bunch of functions into a new file hierarchy for forcefreestates to make more sense now that the integrators are abstracted, and also de-slop all the FFS comments and docstrings and code.

[logan-nc]

Looks good to me! Just a few minor cleanups ID'd in human read through

[logan-nc]

Overly verbose annotation

[logan-nc]

Again overly verbose. Stop calling out that it is "clamped" to the separatrix - I think a psin<=1 clamp is obvious to any physicist and doesn't need to be reiterated all over the place

[logan-nc]

this test seems excessive

[logan-nc]

Correction to my h5 naming call above

I got the dataset name wrong in my previous comment, and the reasoning I gave for it was wrong too. Pushed a fix in 49d72d1.

I said no rename and no EQUIL_H5_NAMES entry were needed, arguing that falling back to the field name reads fine next to Info/psilim. Re-reading docs/development/hdf5-conventions.md, that argument doesn't survive contact with the rule as written:

Fortran-era contractions (bt0, amean, crnt, qa, betan, li1) survive only as Equilibrium.EquilibriumParameters struct fields: EQUIL_H5_NAMES in src/HDF5Schema.jl maps each field to its dataset name

psihigh is exactly such a contraction, and the rule is scoped to scalar equilibrium parameters — so Info/psilim, which is run metadata in a different group, isn't the precedent I claimed. The doc also names the qualifier form explicitly (trailing _edge, cf. q_edge).

So the dataset is now Equilibrium/psi_edge via EQUIL_H5_NAMES[:psihigh_resolved]. That also matches what the code already calls this quantity internally (psi_edge = pe.rzphi_xs[edge_idx], Equilibrium.jl:156). It is deliberately not in the psi_boundary family: those describe the LCFS (psi_boundary_norm is 1 by definition), whereas this is the outermost surface actually formed, which sits inside it.

Two notes on the verification, since I over-claimed there as well:

• runtests_h5_schema.jl enforces group names only — the file says "Dataset (leaf) names are not" checked. So the 14/14 I quoted was never evidence the leaf name conformed. Leaf naming is doc-enforced at review time, which is why this needed a human-convention read rather than a green test.
• Confirmed against a real gpec.h5 that Equilibrium/psi_edge is present with the right value (0.9995), psihigh_resolved is gone, and long_name/units are attached. Worth doing explicitly because annotate! silently skips missing paths, so the metadata walk passes either way.

The struct field stays psihigh_resolved — the request/resolved/psilim triple in the Julia API is unaffected, and my recommendation to keep that name stands. Nothing else in the review changes; no numerical behaviour is touched, so the regression table above still holds.