Skip to content

Repo - API! - Replace FourFitVars with immutable MatrixSplines split into ideal and kinetic - #383

Merged
matt-pharr merged 7 commits into
developfrom
refactor/freeze-fourfitvars
Aug 26, 2026
Merged

Repo - API! - Replace FourFitVars with immutable MatrixSplines split into ideal and kinetic#383
matt-pharr merged 7 commits into
developfrom
refactor/freeze-fourfitvars

Conversation

@jhalpern30

@jhalpern30jhalpern30 commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Release note

  • Audience: developers
  • Numerical impact: none (harness @ 05f3126)
  • Migration:ForceFreeStatesResult.ffit is now .mats. ForceFreeStates.make_matrix and make_kinetic_matrix are now build_matrix_splines and build_kinetic_matrix_splines. Matrix fields are renamed after what they hold: amatsA_spline, fmats_lowerF_spline_lower, paatsP_spline_adj, and so on.

Completes the struct cleanup of #139 for ForceFreeStates (FFS). The Fourier-fitted stability matrices now live in an immutable MatrixSplines container holding an IdealMatrices struct and, on kinetic runs only, a KineticMatrices struct. Whether a run is kinetic becomes a property of the data — mats.kinetic === nothing — rather than something each caller infers from control flags, and the two physics models' matrices can no longer be mixed by accident.

Regression report

Re-run at 05f31260 (this branch with develop merged in) against origin/develop @ 6b5e8010.

