Skip to content

DOCS/CONFIG - REFACTOR - Rename TOML config variables to descriptive spellings (closes #287) - #382

Draft
priyanshlunia wants to merge 5 commits into
developfrom
docs/toml_variable_cleanup
Draft

DOCS/CONFIG - REFACTOR - Rename TOML config variables to descriptive spellings (closes #287)#382
priyanshlunia wants to merge 5 commits into
developfrom
docs/toml_variable_cleanup

Conversation

@priyanshlunia

@priyanshlunia priyanshlunia commented Aug 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

DRAFT — do not merge. Stacked on #363/#364 (refactor/hdf5-metadata): this branch
merged them in to avoid deck/docs conflicts, so the diff vs develop shows their commits
until they land. Keep as draft until #363 and #364 merge into develop, after which
this diff collapses to the pure TOML cleanup (7825f5d7 + cab894a0).

Summary

Closes #287: initials-style and misnomer TOML input keys are renamed to descriptive
spellings across every config section. All old spellings keep working with a deprecation
warning (warn-and-remap) and produce identical control structs; removal is scheduled
for after v2.0.0. The headline change replaces the confusing use_parallel/use_riccati
boolean pair with a single integrator enum — "parallel" was a misnomer: it selects the
STRIDE FM/BVP algorithm (the only path producing SingularSurfaces/delta_prime_matrix),
and parallel_threads = 1 runs it serially.

Key renames (warn-and-remap until after v2.0.0)

Section Old New Notes
[ForceFreeStates] use_parallel + use_riccati integrator = "stride" | "riccati" | "serial" Default "stride". Remap mirrors the old dispatch order (parallel wins over riccati, both false → serial, old default use_parallel=true); an explicit integrator wins over old flags
[ForceFreeStates] parallel_threads integrator_threads
[ForceFreeStates] psiedge dW_edge_scan_start It is a diagnostic scan start, not the plasma edge
[ForceFreeStates] nstep, diagnose_ca (deleted) Never implemented; now dropped-with-warning like the other deprecated keys
[Equilibrium] newq0 (Int) q0_override (Float64) Int→Float64 also fixes a latent InexactError in the newq0 = -1 sign-flip path
[Equilibrium] use_galgrid use_galerkin_grid
[PerturbedEquilibrium] reg_spot regularization_width
[SLAYER] dc_type delta_crit_type
[SLAYER] dr_val delta_crit_D_R
[SLAYER] dgeo_val delta_crit_geo_factor
[[ForcingTerms.coil_set]] xnom / ynom / znom rotation_center_x / _y / _z The coil parser previously dropped unknown keys silently, so the remap is load-bearing here

Value renames (warn-alias for old spellings)

Key Old New
[Equilibrium] grid_type "ldp" "rational_packed"
[KineticForces] f0type "jkp" "park"
[SLAYER] delta_crit_type "rfitzp" "fitzpatrick"

Canonical surnames/acronyms are kept (pest/boozer/hamada/park, ggj_*, cgl,
krook, sauter/redl/spitzer/wesson, nrl), as are the Fortran-v1-heritage names
(mpsi, psilow/psihigh, qlow/qhigh, dmlim, ucrit, singfac_min, wall
a/aw/bw/…, Solovev and KF parameter names) per the conservative-sweep decision.

Mechanics

  • New _rename_keys! / _rename_value! / _remap_integrator_keys! helpers next to
    _drop_deprecated_keys! in src/GeneralizedPerturbedEquilibrium.jl, wired into the
    Equilibrium/FFS/PE/KF loaders, Rerun.jl, slayer_control_from_toml, and
    _parse_coil_set_config.
  • Rerun path: post-ALL - REFACTOR - Module-mirroring CamelCase HDF5 schema (issue #226, PR A) #363, Input/gpec_toml_raw is the only input echo, so h5
    snapshots written by older versions rerun through the same remap (with deprecation
    warnings) forever.
  • Config-time guard: [SLAYER] enabled with integrator != "stride" now errors at
    load time instead of silently falling back to the per-surface stub Δ' after a long run.
  • integrator is validated against ("stride", "riccati", "serial") at load.

⚠️ Scope decisions — reviewer sign-off requested

  • HDF5 dataset leaf names are NOT renamed. Tearing/PerSurface/{dr_val,dgeo_val,dc_type}
    keep their old spellings for schema stability (only the Julia struct fields / TOML keys
    moved). Renaming output datasets is a breaking schema change — flagged as a future
    schema-bump candidate, same precedent as ALL - IMPROVEMENT - Self-describing HDF5 metadata (issue #226, PR B) #364's omega_Hz note.
  • One value-level output change: the PerSurface/dc_type string content now reads
    "fitzpatrick" instead of "rfitzp" for decks using that option (label only, no numerics).
  • Deferred to the hackathon (out of scope, per plan): ksing, [Wall]
    shape-doubles-as-filepath, LAR/TJ qc-vs-q0 and ma inconsistency,
    ncoil_gen/nsec_gen vs n_coils, KF method-flag naming.

Sweep

All example decks, test fixtures, tests, and benchmarks updated to the new spellings.
Docs updated (stability.md — which also had its incorrect use_parallel = false
default claim fixed — equilibrium.md, developer_notes.md, citations.md,
hdf5-conventions.md cross-references the reserved RiccatiIntegration/ group to
integrator = "riccati"). The pre-commit toml-no-deprecated-keys pattern is extended
with all old key names, so they cannot re-enter the decks.

Back-compat testing

New test/runtests_toml_backcompat.jl (42 assertions): every renamed key/value loads
from the old spelling with a warning and produces a field-identical control struct
vs the new spelling, including all four use_parallel/use_riccati combinations against
the old dispatch order, precedence of explicitly-set new keys, and the real
build_inputs_from_toml loader path on a temp deck.

Verification

Merging into develop requires third-party human review — non-negotiable.

🤖 Generated with Claude Code

logan-nc and others added 5 commits August 12, 2026 20:38
@logan-nc @claude
ALL - IMPROVEMENT - Self-describing HDF5 metadata (long_name, units, …
951bf4e 
…dims, scales)

Every gpec.h5 dataset outside Input/ and the debug-only GalerkinIntegration/Match/
now answers "what is this, in what units, plotted against what" without opening
the source, readable natively by h5py/xarray/HDFView:

- New Utilities.HDF5Annotations: annotate!(parent, table) applies per-writer
  path => (; long_name, units, dims) tables post-write (skipping absent paths);
  make_scale!/attach_scale! wrap the H5DS dimension-scale API (netCDF-4
  coordinate mechanism) with Julia-axis -> C-dim index translation;
  write_root_attrs! stamps schema_version=2.0, Conventions=GPEC-HDF5-2.0,
  references, title, date_created.
- Table-driven, not per-write-call: writers are untouched except one annotate
  call at the end of each (src/HDF5Schema.jl for the main writer; tables live
  next to write_galerkin!, the PE writer, KineticForces/Output.jl, and
  Tearing/Runner/HDF5Output.jl). ~340 dataset annotations total.
- Coordinate datasets (psi grids, rational-surface psi, geometry xs/ys) are
  marked as HDF5 Dimension Scales and attached to the profiles sharing the axis;
  a greppable "dims" attribute mirrors the scales in Julia axis order.
- Attribute wording/units audited by the fortran-physics-reviewer against the
  layer/field-reconstruction sources; 27 corrections applied (J-weighted field
  units T*m^2, Q-normalization time vs resistive-kink time, island half-width
  vs full width, omega_Hz actually rad/s, etc.).
- runtests_h5_schema.jl now enforces the contract: long_name+units on every
  non-exempt dataset, dims on rank >= 2 arrays, root attrs present, scales
  attached; docs/development/hdf5-conventions.md gains the metadata contract.

Attributes are invisible to the rerun leaf-walk and the regression extractor:
no tracked value moves.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0129rSTCmYJDBbcH9khHqYnz
@logan-nc @claude
ALL - CLEANUP - Wrap annotation-table lines to the 180-char margin
0ece9c4 
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0129rSTCmYJDBbcH9khHqYnz
@priyanshlunia
Merge remote-tracking branch 'origin/refactor/hdf5-metadata' into doc…
0603103 
…s/toml_variable_cleanup

# Conflicts:
#	benchmarks/gal_validation/gal_beta_scan.jl
#	benchmarks/gal_validation/gal_epsilon_scan.jl
#	benchmarks/gal_validation/pfac_study/gal_pfac_scan2d.jl
#	src/ForceFreeStates/EulerLagrange.jl
#	src/GeneralizedPerturbedEquilibrium.jl
@priyanshlunia @claude
ALL - REFACTOR - Rename TOML config variables to descriptive spelling…
7825f5d 
…s (issue #287)

Rename initials-style and misnomer TOML input keys across all config sections,
with warn-and-remap back-compat: old spellings load with a deprecation warning
and produce identical control structs until removal after v2.0.0. The
use_parallel/use_riccati boolean pair becomes a single `integrator` enum whose
remap mirrors the old dispatch order (parallel wins over riccati, both false
means serial, old default use_parallel=true).

Key renames:

| Section              | Old                        | New                                        |
|----------------------|----------------------------|--------------------------------------------|
| [ForceFreeStates]    | use_parallel + use_riccati | integrator = "stride"|"riccati"|"serial"   |
| [ForceFreeStates]    | parallel_threads           | integrator_threads                         |
| [ForceFreeStates]    | psiedge                    | dW_edge_scan_start                         |
| [ForceFreeStates]    | nstep, diagnose_ca         | (deleted — never implemented)              |
| [Equilibrium]        | newq0 (Int)                | q0_override (Float64)                      |
| [Equilibrium]        | use_galgrid                | use_galerkin_grid                          |
| [PerturbedEquilibrium] | reg_spot                 | regularization_width                       |
| [SLAYER]             | dc_type                    | delta_crit_type                            |
| [SLAYER]             | dr_val                     | delta_crit_D_R                             |
| [SLAYER]             | dgeo_val                   | delta_crit_geo_factor                      |

Value renames (warn-alias for old spellings):

| Key                  | Old        | New               |
|----------------------|------------|-------------------|
| [Equilibrium] grid_type | "ldp"   | "rational_packed" |
| [KineticForces] f0type  | "jkp"   | "park"            |
| [SLAYER] delta_crit_type | "rfitzp" | "fitzpatrick"  |

Mechanics: new _rename_keys!/_rename_value!/_remap_integrator_keys! helpers next
to _drop_deprecated_keys!, wired into the Equilibrium/FFS/PE/KF loaders,
Rerun.jl (the Input/gpec_toml_raw echo is the only input record post h5
refactor, so old h5 snapshots rerun through the same remap), and
slayer_control_from_toml. SLAYER with integrator != "stride" now errors at
config time instead of falling back to the stub Δ' at runtime. HDF5 dataset
leaf names (Tearing PerSurface/dr_val, dgeo_val, dc_type) deliberately keep the
old spellings for schema stability; renaming them is a future schema-bump
candidate.

Sweep: all example decks, test fixtures, tests, benchmarks, and docs updated to
the new spellings (stability.md also fixes its wrong use_parallel=false default
claim); pre-commit toml-no-deprecated-keys pattern extended with the old keys;
new test/runtests_toml_backcompat.jl covers the remap machinery and old-vs-new
ctrl struct equality.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@priyanshlunia @claude
FORCINGTERMS - REFACTOR - Rename coil_set xnom/ynom/znom to rotation_…
cab894a 
…center_x/_y/_z (issue #287)

Completes the issue #287 rename sweep: the [[ForcingTerms.coil_set]] explicit
rotation-center keys move to descriptive spellings, with the same
warn-and-remap back-compat as the other renames (the coil parser previously
dropped unknown keys silently, so the remap is required for old decks to keep
working). Old keys added to the pre-commit toml-no-deprecated-keys pattern and
covered in runtests_toml_backcompat.jl.

| Section                    | Old  | New               |
|----------------------------|------|-------------------|
| [[ForcingTerms.coil_set]]  | xnom | rotation_center_x |
| [[ForcingTerms.coil_set]]  | ynom | rotation_center_y |
| [[ForcingTerms.coil_set]]  | znom | rotation_center_z |

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@priyanshlunia priyanshlunia added architecture refactor Behavior-preserving restructuring labels Aug 14, 2026
logan-nc added a commit that referenced this pull request Aug 17, 2026
@logan-nc @claude
EQUIL/EXAMPLES - Default psihigh_from_layer_overlap on across every deck
3cea719 
Flips EquilibriumConfig.psihigh_from_layer_overlap to true and sets it
explicitly, annotated, in all 17 decks. Kinetic profiles are wired via a
[SLAYER] profile_file entry using each deck's OWN family file -- never a
[KineticForces] block, which would flip needs_kinetic_profiles and change the
auto-grid knot density, an unrelated results channel. A [SLAYER] section
carrying only profile_file is inert: slayer_ctrl.enabled defaults false and the
stage returns before running or writing anything.

Deck sweep, equilibrium stage, all 17:
  MOVED  DIIID-like_SLAYER          0.9995 -> 0.993281
  MOVED  DIIID-like_ideal           0.995  -> 0.993281
  no-op  DIIID gal_resistive, _pe   0.993 (already at the floor)
  no-op  Solovev x5, LAR x4, a10
  n/a    DIIID-like_ideal_IMAS      needs a runtime dd; flag left off anyway

The analytic decks no-op exactly as predicted: limited plasmas with finite edge
q, so the edge q-law fit is rejected and the two well-separated surfaces never
overlap. LAR has no legitimate kinetic file, so those four no-op on the missing
profiles instead.

NOTE FOR REVIEW: on all four DIII-D decks the psiedge floor OVERRIDES the layer
criterion. Each asks for ~0.9892 and each is held at the q=6 window top. The
feature as configured therefore reduces to "clamp psihigh to the first rational
surface outboard of psiedge"; the layer physics sets the domain nowhere in the
shipped examples.

IMAS deck keeps the flag off with a comment: its rerun path re-runs read_imas and
must resolve the same psihigh on both passes (_validate_psi_nodes errors
otherwise), which is what resolving a new domain changes; "imas" is also outside
EFIT_KINDS so there is no separatrix clamp and no raw psi map.

benchmarks/equil_psihigh_scan.jl now records the structured equilibrium_edge_quality
verdict per point instead of a hand-rolled round-trip, captures et/ep/ev, and adds
a gate-validation pass that fails loudly if any psihigh exists where the gate
passes while the energies are already unusable. It forces the flag off so the
ladder tests the psihigh it sets.

docs: corrects a naming bug I introduced -- workflow.md called the variable
dW_edge_scan_start, but that rename is an unmerged draft (PR #382) and the code
says psiedge. Documents the floor, the cross-section dependency on
[ForceFreeStates] psiedge, and that truncations are reported in mm as well as psi.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PSrf6JCViFfVzqzkQ66o6b
@logan-nc logan-nc mentioned this pull request Aug 19, 2026
Merged
@logan-nc

logan-nc commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

@priyanshlunia #363 and #364 were merged. Do you want to mark this as ready for review?

It now has a lot of conflicts, but I suspect they are largely trivial. Please merge in the newest develop and ping us afterwards when it is ready for review. Is the assignee yourself?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@d-burg d-burg Awaiting requested review from d-burg
@logan-nc logan-nc Awaiting requested review from logan-nc

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

refactor Behavior-preserving restructuring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

TOML variable cleanup

2 participants

@priyanshlunia @logan-nc