Make gpec.h5 a self-contained rerun snapshot

[logan-nc]

Closes #171.

Summary

• Store the full merged TOML, raw equilibrium arrays, and (when enabled) forcing-mode data inside gpec.h5 so a run can be replayed from any directory without needing the original g-file / CHEASE / auxiliary TOML (sol.toml / lar.toml) / forcing.dat.
• main("run.h5") dispatches to a new main_from_h5 replay entry point. It rebuilds a DirectRunInput / InverseRunInput from the stored raw arrays, or reconstructs a SolovevConfig / LargeAspectRatioConfig from the merged TOML sections, and hands the result to setup_equilibrium via the existing additional_input channel — no sentinel path strings.
• Default output is <source_basename>_rerun.h5 in pwd() so the source is never overwritten. --output-dir, --output-name, --override-file <toml>, and repeatable --override section.key=value CLI flags let users retarget the output or tweak TOML fields on the fly.
• The field-by-field input/ForceFreeStates/* and input/EQUIL_CONTROL/* mirrors are replaced with a single input/gpec_toml_raw string (full merged TOML), plus input/raw_inputs/equilibrium/* and (when PE is enabled) input/raw_inputs/forcing_terms/* groups.
• Scope note: this PR is the ''always recompute'' variant per the issue discussion — every rerun re-runs equilibrium + stability + PE from the stored inputs. Stage skipping (reusing stored ODE state, vacuum data, etc.) is deferred to a follow-up issue, as is snapshotting filepath-based wall polylines (all current examples use built-in wall shapes).
• Removed group readers: a repo-wide grep confirms nothing in the regression harness, test suite, analysis scripts, or benchmarks/ directory reads input/ForceFreeStates or input/EQUIL_CONTROL. Worth flagging for any downstream notebooks that might.

Test plan

• Full unit test suite: 681/681 tests pass, including 16 new rerun tests in test/runtests_rerun_from_h5.jl (self-contained replay matches source bit-for-bit; --override ForceFreeStates.eulerlagrange_tolerance=1e-6 propagates; rerun refuses to overwrite its own source)
• Regression harness vs develop on solovev_n1: 20/20 unchanged, zero diffs
• Regression harness vs develop on diiid_n1: 26/26 unchanged, zero diffs
• Manual smoke test — Solovev analytic: source and replay produce bit-for-bit identical et[1], ep[1], ev[1], mpert, msing, singular ψ/q
• Manual smoke test — DIIID-like direct-EFIT with forcing.dat: source and replay produce bit-for-bit identical et[1], delta_prime, island_half_width, ||resonant flux|| from a temp dir that contains only gpec.h5

🤖 Generated with Claude Code

[jhalpern30]

@claude review, keep it short

[claude]

Code Review

The overall design is sound: snapshotting raw arrays + merged TOML into gpec.h5 and dispatching via main_from_h5 is a clean way to achieve self-contained reruns. The refactor of main → main + main_with_inputs is well-executed and the backward-compatibility outer constructors handle pre-existing call sites cleanly.

Issues

1. Test coverage gap: EFIT/CHEASE rerun path has no automated test

The new test suite only exercises the Solovev (analytic/sol) rerun path. The build_direct_from_raw and build_inverse_from_raw round-trips for EFIT and CHEASE equilibria are only manually smoke-tested. These paths include non-trivial array reconstruction logic (spline rebuilding from raw node arrays), and a regression would not be caught by CI. At minimum, add an automated test using the existing regression_diiid_like_ideal_example fixture (or a minimal EFIT case) to cover the kind = "direct" path.

2. Silent string fallback in parse_override_flag can mask type errors

# src/Rerun.jl
parsed_val = try
    parsed = TOML.parse("_rerun_override_ = $rhs")
    parsed["_rerun_override_"]
catch
    # Fall back to string if TOML parse fails (e.g., unquoted bare words).
    rhs
end

A bare catch here silently converts any unparseable RHS to a string. For example, --override ForceFreeStates.npert=blorp stores "blorp" as a String, which passes through apply_toml_overrides! and only errors later (or silently does nothing) when ForceFreeStatesControl tries to use it as an integer. The user sees no warning at the point of the mistake. Replace with catch e and emit a @warn or re-throw with a message indicating that the value was interpreted as a string.

3. Deprecated equil.toml path produces silently non-rerunnable snapshots

When gpec.toml has no [Equilibrium] section and the run falls back to equil.toml, inputs["Equilibrium"] is never populated in the main function's inputs dict. The stored gpec_toml_raw blob therefore has no [Equilibrium] section, and main_from_h5 will crash with a KeyError on inputs["Equilibrium"]. The run completes successfully and writes an input/gpec_toml_raw, so the user has no indication that the snapshot is not rerunnable until they actually try.

Options: either add a @warn "equil.toml runs produce non-rerunnable snapshots; migrate to gpec.toml [Equilibrium]" at snapshot time, or merge the parsed equil.toml content into inputs["Equilibrium"] before calling main_with_inputs.

4. No test for --override-file

The flag is documented and implemented, but there is no test exercising it. Since it follows the same apply_toml_overrides! path as --override, a single test case verifying that a TOML file override propagates (e.g., changes mpert or tolerance) would provide meaningful coverage without much additional setup.

Minor

5. File-level "docstring" in Rerun.jl

"""
    Rerun support: helpers for saving run inputs into gpec.h5 and replaying them.
    ...
"""

A bare string literal at the top of an included file is not a Julia docstring — it is evaluated and discarded. Use # comments for file-level documentation, consistent with the rest of the codebase.

[jhalpern30]

This looks very cool! Going to hold off on reviewing it until #202 is merged in, since there are some conflicts between the two and I've already started reviewing that and working with Calin to get it merged in. Once that's done, I think there's an interesting way to work it into this branch and make the inputs logic concise. Perhaps something like a:

function main(combination of an h5, dd, toml, etc.)
  if toml
    contruct inputs/equil from toml
    main_from_inputs()
  else if h5
    construct inputs/equil from h5 (this branch)
    main_from_inputs()
  else if dd
    construct inputs/equil from dd (@calin1989's branch)
    main_from_inputs()
and so on

[jhalpern30]

@logan-nc since Calin's code is merged in now, I can take a look at this whenever the merge conflicts are resolved

[logan-nc]

@jhalpern30 I am out on vacation today and the claude max expires tomorrow. I don't think the merge conflict is serious... can you resolve it or have claude do so for you? don't wait on me to review this

[logan-nc]

note, this capability definitely needs a good regression test so it doesn't get messed up whenever someone adds a new toml var or other io change (example: @d-burg will add profiles needed for slayer soon)

[jhalpern30]

@jhalpern30 I am out on vacation today and the claude max expires tomorrow. I don't think the merge conflict is serious... can you resolve it or have claude do so for you? don't wait on me to review this

I am not currently logged into your claude but I can take a look

[logan-nc]

ty. I expect the merge conflict is trivial

[logan-nc]

@jhalpern30 — merge conflicts with develop are resolved and this is ready for your review.

Unified dispatch (your suggestion). main() now routes every input source through a builder that returns a ready (inputs, eq_config, additional_input), then calls one shared main_from_inputs:

• build_inputs_from_toml(path; dd) — gpec.toml + IMAS dd
• build_inputs_from_h5(args) — gpec.h5 snapshot

This also fixed a latent IMAS bug from the #202 merge where main(; dd) accepted dd but never threaded it into the pipeline body (now covered by a regression test).

Bot review issues — all addressed: equil.toml is more fully/clearly deprecated; parse_override_flag warns instead of silently storing an unparseable value as a string; the file-level docstring is now # comments; direct/EFIT recovery and --override-file are now tested.

Rerun drift coverage — kept lightweight. Instead of heavy regression-harness cases (two full pipeline runs each), the guard lives in the unit suite: one source + one replay with an all-datasets source-vs-replay diff (fires if any new TOML var or IO field isn't snapshotted — e.g. future slayer profiles), plus a pipeline-free direct-EFIT recovery check through the real h5 reader. Net rerun unit-test time ~1 min.

Verified: full unit suite green; rerun tests pass; solovev_n1 regression unchanged vs develop (21/21, 0 diff).

[jhalpern30]

I apologize for the long PR review here, but this touches a lot of the code and I wanted to avoid the extra cruft Claude seems persistent to add. Most of the changes are cosmetic, specifically overly verbose comments, with the exception of a few that are more widereaching such as backwards compatibility and how to carry around the raw_data.

I didn't look too in depth at the forcing terms section since I'm not as familiar with it.

[jhalpern30]

Add this to docstring. Probably no need to duplicate docstring in the comments for this struct

[jhalpern30]

I'm not entirely sure what this is doing / why it needs to be here. I guess this is taking the place of using a @kwdef wrapper on the struct and having raw_data default to none if not specified?

[jhalpern30]

Description needs to be in the docstring again

[jhalpern30]

docstring. Maybe update your claude.md to note this

[jhalpern30]

This seems like this issue could be easily avoided by just moving the external using statements to the top of the file above the include statements, and then including include("Rerun.jl") with the rest of the the includes. And then this 4 line comment is unnecessary

[jhalpern30]

You already explicitly check for analytic before entering this function in build_inputs_from_h5

[jhalpern30]

oh my logic... I had to read this 3 times to get it, I've never seen logic coded like this before. If you think this is best that's fine, but I think this whole section would be much more readable as

if raw_eq["kind"] == "analytic"
  if eq_config.eq_type == "lar"
        lar_data = get(inputs, "LAR_INPUT", Dict{String,Any}())
        additional_input = Equilibrium.LargeAspectRatioConfig(; (Symbol(k) => v for (k, v) in lar_data)...)
    else
        sol_data = get(inputs, "SOL_INPUT", Dict{String,Any}())
        additional_input = Equilibrium.SolovevConfig(; (Symbol(k) => v for (k, v) in sol_data)...)
    end
 end
elseif raw_eq["kind"] == "direct"
     additional_input = Equilibrium.build_direct_from_raw(eq_config, raw)
elseif raw_eq["kind"] == "inverse"
    additional_input = Equilibrium.build_inverse_from_raw(eq_config, raw)
else
   error("Unknown raw equilibrium kind in gpec.h5: $kind")

I think the repeated additional_input is worth the clarity here. And removing the rebuild_equilibrium_input function

[jhalpern30]

A long term goal might be to just merge this into gpec.toml as a section that only appears if you specify that type of EQ? Seems like this extra toml carrying around is unnecessary

[jhalpern30]

A general comment on this file - I think its existence is necessary due to the large amount of CLI/override capabilities added, but I think pretty much all of the equilibrium functions can be deleted and in-lined in their respective locations. This will cut the file down a bit

[jhalpern30]

A suggestion here that you can take or ignore based on how difficult you'd think it'd be to implement - so this PR adds a raw_data struct member to the Input and PlasmaEquilibrium struct, but all this raw_data really contains is the struct data itself in dictionary form? Why don't we just return the input struct itself from the equilibrium setup (or even just make it a part of PlasmaEquilibrium, this would actually be quite easy probably) and then dump that data to the HDF5. I guess you'd still need to load and dump things from an HDF5 so its not perfectly clean, but removes most of the additions to the Equilibrium code

[logan-nc]

Thanks for the thorough review — worked through all of it. Summary of how each thread was resolved:

Architecture

• raw_data dict → typed DirectIngest/InverseIngest (ReadEquilibrium.jl:1, EquilibriumTypes.jl:404). Your instinct to "just return/dump the struct" was right; the blocker is that the input structs hold built spline interpolants, which aren't HDF5-serializable, and the raw node arrays weren't stored anywhere else. So the ingest is now a typed struct holding exactly the serializable node arrays, carried as a field on DirectRunInput/InverseRunInput/PlasmaEquilibrium and dumped by iterating fieldnames. A bad key is now a field error, not a silent dict miss. This also let the field-by-field write inline into write_outputs_to_HDF5 (GeneralizedPE:614) and removed write/read_equilibrium_raw_inputs.
• No backwards-compat / all runs rerunnable (Rerun.jl:56, 78). IMAS and TJ-analytic now capture/regenerate too, so I dropped the isempty guards and the empty-default constructors. The only remaining hard error is for a genuinely pre-snapshot gpec.h5 (no input/gpec_toml_raw), which is a real format check.
• Inlined/clarified the replay dispatch (Rerun.jl:243, 339, 1). Removed rebuild_equilibrium_input and its analytic => nothing arm. The analytic-vs-direct-vs-inverse branch is now one readable block, and analytic config construction is a shared build_analytic_config used by both the TOML and rerun builders (no more lar/else-Solovev misrouting of other analytic kinds).
• Side-car analytic TOML → embedded section (GeneralizedPE:123). [SOL_INPUT]/[LAR_INPUT] now live in gpec.toml like [TJ_ANALYTIC_INPUT]; the sol.toml files are deleted and merge_auxiliary_eq_toml! is just a deprecation fallback.

Comments / structure

• Removed the diff-narration / "dialogue-with-existing-comments" comments (GeneralizedPE:162, 188, 192, 607, the main_from_inputs block) and the stranded TJ-analytic comment.
• Moved struct-field descriptions into the docstrings, documented the ingest contract, dropped the rot-prone positional-arg counts (EquilibriumTypes.jl:399/404/434/831).
• Moved external using to the top and include("Rerun.jl") in with the other includes, deleting the 4-line justification (GeneralizedPE:64).

Banner (GeneralizedPE:86) — good call, and it's already what the rerun path does: it prints its own banner rather than the normal one, showing source git: <info/git_version from the HDF5> → current: <git describe of the running checkout> plus source/output paths. So you get exactly the "version the inputs were created on vs. version producing the new outputs" comparison you described. The normal banner still prints on normal runs.

Verification

• Full test suite green, including a new CHEASE inverse-ingest recovery test alongside the existing direct + analytic round-trips.
• Regression harness bit-for-bit identical vs develop: solovev_n1 21 unchanged, diiid_n1 30 unchanged (every diff 0.0e+00) — confirms the refactor + TOML migration are numerically inert.

[logan-nc]

Follow-up (0a7af46): consolidated the analytic-equilibrium dispatch into a single registry.

Review flagged that analytic kinds were dispatched across three parallel sites that had to stay in lock-step — the ANALYTIC_EQ_SECTIONS dict, the build_analytic_config if/elseif (whose else silently defaulted to TJAnalyticConfig), and the setup_equilibrium elseif chain. With more feature branches adding [SECTION] blocks soon, that was the main extensibility hazard.

Changes:

• New Equilibrium.ANALYTIC_EQ registry of AnalyticEqSpec(section, config_type, run_fn) as the single source of truth. Both the fresh-run and rerun paths dispatch off it, so adding a new analytic kind is now one row. The per-entry run_fn cleanly handles the tj_analytic / tj_analytic_direct split (shared config, different solver). Removed the unsafe else-defaults-to-TJ branch.
• Extracted EFIT_KINDS for the twice-duplicated efit-family list; documented the ingest positional round-trip contract; sharpened two rerun error messages to point at the registry.
• Added a guard test asserting every registry entry is well-formed (Dict/String ctors, run_fn method, non-empty section) and that build_analytic_config resolves it — so future drift fails at test time, not at a user's rerun.

Note: splines are not serialized — only the DirectIngest/InverseIngest node arrays are stored and the splines are rebuilt via cubic_interp on replay (the splines/ HDF5 group is diagnostic output only).

Type stability: setup_equilibrium still infers a concrete PlasmaEquilibrium and the numerical kernels (sol_run/lar_run/…) stay concretely typed; dynamic dispatch is confined to the once-per-run config boundary behind a function barrier.

Verification:

• Rerun suite green (analytic Solovev e2e 8/8, coil snapshot 8/8, direct 20/20, inverse 20/20, override 5/5, new registry guard 21/21); Equilibrium 261/261; TJ-analytic 16/16.
• Regression-guardian: bit-for-bit identical vs pre-refactor HEAD across all tracked quantities — solovev_n1 (21 unchanged) and diiid_n1 (46 unchanged).

[jhalpern30]

I cleaned up some minor things myself, but I think two things still need attention regarding backwards compatibility (sorry for the second round).

1. Did you mean to add a section for deprecated TOML parameters? This seems unnecessary at this time
2. Did you mean to still support an auxiliary equilibrium file? This seems like another one we can just get rid of

[jhalpern30]

This seems out of place in this PR? Just want to make sure you meant to put this here

[logan-nc]

The custom coils in general? Yes, it was a sub-branch that came back into here. Why? The main objective was to be able to re-run coils from an h5 without having to carry around the matching .dat files, which is a necessary condition for this PR to be counted as complete. The custom coils made from toml params was a little missing creep but was a straightforward port of existing OMFIT python code and the only new part was the toml IO, which was tied up in this PR's architecture. So there it is.

If I had more bandwidth, I think you are right that the custom coils should be a separate PR branching off this but landing to develop after this merges. I wish. Next time 👍

[jhalpern30]

Is this something we actually want in here? I could see this list getting very long... I thought our approach was just to ignore backwards compatibility until after a first julia release

[logan-nc]

I vote we include it hear as the architecture is needed for v2.0.0 and having something there is needed to test whether it works. In the final cleanup of v2.0.0 we can clobber the list to stat fresh if it feels unwieldy. I could be convinced otherwise though... thoughts @jhalpern30 and @matt-pharr?

[logan-nc]

@jhalpern30 to address the top level comments:

1. Yes, I intended the section of deprecated toml parameters because it seemed like something we need for @matt-pharr's v2.0.0 milestone. Major versions are supposed to maintain backwards compatibility (https://semver.org/) so future 2.1.0 2.2.0 etc. should technically be able to re-run an old case run with 2.0.0 but @matt-pharr assures me that it is acceptable to count a deprecation message and ignored inputs as "compatible".
2. I guess I was just cruising on the logic of (1) but... yes, we are still in the un-versioned wild west and are free to not maintain backwards compatibility now if we don't want to. I suppose I was trying not to break things for @d-burg too much but if he is ok with a minor effort to update his TJ workflows to this then yeah, let's make it a hard policy change.

[jhalpern30]

@jhalpern30 to address the top level comments:

1. Yes, I intended the section of deprecated toml parameters because it seemed like something we need for @matt-pharr's v2.0.0 milestone. Major versions are supposed to maintain backwards compatibility (https://semver.org/) so future 2.1.0 2.2.0 etc. should technically be able to re-run an old case run with 2.0.0 but @matt-pharr assures me that it is acceptable to count a deprecation message and ignored inputs as "compatible".
2. I guess I was just cruising on the logic of (1) but... yes, we are still in the un-versioned wild west and are free to not maintain backwards compatibility now if we don't want to. I suppose I was trying not to break things for @d-burg too much but if he is ok with a minor effort to update his TJ workflows to this then yeah, let's make it a hard policy change.

I see. Isn't the point of that article that major versions can break backwards compatibility though, and minor/patch versions must maintain it?

I agree backwards compatibility is necessary, but thought is needed to what it means for our case. i.e. in this PR, backwards compatibility is introduced for the banded matrix parameters that I removed in #286 - I would classify this more as "legacy code that was converted last year but no one is/ever really used", so its not like we're preserving someone's workflow, we're just preserving their ability to run with their existing input file without removing a line they probably weren't looking at anyway. And anyone currently working with the Julia code has pretty much signed up for this level of variation in the day to day code, so I don't think we should support this. I would vote to remove it from this PR and make it a new issue/PR for how to intelligently deprecate things such that we can create something that is sustainable in the long term that can deal with more complicated backwards compatible fixes outside of just "ignore this input".

[logan-nc]

I see. Isn't the point of that article that major versions can break backwards compatibility though, and minor/patch versions must maintain it?

Exactly. 2.0.0 will break compatibility with fortran v1.5 but will need to maintain compatibility with itself moving forward (i..e 2.0.1, 2.1.0 2.2.0, etc.). Thus, it will very quickly need this deprecation guard. I want it in place so no one goes rogue and deprecates something with no guard. It being there makes it easy to note "you didn't guard your deprecation" in the PR and easy for them to do so instead of replying "what do you mean? how should I?".

If you want to move it to another PR and/or champion some other guard technique, I support you doing that. If you don't want to see mband ever used as an example, then also ok. It's weird to add the capability without at least one example though - how about the mer_flag that was changed to local_stability_flag in #234?

[logan-nc]

ok @jhalpern30 I removed the side-car TOML file support for analytic equilibria (your item 2). Item 1 is in your court.

[jhalpern30]

Exactly. 2.0.0 will break compatibility with fortran v1.5 but will need to maintain compatibility with itself moving forward (i..e 2.0.1, 2.1.0 2.2.0, etc.). Thus, it will very quickly need this deprecation guard. I want it in place so no one goes rogue and deprecates something with no guard. It being there makes it easy to note "you didn't guard your deprecation" in the PR and easy for them to do so instead of replying "what do you mean? how should I?".

If you want to move it to another PR and/or champion some other guard technique, I support you doing that. If you don't want to see mband ever used as an example, then also ok. It's weird to add the capability without at least one example though - how about the mer_flag that was changed to local_stability_flag in #234?

Got it. I support mer_flag because I think that's a good example of an input where the name was changed but the functionality is still there and someone might try to use it. I will 1) modify this PR to use that for deprecation instead of the banded inputs sometime today that way we have something in the repo already and 2) make an issue/think about the best way to do deprecation moving forward.

[jhalpern30]

Ok, deprecation is added. The merge conflicts are due to what looks like the half-area weighting, which I don't know much about so I am not going to touch that to avoid breaking it. Once they are resolved, I think this is ready to merge