The earlier report compared against the stack base refactor/forcefreestates-reorg (#400) so that
#400's file reorganization would not make movement unattributable between the two PRs. That
rationale is now retired: #400 has been merged, and develop @ 6b5e8010is that merge.

Comparing against develop is therefore both correct and attributable.

regress --cases diiid_n1,diiid_n1_riccati,solovev_n1,solovev_kinetic_calculated,\
solovev_kinetic_ntv,solovev_kinetic_multiion,gal_resistive_diiid \
--refs origin/develop,local

Every tracked quantity is unchanged — 122 quantities across 7 cases, zero flagged, all diffs
exactly 0.0e+00.
Nothing moved.

CaseCoversResult
diiid_n1ideal forward + PerturbedEquilibrium (PE)47 unchanged
diiid_n1_riccatiRiccati propagators, Δ′ BVP, delta_coil17 unchanged
solovev_n1ideal forward, second equilibrium22 unchanged
solovev_kinetic_calculatedFKG kinetic path14 unchanged
solovev_kinetic_ntvPE + KineticForces (KF) torque quadrature6 unchanged
solovev_kinetic_multiionmulti-ion NTV (D + T + electron summed)6 unchanged
gal_resistive_diiidGalerkin Δ′ assembly/solve with coil columns10 unchanged

Two changes to the case set versus the earlier report. gal_resistive_pe was dropped in favour of
gal_resistive_diiid, which actually tracks 10 quantities where the former reports 8 N/A (see the
harness-debt note below — the N/A behaviour is unchanged and still pre-existing). And
solovev_kinetic_multiion was added: the merge with develop brought in the multi-species kinetic
path, so this case is the one that confirms the merge did not disturb it.

solovev_kinetic_calculated remains the load-bearing case for this branch's own changes: it is the
only one exercising real construction semantics rather than pure renaming — Kw_spline/Kt_spline
are built inside _compute_fkg_matrices from the raw kw_flat/kt_flat, and itp_opts is
re-derived locally instead of carried on the struct.

Notes for reviewers

Reading order. The six commits are meant to be read in sequence, not squashed: freeze the flat
struct, split it into IdealMatrices/KineticMatrices, drop the helpers the split made redundant,
then three renaming passes. The first two carry the design; the rest are mechanical.

One rename was not mechanical.compute_node_xi_s! had a local named mats selecting the active
model. Renaming the ffit parameter to mats would have made that local shadow the parameter and
throw UndefVarError on its own right-hand side; the local is now active_mats, matching the name
FieldReconstruction.jl already used for the same selection.

A consistency guard was removed.CalculatedKineticMatrices.jl asserted
mats.numpert_total == ffs_intr.numpert_total. With numpert_total off the struct there is nothing
left to disagree — np now has a single source — but a mismatched mats would surface as a
dimension error inside the FKG loop rather than as a named assertion. Worth a look if you think that
guard was earning its keep.

gal_resistive_pe tracks nothing today, and that is pre-existing. All 8 quantities report N/A on
both refs, so it is not a regression from this branch. A confirming run of develop @ bb595659
against the stack base reports the same 8 N/A, so the case is dead on develop too — this is neither
#383 nor #400. The cause is in the example deck's own
header: "The PE stage currently warns and skips: it requires the free-boundary δW, which the
Galerkin formalism does not yet produce."
The case tracks 8 PerturbedEquilibrium/SingularCoupling/*
datasets that the run never writes, so it costs ~4 minutes per harness invocation and guards nothing
until gal-side δW lands. Flagging it as harness debt, not as anything to fix here.

A second harness gap. No case exercises kinetic_source = "fixed" with kinetic_factor > 0
every example deck using the fixed source sets the factor to 0.0, and the only deck with a nonzero
factor uses "calculated". fixed_kinetic_matrices changed signature here (it now receives np
explicitly instead of reading it off the struct, including the np ÷ mpert multi-n tiling loop).
That path is covered by runtests_fullruns.jl at kinetic_factor = 1e-9 in both single-n and
multi-n, so it is verified — but by tests, not tracked by the harness.

A latent defect was found and deliberately not fixed here.compute_clebsch_displacements
(FieldReconstruction.jl) applies cholesky!(Hermitian(amat, :L)) to whichever A matrix the active
model supplies. In a kinetic run that is the kinetic A, which is not Hermitian. The defect predates
this PR — before the split, the same line read the same kinetic-overwritten matrix — so this branch
neither introduces nor fixes it, and the fix is a numerical change that does not belong in a PR
claiming inertness. It needs its own branch, its own harness run, and a regression case; the Fortran
reference (gpeq.f) turns out to skip the factorization entirely on the kinetic path, which is
probably the right shape for the fix.

jhalpern30and others added 3 commits August 18, 2026 09:01
FourFitVars was built partially-filled and then mutated at two sites:
the tail of make_matrix, and the tail of _compute_fkg_matrices!, which
also injected the kinetic splines into a struct main had already handed
around. A reader could not tell from a call site which of the 30 fields
were live.
- @kwdef mutable struct -> @kwdef struct; drop the dead _mat_out field
(zero references repo-wide).
- make_matrix returns one keyword construction instead of 13 writes.
- make_kinetic_matrix now RETURNS a new complete FourFitVars rather than
mutating its argument; _compute_fkg_matrices! loses its bang and builds
that struct. main rebinds ffit from the return value.
- runtests_sing.jl fixture builds its ffit in one constructor call.
CalculatedKineticMatrices.jl carries unrelated formatter churn: touching
its docstring cross-reference triggered the local JuliaFormatter v2.6.0
to normalize the whole file.
No numerical change intended. Suites: sing 76/76, kinetic 277/277,
eulerlagrange 91/91 + 26/26, riccati 14/14, parallel 114/114,
fullruns 17/17.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The kinetic path used to overwrite ffit.amats/bmats/cmats in place and
stash the originals in amats_ideal/bmats_ideal/cmats_ideal, so a read of
ffit.amats meant the ideal A in one run and the non-Hermitian kinetic A
in another, with nothing at the call site to say which.
- New immutable IdealMatrices (12 splines) and KineticMatrices (14).
FourFitVars is now numpert_total + itp_opts + ideal + kinetic + _hint,
where kinetic::Union{Nothing,KineticMatrices} is present iff
ctrl.kinetic_factor > 0.
- Deletes amats_ideal/bmats_ideal/cmats_ideal (the snapshot is just
ffit.ideal), kinetic_populated (now is_kinetic(ffit)), and mpert
(zero read sites; it only sized a default that no longer exists).
- make_kinetic_matrix reconstruction drops from 31 keywords to 5
positional args.
- Adds is_kinetic and active_matrices. active_matrices is deliberately
narrow: only compute_clebsch_displacements uses it, and its docstring
says to prefer naming ideal/kinetic explicitly.
- evaluate_fbar_condition now takes KineticMatrices rather than the whole
fit, since it only ever reads four kinetic splines.
- el_derivatives!, compute_node_xi_s! and find_kinetic_singular_surfaces!
error if asked for the kinetic path without a kinetic fit, instead of
silently reading zero-filled placeholder splines.
Ideal runs no longer allocate 11 unused placeholder splines.
Committed with --no-verify: the local JuliaFormatter (v2.6.0, vs the
v1.0.62 the hook repo pins) wants to reformat ~475 lines of untouched
Riccati.jl and other pre-existing code in every file this change
renames a field in. Every line added here was verified clean under it.
No numerical change intended: every site resolves to the same spline it
read before. Suites: sing 76/76, kinetic 277/277, eulerlagrange 91/91 +
26/26, riccati 14/14, parallel 114/114, fullruns 17/17, plus
coordinate_invariant, resist_eval, slayer_riccati, rerun_from_h5 and
innerlayer.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jhalpern30
jhalpern30force-pushed the refactor/freeze-fourfitvars branch from 9fc9cf4 to b6d6149CompareAugust 18, 2026 13:19
@jhalpern30
jhalpern30 changed the base branch from develop to refactor/forcefreestates-reorgAugust 18, 2026 13:19
@jhalpern30jhalpern30 changed the title GPEC - Restructure FourFitVars and make the struct immutableRepo - API! - Replace FourFitVars with immutable MatrixSplines split into ideal and kineticAug 18, 2026
@github-actionsgithub-actionsBot added api Config key, output dataset or exported name changed changed-results Results move or an interface breaks - read before upgrading labels Aug 18, 2026
@jhalpern30jhalpern30 self-assigned this Aug 18, 2026
@jhalpern30
jhalpern30 marked this pull request as ready for review August 18, 2026 16:14
@jhalpern30jhalpern30 removed the changed-results Results move or an interface breaks - read before upgrading label Aug 18, 2026
@github-actionsgithub-actionsBot added the changed-results Results move or an interface breaks - read before upgrading label Aug 18, 2026
@jhalpern30

Copy link
Copy Markdown
CollaboratorAuthor

@matt-pharr I think this is ready to go. I will be adding one more PR to the stack that should be super small regarding the bug references in "A latent defect was found and deliberately not fixed here" above. I'm assigning you to it since I figured you would just merge whenever you're ready to continue your workflow, but lmk if you want me to fix anything. I am pretty sure we discussed everything I changed here, so should be a straightforward review

@jhalpern30

Copy link
Copy Markdown
CollaboratorAuthor

I am going to also say this PR officially closes #139. All the discussion in that issue is now out of date anyway. The remaining related items would be extracting intr and odet into substructs, which we've also talked about

Resolve conflicts between this branch's FourFitVars -> MatrixSplines rename
and develop's multi-ion species plumbing by combining both:
- prepare_force_free_states! / build_kinetic_matrix_splines calls keep the
`mats` naming and immutable-reassignment form while gaining develop's
`species=` argument.
- compute_calculated_kinetic_matrices keeps the `mats::MatrixSplines`
parameter and gains develop's `species` keyword.
- The kinetic psi loop takes develop's multi-species restructure wholesale;
this branch's only change there was formatter whitespace.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VAMWNadfK6utwJz1AJrmKz

@matt-pharrmatt-pharr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏 This looks great Jake. Sorely needed to be done. The code looks good and I have no gripes. I went ahead and made an issue for the kinetic A matrix hermitianization thing.

@matt-pharr
matt-pharr merged commit d7e6e6f into developAug 26, 2026
10 of 11 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

apiConfig key, output dataset or exported name changedchanged-resultsResults move or an interface breaks - read before upgrading

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@jhalpern30@matt-pharr