Skip to content

ForceFreeStates - FEATURE! - Cap the integration domain where resistive layers overlap - #298

Open
amlakbekaluachule wants to merge 14 commits into
bugfix/slayer-dprime-reference-lengthfrom
FKR-width
Open

ForceFreeStates - FEATURE! - Cap the integration domain where resistive layers overlap#298
amlakbekaluachule wants to merge 14 commits into
bugfix/slayer-dprime-reference-lengthfrom
FKR-width

Conversation

@amlakbekaluachule

@amlakbekaluachule amlakbekaluachule commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Release note

  • Audience: users
  • Numerical impact: none by default. The feature is opt-in (psilim_from_layer_overlap, default false) and no shipped deck enables it. The edge q-law validity gate was corrected, which can change the auto-grid edge knot density for equilibria that the old gate wrongly rejected — no shipped deck is affected, but a real reconstruction on grid_type = "auto" will see a different grid. (harness @ 6a75d0f), re-run after the develop sync merge and the review-fix commits moved src/ past the previous stamp
  • Migration: LayerWidths gains delta_norm, delta_visco and delta_dr; code constructing it positionally must supply them. sing_lim! and refined_psi_grid gain optional keywords and are otherwise unchanged.

GPEC can now locate the point past which adjacent rational surfaces' resistive layers overlap — the outer limit of the matched-asymptotic treatment, after Fitzpatrick, Nucl. Fusion 2025 Sect. 5.9 — and use it as an upper bound on the stability integration domain psilim. The overlap point is recorded whether or not it binds, so a run always shows whether layer physics constrained the domain. psihigh is untouched and remains user-set.

What is actually wired

piece where
opt-in control [ForceFreeStates] psilim_from_layer_overlap (default false)
the scan Tearing.resistive_layer_overlap, run in prepare_force_free_states! whenever kinetic profiles are readable
the cap sing_lim!(...; psilim_cap=...) — an upper bound on qlim, applied before dmlim/qhigh
the record ForceFreeStates/LayerOverlap/ — always written when the scan ran

The scan runs regardless of the flag; only applying the cap is gated. That is deliberate: a run should show where layer physics would have cut the domain even when it was allowed to. Profiles are taken from whichever of [KineticForces] kinetic_file or [SLAYER] profile_file exists on disk.

Architecture, as agreed with @logan-nc: an upper bound on psilim, applied as a cap on qlim at the top of sing_lim!, so dmlim/qhigh still select the final surface from inside it and a cap beyond psihigh is inert with no special case. Nothing re-forms the equilibrium, which removes the trial/back-off loop, EdgeQuality, and the immutable-config collision of the earlier design.

Verified end to end, both directions

On the shipped DIII-D-like SLAYER deck at its psihigh = 0.9995, the overlap point is ψ = 0.999695 — outside the requested domain — so the cap is inert, applied = 0, and nothing moves. Raising psihigh to 0.99995 makes it bind:

Resistive-layer overlap caps the domain: qlim 8.536 -> 8.000 (psi 0.999950 -> 0.999756)
Setting psilim via dmlim: initial qlim = 8.000, dmlim = 0.200
Filtered 1 singular surface(s) outside integration domain: [([8], 8.0)]

Worth knowing: because the limit lands on a rational surface's own position, dmlim's trunc then backs off to the rational below it (7.200 here). With set_psilim_via_dmlim = true — the default for diverted plasmas — the dmlim floor therefore absorbs the difference between cutting at the last clean surface and cutting half a layer width inside it. That is dmlim doing its job (it exists to avoid ending the integration on a rational), but it means the convention choice matters less in practice than the raw ψ values suggest.

The domain-limit convention

The scan cuts at ψ[k−1], the position of the last surface with a well-separated inner region. This is the paper's own 1 − ε_c, and it is what runtests_layer_overlap_fitzpatrick.jl reports in order to reproduce the paper's Ψ = 0.9985 / 0.9952.

Settled against ten equilibria (the shipped deck plus nine IDA DIII-D reconstructions) rather than by preference — the two half-width alternatives both fail:

convention result
ψ[k−1] − w/2 (the original) drops the last surface the criterion found clean; half a layer width more conservative than the paper
ψ[k−1] always inside the plasma, always inside the first contaminated surface
ψ[k−1] + w/2 exceeds ψ = 1 on 4 of 10, and on those same 4 overshoots surface k itself

Full per-case table in the thread.

Bugs found while making the feature trustworthy

1. The edge q-law gate rejected 8 of 9 real DIII-D reconstructions (d4ce058ff, carries !). EDGE_FIT_MIN_R2 = 0.99 was calibrated against the synthetic TokaMaker deck (r²_log 0.9957, 0.9989). Real EFIT/CAKE reconstructions carry noise in the edge q and fit at 0.929–0.977, so all were refused: no surfaces placed past the grid, and the scan reported "no overlap" — a silent fail-open on exactly the case the criterion exists for. No absolute threshold can separate the classes (the limited a10 deck sits at 0.972, inside the diverted band); the r²_log > r²_linear comparison does, and is unchanged. Lowered to 0.90 as a weak floor rather than removed. The ! is because the same gate controls the auto-grid edge density floor.

2. Reverse shear crashed the layer build — split out as #431, being pre-existing on develop. #431 has since merged; the cherry-picked copy resolved away in the develop sync merge (7e9e66508).

3. A single failing surface aborted the whole scan (6781b639f). The per-surface guard caught only ArgumentError.

4. The no-surfaces path threw instead of returning an empty scan (c02a3c43d, found in adversarial review). The early return built LayerOverlapScan with 13 arguments for a 15-field struct — a MethodError the pipeline's blanket catch downgraded to a spurious "scan failed" warning. Fixed, with a regression test forcing the path (m_max=0, extrapolation off), and the catch now logs the backtrace so a programming error is distinguishable from a physics refusal.

Jacobian independence

@logan-nc asked for confirmation that delta_s gives a Jacobian-independent ψ width. Measured under pest / hamada / equal_arc: r_s invariant to 1e-16, da/dψ to 3e-14, |δ_s| and both Δψ widths to ~7e-6 (the equilibrium solve's own noise — the inputs are invariant to 1e-14). Same under :flux. Confirmed clean.

The check did find that :fsa and :halfwidth are coordinate-dependent (up to 7% in r_s); those labels live in #403, are not defaults, and are not used here.

Notes for reviewers

  • The scan is driven at rs_method = :flux, not the SLAYER default. Eq. (100) is not covariant — δ ∝ (q/|dq/dr|)^(1/2) — so it is only valid in the coordinate it was derived in, the toroidal-flux label of the paper's Eq. (30).
  • delta_s_abs now appears in a third HDF5 group. The one-name-per-quantity rule says this is right, but @logan-nc asked to be told if it stayed ambiguous.
  • psilim_from_layer_overlap is not in any shipped gpec.toml. It works as a @kwdef default; adding it to the SLAYER deck is bound up with the open question below.
  • Still open, and not gated on this PR: the two width channels (|δ_s| Riccati vs Eq. 100) disagree by about two surfaces. Both are reported; the disagreement belongs to the ODE-vs-Eq.(100) reduction issue.
  • Open question for @jhalpern30 / @matt-pharr, raised by @logan-nc: should the shipped decks stop being a no-op? They are not uniform today — SLAYER and IMAS are at psihigh = 0.9995, ideal and riccati at 0.995, gal_resistive at 0.993.

Regression report

Current head: regress --cases diiid_slayer_n1,diiid_n1_riccati,solovev_n1 --refs 6d25c7739,6a75d0f50 --force (this branch's merge-base on #403's branch vs this head, so the comparison isolates exactly this PR's commits; raw shas, both sides fresh, julia 1.11.6, pinned manifest):

diiid_slayer_n1   Summary: 1 changed, 14 unchanged
                  SLAYER γ_Hz [2/1,3/1,4/1]  1.217e-01 (0.01%)  ** CHANGED **
diiid_n1_riccati  Summary: 17 unchanged
solovev_n1        Summary: 21 unchanged

The lone γ line is the threaded dispersion root search's known discrete reproducibility step (#420) — the identical 1.217e-01 value has been measured in same-commit-vs-itself forced runs — and every layer input on that case is bit-identical, so it is jitter, not this diff. An earlier run of the pre-rebase series (base 29df8700e vs c9569c0ec; solovev_n1, solovev_multi_n, diiid_n1, diiid_slayer_n1) came back 99 tracked quantities, none changed.

That is the expected result and the reason to trust the default-off claim: the feature is opt-in, no shipped deck enables it, and the corrected edge q-law gate does not alter any shipped deck's grid (the DIII-D-like deck passed both the old and new thresholds; Solovev and LAR reject on the relative log-vs-linear test either way; a10 rejects on knot count).

Coverage gap, stated rather than hidden: the edge_q_law gate change is exercised by diiid_n1 and diiid_slayer_n1 (both auto grids, both unchanged) but by no limited deck on an auto grid, because every Solovev/LAR case uses grid_type = "ldp" with fixed mpsi. The gate's limited-plasma branch is therefore argued from measured r² values rather than covered by a harness case. A Solovev auto-grid case would close this.

Validation

  • Full suite julia -t 4 --project=. test/runtests.jl on the develop-sync merge 7e9e66508: 64 testsets, 2390 assertions, 0 failures
  • Targeted file at this head 6a75d0f50 in a fresh worktree: JET reproduction 7/7, cap wiring 12/12 (includes the new empty-scan regression test)
  • External reference: runtests_layer_overlap_fitzpatrick.jl reproduces the paper's overlap boundary at Ψ = 0.9985111 (paper 0.9985) for n = 1 and 0.9949738 (paper 0.9952) for n = 4
  • Cap wiring assertions on the contract — a cap inside the domain pulls qlim/psilim in, a cap beyond psihigh is inert, nothing is a no-op, and the control defaults to off
  • End-to-end on the DIII-D-like SLAYER deck, flag off and on, plus a binding configuration
  • CI has not run the suite on this PR while the base is bugfix/slayer-dprime-reference-length; it will when the base is retargeted to develop after Tearing - BUGFIX! - Convert Δ' to the r_s reference length before slab-layer matching #403 merges

Important

Stacked on #403. Base will be retargeted to develop once that merges. Do not merge without human review.

@amlakbekaluachule
amlakbekaluachule marked this pull request as draft June 22, 2026 14:58
@amlakbekaluachule
amlakbekaluachule marked this pull request as ready for review June 22, 2026 14:58
@amlakbekaluachule amlakbekaluachule added refactor Behavior-preserving restructuring auto-merge feature New capability and removed refactor Behavior-preserving restructuring auto-merge labels Jun 22, 2026
@amlakbekaluachule

Copy link
Copy Markdown
Collaborator Author

@logan-nc this PR does two things:

  1. Fixes a bug in slayer_layer_thickness (LayerThickness.jl)
  2. Adds set_resistive_width_based_psihigh (new file,SetResistiveWidthPsihigh.jl)

Let me know if anything looks off.

@d-burg
d-burg changed the base branch from develop to feature/tearing-growthrates July 31, 2026 00:33
@d-burg

d-burg commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

I synced this branch with feature/tearing-growthrates (merge 8a771555) and retargeted the PR base from develop to feature/tearing-growthrates. The diff now shows your 2 files instead of 65.

Worth knowing why the retarget was needed: LayerThickness.jl doesn't exist on develop at all — the whole SLAYER module is tearing-branch work, and develop only carries a Slayer.jl stub. So the PR couldn't have merged there regardless of how clean it was.

The merge hit two conflicts, both caused by src/Tearing/InnerLayer/SLAYER/ being relocated to src/InnerLayer/SLAYER/ on the parent branch (the InnerLayer module had to move above ForceFreeStates to break a dependency cycle). Nothing to do with your changes — resolved keeping your δ_FKR / δ_visco constructor arguments. The package precompiles and the SLAYER tests pass on the merged branch.

Two things worth a look before review:

1. SetResistiveWidthPsihigh.jl is never loaded

SLAYER.jl includes LayerParameters.jl, Riccati.jl, LayerThickness.jl and LayerInputs.jl (lines 51–54) but not SetResistiveWidthPsihigh.jl, so the helper isn't reachable:

julia> using GeneralizedPerturbedEquilibrium
julia> isdefined(GeneralizedPerturbedEquilibrium.InnerLayer.SLAYER, :set_resistive_width_based_psihigh)
false

Adding include("SetResistiveWidthPsihigh.jl") alongside the others should wire it in, plus an export if it's meant to be public API.

2. δ_FKR / δ_visco aren't in the LayerWidths docstring

The # Fields list in LayerThickness.jl (lines 133–139) still stops at d_beta. The repo convention is to document struct fields in the docstring rather than as inline comments, so these want a line each saying what they are and their units.

Minor: I also stripped some trailing whitespace from both files while resolving the merge — the pre-commit hygiene hook rejects it.

Base automatically changed from feature/tearing-growthrates to develop August 11, 2026 22:51
@logan-nc

Copy link
Copy Markdown
Collaborator

@d-burg whats the status of this? Should it be reviewed now? Or should it still be marked as draft?

@amlakbekaluachule

amlakbekaluachule commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Fixed both flagged issues — SetResistiveWidthPsihigh.jl is now included/exported, and δ_FKR/δ_visco are documented in the LayerWidths docstring.

@logan-nc

Copy link
Copy Markdown
Collaborator

@claude review this

@claude

claude Bot commented Aug 15, 2026

Copy link
Copy Markdown

Review

Reminder (per repo policy): this PR must not be merged without third-party human review, regardless of any automated approval. Also — please run the regression harness (regress --cases <relevant_case> --refs develop,local) before merge; this PR touches LayerWidths/slayer_layer_thickness, which is exactly the kind of numerical-output change the harness is meant to catch.

Correctness / design concerns

  1. Hardcoded example data file inside an exported library function. set_resistive_width_based_psihigh unconditionally loads examples/DIIID-like_ideal_example/TkMkr_D3Dlike_Hmode_kinetic.h5 (SetResistiveWidthPsihigh.jl:39) regardless of the equilibrium/kinetic data actually carried by the caller's r. Since the function is exported from both SLAYER and InnerLayer, any user who calls it on a different equilibrium silently gets DIII-D-like kinetic profiles instead of their own, and the relative path only resolves when the working directory happens to be the repo root. build_slayer_inputs already accepts a profiles::KineticProfiles argument (LayerInputs.jl) — this function should take the same, rather than reading a fixed file. If this really is meant to be a one-off sanity-check script for that specific example (as the PR title suggests), it probably shouldn't be a permanent exported module API at all.

  2. Reimplements existing rational-surface-finding instead of reusing it. find_psi_for_q brute-forces a 200,000-point grid of q_of_psi_real per call (called once per m in q_scan, i.e. up to ~39× → ~7.8M spline evaluations) followed by a hand-rolled 60-iteration bisection. src/ForceFreeStates/Sing.jl already has _find_rational_surfaces/rational_psi_nodes, which locate q=m/n surfaces via Roots.jl's find_zero(..., Roots.Brent()) directly on profiles.q_spline, segmented between q-extrema (so it's also reverse-shear-safe, which the new bisection is not — it just finds the first sign change on a dense scan). Roots.jl is already a project dependency. Per CLAUDE.md's minimal-change-discipline section, this should reuse rational_psi_nodes/_find_rational_surfaces instead of re-deriving an independent, less robust q(ψ) inversion.

  3. omega_star_at (SetResistiveWidthPsihigh.jl:59-67) duplicates _omega_star_at in LayerInputs.jl almost verbatim. Same formula, same sign convention, same structure — this is a second copy of logic that already exists and is already exercised by build_slayer_inputs.

  4. Extrapolation edge case: A = (q2 - q1) / (log(1-ψ1) - log(1-ψ2)) (SetResistiveWidthPsihigh.jl:9) divides by a log-difference that goes to -Inf if x_knots[end] == 1.0 exactly, making A = 0; the target_q > q_last branch then computes exp((q_last-target_q)/A) → division by zero. In practice psihigh < 1 is typical, but there's no guard or comment noting this assumption, and this is precisely the kind of degenerate input this function is meant to help users avoid — worth an explicit check/error message.

Physics traceability

  1. δ_FKR = r_s · S^(-1/3) is labeled "FKR resistive-inertial length scale" (LayerThickness.jl docstring), but the classic Furth-Killeen-Rosenbluth (1963) resistive-inertial ("constant-ψ") tearing-layer width scaling is usually quoted as δ ∝ r_s S^{-2/5}, not S^{-1/3} (the S^{-1/3} scaling is closer to the Δ'→∞ / resistive-internal-kink limit). No equation citation is given, and neither docs/development/references.md nor docs/resources/ currently lists an FKR reference — only the 2026 Burgess two-fluid-layer paper is present. Given CLAUDE.md's explicit encouragement to cite the source equation for physics kernels, please add the citation (equation number) this formula comes from, and double check the exponent — this looks worth a pass from the fortran-physics-reviewer agent before merge.

Style / consistency

  1. New struct fields δ_FKR, δ_visco (LayerThickness.jl:155-156) use unicode Greek + mixed casing, while every other field in LayerWidths/SLAYERParameters uses ASCII delta_*/d_* snake_case (delta_s, delta_s_m, d_beta). This is inconsistent with the rest of the struct and forces awkward unicode access like lw.δ_visco in the new call site — recommend delta_fkr/delta_visco to match convention.
  2. Stray blank line before end in the LayerWidths struct definition (between the new fields and end) — minor formatting nit.
  3. q1_val (SetResistiveWidthPsihigh.jl:63) is bound to dqdpsi_hybrid(ψ_m) (a derivative), reusing the name shortly after q1, q2 were bound to actual q-values at ψ1, ψ2 a few lines above — easy to misread; consider dqdpsi_val or similar.
  4. Parameter name r for the response/model object (set_resistive_width_based_psihigh(r; ...)) is easy to confuse with the various rs/rs_val (minor radius) variables used throughout the same function.

Documentation / tests

  1. Both new exported functions (set_resistive_width_based_psihigh, print_layer_width_table) have no docstrings. Per CLAUDE.md, Documenter CI fails on missing_docs for any exported symbol without a docstring — this should fail the docs build as-is.
  2. No test coverage was added for the two new LayerWidths fields or for the new 158-line function (diff touches only src/, no files under test/).

None of the above touch the pre-existing slayer_layer_thickness/LayerWidths core computation beyond the two new fields, so the blast radius of a fix should be contained to the new file plus the docstring/struct nits.

@logan-nc

Copy link
Copy Markdown
Collaborator

Status

Rewritten from the original 172-line contribution, which had zero call sites and compared layer widths in metres against positions in normalized flux. That file is gone.

Feature[Equilibrium] psihigh_from_layer_overlap, opt-in, default off. Forms a first-pass equilibrium, finds where adjacent rational surfaces' resistive layers overlap, checks the reconstruction can actually deliver the resulting domain, and re-forms. Floored by the rational-q window containing psiedge so the dW scan band stays open. The scan runs whenever kinetic profiles are readable regardless of the flag, so gpec.h5 always records Equilibrium/psihigh_resistive_layers beside psihigh_resolved, plus per-surface widths under Equilibrium/ResistiveLayers/.

Default is off because enabling it crosses a rational surface on a diverted plasma: on DIIID-like_ideal_example the domain goes 0.995 → 0.993281, 5 surfaces → 4, mpert 35 → 34, moving et[1] 76% and NTV torque ~1800%. That is the criterion working, not a defect, but not a change to make on every user's behalf silently.

Nine defects fixed along the way, all found by trying to make the feature trustworthy:

defect consequence
field-line ODE retcode never checked truncated solves consumed as closed flux surfaces — silently wrong equilibria
round-trip check sampled only at ψ knots structurally blind to inter-knot ringing; passed while et[1] reached −47707
surface_da_dpsi stencil clamped at 1−1e-4 the real limit on near-boundary accuracy, not psi_cap
run_slayer passed b0exp as B_T Lundquist number off by ~95% — split out as #399
trial formations unguarded crash instead of back-off, psihigh left corrupted
round-trip unavailable on the efit path the path every shipped deck uses
gpec.h5 replay crash stored relative profile paths don't resolve in the output dir
edge q-law duplicated, applicable to limited plasmas one shared helper, with a guard that actually discriminates
inverse equilibria extrapolated fabricated 3 non-existent surfaces on the CHEASE fixture

Verification — 122 + 67 + 59 + 32 + 14 tests; docs clean; the metres→flux conversion triple-checked three independent ways (the binding pair is 1.821 mm apart with half-widths summing to 2.338 mm, argued in pure metres); the back-off gate validated against ForceFreeStates energies as ground truth, and it flips one step before they degrade.

Still open

@logan-nc

Copy link
Copy Markdown
Collaborator

@d-burg — ready for review, but please take #399 first.

#399 is the one-line bt fix (SLAYER was running at b0exp = 1.0 T instead of the physical ~1.95 T, so the Lundquist number was off by ~95%). It is cherry-picked here as 4304caa, so once #399 merges this branch's copy resolves cleanly and the diff shrinks to this PR's own work.

That ordering also separates the regression movement. diiid_slayer_n1 needs re-pinning, and with #399 landed first it is unambiguous which part is the bt correction and which is this feature — measured separately, #399 alone accounts for S +94.5%, D_norm 35%, γ 4.4%.

Worth your attention specifically:

  • The psiedge floor governs every DIII-D deck, so the layer criterion never actually sets the domain in the shipped examples — each asks for ~0.9892 and is held at the q=6 window top. Working as specified, but you may want a deck that exercises the physics path.
  • The bt fix changes the layer widths themselves, so any Tearing/LayerWidths/ numbers you have from before InnerLayer.SLAYER - BUGFIX! - Use the physical toroidal field, not the b0exp normalization #399 were computed at half the true field.
  • delta_s_abs now appears in two h5 groups. They agree to ~1e-6 and the difference in scope is documented in hdf5-conventions.md; flag it if that still reads as ambiguous.

Everything except the opt-in flag is a bug fix that stands on its own — the field-line ODE retcode was never checked, and the round-trip gate was structurally blind to spline ringing (it passed while et[1] hit −47707).

@logan-nc logan-nc changed the title resistive-layer-width sanity checks for SLAYER Resistive-layer-width based equilibrium truncation Aug 18, 2026
@logan-nc logan-nc removed their assignment Aug 18, 2026
@logan-nc
logan-nc removed the request for review from jhalpern30 August 18, 2026 18:37
@logan-nc

Copy link
Copy Markdown
Collaborator

@d-burg, as claude says this has hit a point where it needs your expert review. Workflows are in place to decide the truncation from the resonant layer widths. The issue is that it is reporting big widths and wanting to truncate way in (~0.9892 for DIII-D). I find this suspect, since Fitzpatrick had been finding things like to be very far out past 0.9999. In the process of adding the truncation logic, claude did convince me the units of the layer widths had been wrong in the original overlap-checking-loop (comparing layer widths of m to rational spacing in psin) and a conversion from m to psin was added... so maybe double check that?

Do you think we need to try and reproduce the exact case from Fitzpatricks paper on this as a sanity check?

@logan-nc

Copy link
Copy Markdown
Collaborator

@d-burg please hold for me to find time to skim the full diff. Quick thoughts from the comments:

Architecture, per your sign-off: the overlap point is an upper bound on psilim, nothing
more. It is applied as a cap on qlim at the top of sing_lim!, ... roughly 1800 insertions become about 700.

Great. I like this simplification a lot. Open physics question: Do we want to always cut inside this stochastic boundary (cut off some valid region) or always get at least to this boundary (include a little invalid region in the name of making sure we capture all the valid region)? The former is numerically easier (we don't have to go out as far), but perhaps the later in more physical (debatable - lets debate it here a bit)?

Still open, and I would not gate this PR on it: the two width channels now disagree by about two
surfaces (|delta_s| cuts at 0.98978, Eq. 100 at 0.99939).

Please explicitly confirm non of the r-vs-rho and other normalization bugs need to be extended to delta_s. Are we tripple sure this one is being implemented correctly for getting a Jacobian independent psi width (note that this is easily testable by running with Hamada and PEST)?

The feature is a no-op on everything we ship.

Do we want this to be true? If the feature is the future and how we want users to default use the code, then shouldn't we be shipping examples that use it (i.e. psihigh 0.9995 for all DIII-D examples)? The DIII-D like example inherited the dmlim truncation behavior only to make it easy to compare to fortran, but we've moved past that I think (and the bechmark_with_fortran script can easily account for it by defaulting to a dmlim type comparison). @jhalpern30 and @matt-pharr may want to add their two cents.

@logan-nc
logan-nc requested a review from jhalpern30 August 25, 2026 17:39
@logan-nc

Copy link
Copy Markdown
Collaborator

@d-burg and I would like to request @jhalpern30's review. Primarily focus on the top level workflow, which now does not touch the psihigh for equilibrium reconstructions but does sets a psilim that the standard truncation logics (dmlim, max dW, etc.) cannot exceed. Also, please weigh in on this being a no-op on the shipped examples (i.e. should we change psihigh to 0.995 for the DIII-D examples).

Of course, calling out AI nonsense is much appreciated too.

@d-burg please do post replies to my comments above. You can assume my review becomes an approval if @jhalpern30 or another human looks through the diff and approves.

@d-burg
d-burg force-pushed the bugfix/slayer-dprime-reference-length branch from baf6746 to 29df870 Compare August 27, 2026 03:45
d-burg and others added 6 commits August 26, 2026 23:46
…e layer width

Adds three algebraic scales to `LayerWidths` alongside the Riccati `delta_s`:
`delta_norm` (r_s S^-1/3), `delta_visco` (the P^1/6 viscous-resistive broadening) and
`delta_dr`, the diffusive-resistive width of Fitzpatrick, Nucl. Fusion 2025
(doi 10.1088/1741-4326/ae4fdd) Eq. (100), which sets the edge layer-overlap criterion.

The mapping of Eq. (100) into SLAYER variables is not literal. The paper's tau_A
(its Eq. 74) carries no shear, whereas SLAYER's tau_h divides by n*s, so
lu = tau_R/tau_h = (n|s|) * (tau_R/tau_A). Substituting that cancels Eq. (100)'s
explicit (n|s|)^(-1/2) exactly, leaving

    delta_dr = r_s^(3/2) * P_perp^(1/4) / ( lu^(1/2) * d_beta^(1/2) )

with no shear factor. Carrying the (n|s|)^(-1/2) on top of SLAYER's lu double-counts
the shear and understates the width by sqrt(n|s|) -- a factor of 6.6 at q=7 on the
DIII-D-like deck, enough to report no overlap anywhere.

Validated against the paper's own Sect. 5.8 JET model, which this reproduces at
Psi = 0.9985 for n = 1 (paper: 0.9985) and 0.9936 for n = 4 (paper: 0.9952).

Marked `!` because `LayerWidths` gains fields.
…er-overlap scan

Factors the diverging edge model q ~ -A*ln(1-psi) out of the grid-refinement density
floor into `edge_q_law`, so one fit serves both that floor and the resistive-layer
overlap scan's search for rational surfaces beyond the equilibrium grid. A cubic in
psi (on q or on iota) saturates instead of diverging and is not usable for the
extrapolation; measured on the DIII-D-like deck, extrapolating from a grid ending at
psi=0.97 to the true q=8 surface at psi=0.99976, the log law lands within 5% while a
cubic iota undershoots by 18%.

`edge_q_law` returns nothing when the diverging model does not describe the
equilibrium, judged by knot count, sign of A, and the log fit beating a plain linear
one. That test now gates the edge density floor, which was previously applied
unconditionally: a limited plasma with finite edge q is no longer packed as if q blew
up. Marked `!` because that changes the auto grid, and so results, for limited decks
(Solovev, LAR); diverted decks are unaffected.

Also adds an optional `psihigh` to `refined_psi_grid`, which truncates the measured
knot density so a pass-1 equilibrium can supply the density for a reduced domain.
Locates the q = m/n rational surfaces, computes each one's resistive layer width, and
reports the outermost psi at which adjacent layers are still separated -- the outermost
domain the matched-asymptotic treatment supports, per Fitzpatrick, Nucl. Fusion 2025
Sect. 5.9. Surfaces beyond the equilibrium grid are located on the shared separatrix
edge q-law, so the scan can report an overlap point lying outside psihigh; that is
recorded rather than clipped, since it is the bookkeeping a user needs to see whether
the domain was constrained by layer physics at all.

Widths are compared in normalized flux, converted from metres by the Jacobian of
whichever radial label `rs_method` selects -- the same label that sets the shear,
tau_R and d_beta/r, because Eq. (100) is not covariant (delta ~ J^(1/2) under a change
of radial variable) and is only valid in the coordinate it was derived in. On the
DIII-D-like deck the label is worth one rational surface: the cut sits at psi=0.99752
under `:midplane` and 0.99939 under `:flux`, Fitzpatrick's Eq. (30) label.

Reports both the |delta_s| and Eq. (100) channels separately so the two can be
compared; they disagree by about two surfaces, which is the subject of issue #411.
…ve-layer overlap

`sing_lim!` gains an optional `psilim_cap`. Where the resistive layers of adjacent
rational surfaces overlap, no surface retains a well-separated inner region and
matched asymptotics is not defined, so that location is an upper bound on the useful
domain.

It is applied as a cap on `qlim` rather than by assigning `psilim` directly, which
keeps it a bound rather than a setpoint: the existing `dmlim` / `qhigh` truncation
still selects the final surface from inside it, and a cap lying beyond `psihigh` is
inert by construction with no special case -- it simply loses the `min`. The domain
can therefore only shrink, never grow, so no equilibrium ever has to be re-formed.

Measured on the shipped DIII-D decks with the Eq. (100) width in the `:flux` label,
the cap changes no retained surface on any of them: it is beyond `psihigh` for the
ideal (0.995), riccati (0.995) and gal_resistive (0.993) decks, and on the SLAYER deck
(0.9995) it binds by 1.1e-4 in psi without crossing a rational. The scan needs kinetic
profiles, which two of those decks do not carry, so it is inert there regardless.
…d Fitzpatrick JET case

Rebuilds the model JET equilibrium of Fitzpatrick, Nucl. Fusion 2025 Sect. 5.8 from the
paper's own definitions -- the Eqs. (32)-(35) safety factor with alpha-/alpha+ solved
self-consistently against Eq. (36), and mtanh edge profiles anchored to its Fig. 8 -- and
checks where adjacent resistive layers first overlap against the values the paper reports.

The widths are driven through `slayer_parameters` and `slayer_layer_thickness` rather than
recomputed here, so the test covers the shipped tau_R / tau_A / d_beta chain and the
`delta_dr` formula itself. That distinction matters: the bug this test would have caught --
carrying Eq. (100)'s explicit (n|s|)^(-1/2) on top of a `lu` that already contains it -- lives
in exactly that chain, and a test that reimplemented the width would have passed while the
shipped one was wrong.

Measured against the paper: Psi = 0.9985111 for n = 1 (paper 0.9985, deviation 1.1e-5) and
0.9949738 for n = 4 (paper 0.9952, deviation 2.3e-4). Bounds are set at 1e-4 and 1e-3, leaving
roughly 9x and 4x margin; the model is quadrature and root-finding with no BLAS, so platform
spread is orders of magnitude below either. The test logs both deviations so a future failure
reports how far it moved rather than only that it moved.

This is the suite's second check pinned to an external published reference rather than to
GPEC's own history.
Adds an InnerLayer (SLAYER) section to the per-module citation list, covering the
two-fluid slab layer paper the module is built on and the Fitzpatrick 2025 paper
whose Eq. (100) width and Sect. 5.9 overlap criterion the layer-overlap scan uses.
Both PDFs now live in docs/resources.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@d-burg
d-burg marked this pull request as draft August 27, 2026 05:07
d-burg and others added 4 commits August 27, 2026 01:21
…escales and widths

slayer_parameters took sval_r with its sign in five places that are magnitudes: the
parallel diffusivity chi_par_lmfp (twice), the island width Wd, the :lar critical-Delta,
and the Alfven time tau_h. On a reverse-shear surface tau_h came out negative, hence a
negative Lundquist number, and tauk = lu^(1/3)*tau_h threw a DomainError before any layer
quantity was produced. Found on DIII-D 153072_3415, whose q dips to 1.855 off-axis.

The layer timescales and widths depend on |dq/dr|, not its sign -- the module's own
delta_dr derivation already states the convention as (n|s|). abs(s) == s wherever the
shear is positive, so no currently-working surface changes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…erlap scan

The per-surface guard caught only ArgumentError, so a DomainError from any single surface
propagated and killed the whole scan. The scan already builds surfaces one at a time
precisely so a degenerate one can be recorded and skipped; this makes that hold for the
error type it is most likely to hit -- kinetic profiles that stop short of an extrapolated
surface, or a fractional power reached with a negative argument.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… rather than an absolute r2

EDGE_FIT_MIN_R2 = 0.99 was calibrated against the synthetic TokaMaker deck (r2_log 0.9957,
0.9989). Real EFIT/CAKE reconstructions of diverted DIII-D plasmas carry reconstruction
noise in the edge q and fit the log law at r2_log = 0.929-0.977, so every one of them was
rejected: no surfaces were placed past the equilibrium grid and the layer-overlap scan
reported "no overlap" on 8 of 9 real shots -- a silent fail-open on the exact case the
criterion exists for.

No absolute threshold can do this job: the limited a10 fixed-boundary deck sits at 0.972,
inside the diverted band. The r2_log > r2_linear comparison does separate the classes
(diverted 0.96 vs 0.75; Solovev 0.760 vs 0.9996) and is unchanged. The floor is lowered to
0.90 rather than removed so a fit describing nothing is still rejected.

Carries ! because the gate also controls the auto-grid edge density floor: an equilibrium
that was previously rejected now receives it, changing its refined grid. Every shipped deck
is unaffected (the DIII-D-like deck passed both thresholds; Solovev and LAR reject on the
relative test; a10 rejects on knot count), so no harness case moves -- but a user running a
real reconstruction on grid_type = "auto" will see a different grid.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…f a layer inside it

The scan returned psi[k-1] - w[k-1]/2, which drops the outermost surface the criterion
found to be clean. Fitzpatrick (2025) Sect. 5.9 retains the rational surfaces in
0 < psi < 1 - eps_c, and psi[k-1] is 1 - eps_c: the surface itself is the boundary. The
branch's own external check, runtests_layer_overlap_fitzpatrick.jl, already reports
Psi_of(S[k-1]) and matches the paper's 0.9985 / 0.9952 with it, so the scan and its
validation were using different conventions.

Measured over ten equilibria (the shipped DIII-D-like deck and nine IDA DIII-D
reconstructions), this moves the limit outward by 1.2e-3 to 4.6e-3 in psi, which is
consistently one rational surface. The symmetric alternative, psi[k-1] + w[k-1]/2, is not
usable: it exceeds psi = 1 on four of the ten, and on those same four it also overshoots
surface k, the layer half-widths there being larger than the surface separation.

Also refreshes four docstrings left stale by earlier revisions: the psihigh field described
a "smaller of the two" rule the code no longer uses, psihigh_dr was undocumented, the
psi_cap note described a finite-difference stencil clamp removed with the analytic
radial_label derivatives, and the finite-width guard cited the same defunct rule.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@d-burg

d-burg commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

@logan-nc all three taken, and you were right that the entry never landed — the branch rewrite dropped references.md from the diff entirely, so your comments ended up orphaned on a file this PR no longer touched. Fixed now:

  • The paper is in docs/resources/ (arXiv:2511.07666v2, the open-access version of doi 10.1088/1741-4326/ae4fdd).
  • references.md gets a one-liner matching the surrounding entries, not the essay. There was no InnerLayer/SLAYER section at all — not even the Burgess 2026 paper the module is built on — so I added a short one covering both.
  • "which is exactly" is gone, along with the claim about other response codes; I could not support it and it should not have been asserted.

In-source annotations already cite it (LayerOverlap.jl header, the delta_dr docstring in LayerThickness.jl), which was your third point.

One correction while I was in the paper: the figures are Fig. 9 for n = 1 and Fig. 10 for n = 4, not 10 and 11. The abstract states ε_c = 1.5e−3 and 4.8e−3 directly, and 1 − ε_c gives Ψ = 0.9985 and 0.9952 — exactly the two values the test asserts.

@d-burg

d-burg commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

The convention question, settled with data

@logan-nc you asked whether to cut inside the overlap boundary or reach it. I ran it across ten equilibria — the shipped DIII-D-like deck plus nine IDA DIII-D reconstructions — and the answer is neither of the half-width options. All values in ψ, n = 1, Eq. (100) width:

case cut_inside at_surface reach next_surf
shipped D3D-like 0.997414 0.998508 0.999602 0.999684
153072_3415 0.996756 0.998617 1.000477 0.999633
153833_3450 0.995828 0.997955 1.000082 0.999752
153841_3000 0.994295 0.996575 0.998854 0.999559
169510_3000 0.998939 0.999547 1.000155 0.999933
201586_4200 0.990769 0.992546 0.994324 0.995331
204441_4400 0.996759 0.998276 0.999794 0.999634
204441_5307 0.995140 0.997262 0.999384 0.999825
179633_3200_L 0.998810 0.999503 1.000196 0.999902
189392_4080_L 0.996998 0.998379 0.999760 0.999774

cut_inside = ψ[k−1] − w/2 (what the code did), at_surface = ψ[k−1], reach = ψ[k−1] + w/2, next_surf = ψ[k].

reach is not implementable. It exceeds ψ = 1 on four of ten — a domain limit outside the plasma. On those same four it also exceeds next_surf: the half-width of the last clean surface extends past the very surface it is meant to exclude, because by then the layers are wider than their separation.

at_surface is the answer, and it is "reach the boundary" in the only well-defined sense. It is the paper's own 1 − ε_c; it is what runtests_layer_overlap_fitzpatrick.jl already reports in order to hit the paper's 0.9985 / 0.9952; it is always inside the plasma and always inside the first contaminated surface. It also removes an inconsistency I should have caught earlier — the scan and its own external validation were using different conventions.

Changed in d10d8b417. Moves the limit outward by 1.2e−3 to 4.6e−3 in ψ, which is consistently exactly one rational surface.

@d-burg

d-burg commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

@logan-nc on your δ_s question from 2026-08-21:

Confirmed clean, by measurement rather than assertion. I took your suggestion and ran it under three jacobians (pest, hamada, equal_arc). Everything is evaluated at fixed ψ_N, so the jacobian sets the θ-coordinate and nothing that matters should move. Max relative spread, default :midplane label:

quantity spread
r_s ≤ 1.3e-16
da/dψ (the metres→flux Jacobian) ≤ 3.0e-14
|δ_s| in metres ≤ 7.4e-06
δ_s as Δψ ≤ 7.4e-06
δ_DR (Eq. 100) as Δψ ≤ 1.2e-06

The ~1e-6 floor on the widths is the equilibrium solve's own coordinate-dependent numerical noise propagating through the Riccati layer solve, not a coordinate artifact — the inputs feeding it are invariant to 1e-14. Same result under the :flux label. So δ_s and the metres→ψ conversion are Jacobian-independent, and none of the r-vs-ρ normalization issues need extending to it.

The check did find something adjacent, though it does not touch the truncation: :fsa and :halfwidth are coordinate-dependent. :fsa is a uniform mean over the coordinate's θ, which is a genuine flux-surface average only when the jacobian is already uniform in θ — so r_s moves 3.8–6.6 % and da/dψ up to 14.5 % on a jac_type change alone. Full table on #403, where that code lives. Neither label is the default and neither is used here.

Your third question — whether this should stay a no-op on the shipped decks — I'll answer separately; worth noting for @jhalpern30 / @matt-pharr that the decks are not uniform today: SLAYER and IMAS are already at psihigh = 0.9995, ideal and riccati at 0.995, gal_resistive at 0.993.

@d-burg

d-burg commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Status: moved to draft; two bugs found against real data

resistive_layer_overlap has no call site, sing_lim!'s psilim_cap is never passed, refined_psi_grid's psihigh has no caller, and there is no TOML key or HDF5 output. The body's claims — "upper bound on psilim", "the overlap point is recorded whether or not it binds", and the per-deck binding numbers — describe a prototype whose wiring was lost in the rewrite. The body will be rewritten to match.

Scanning ten real equilibria to settle the convention question turned up two things worth knowing.

1. The edge q-law gate rejected 8 of 9 real DIII-D reconstructions (d4ce058ff)

EDGE_FIT_MIN_R2 = 0.99 was calibrated against the synthetic TokaMaker deck (r²_log = 0.9957, 0.9989 — smooth and analytic). Real EFIT/CAKE reconstructions carry noise in the edge q and fit the log law at r²_log = 0.929–0.977, so every one of them was refused. No surfaces were placed past the equilibrium grid, and the scan reported "no overlap" — a silent fail-open on exactly the case the criterion exists for.

class case r²_log r²_linear
diverted (real) five DIII-D reconstructions 0.929–0.977 0.713–0.773
diverted (synthetic) TokaMaker D3D-like 0.9957, 0.9989
limited a10 fixed-boundary 0.972
limited Solovev 0.760 0.9996

No absolute threshold can do this job: the limited a10 deck sits at 0.972, inside the real diverted band. The old 0.99 only appeared to work because it was tuned against synthetic data that happens to be smooth. The r²_log > r²_linear comparison does separate the classes and is unchanged.

Why 0.90 rather than removing the absolute test: it is a floor against a fit that describes nothing at all, kept deliberately weak so the relative test does the discriminating. It clears the measured diverted band (0.929–0.977) with margin while still rejecting Solovev-class fits outright. I would have dropped the absolute test entirely except that only one shipped limited deck (Solovev) actually exercises the relative rejection — a10 rejects on knot count before reaching it — which is thin evidence for removing a safety check. If someone has a limited deck with a well-fitting log edge, that is the case that would change my mind.

Carries ! because the same gate controls the auto-grid edge density floor: an equilibrium previously rejected now receives it and gets a different refined grid. Every shipped deck is unaffected (the DIII-D-like deck passed both thresholds; Solovev and LAR reject on the relative test; a10 on knot count), so no harness case moves — but a user running a real reconstruction on grid_type = "auto" will see a different grid.

2. Reverse shear crashed the layer build — split out as #431

slayer_parameters used the signed r-based shear in five expressions that are magnitudes, so a negative-shear surface produced a negative Lundquist number and tauk = lu^(1/3)·tau_h threw a DomainError before any layer quantity existed. Shot 153072_3415 has q dipping to 1.855 off-axis, giving two q = 2 surfaces with the inner one on the negative-shear branch — and _find_rational_surfaces is deliberately reverse-shear-safe, so it hands the layer build exactly those surfaces.

Pre-existing on develop, not introduced here, so it is its own PR. Cherry-picked onto this branch so the scan can run that shot; the copy resolves away once #431 merges.

Also 6781b639f: a single failing surface no longer aborts the whole scan — the per-surface guard caught only ArgumentError, so one bad surface took down the run instead of being noted and skipped.

Other changes since you last looked

…ve-layer overlap point

Wires the resistive-layer overlap scan into the stability workflow. The scan runs whenever
kinetic profiles are readable and its result is always recorded under
ForceFreeStates/LayerOverlap/, so a run shows where layer physics would cut the domain
whether or not it did. The cap is applied only under the new opt-in ForceFreeStates control
psilim_from_layer_overlap (default false).

Architecture as agreed on the PR: an upper bound on qlim at the top of sing_lim!, so dmlim /
qhigh still select the final surface from inside it and a bound beyond psihigh is inert by
construction. Nothing re-forms the equilibrium.

Profiles are taken from whichever of [KineticForces] kinetic_file or [SLAYER] profile_file
exists on disk -- the shipped DIII-D SLAYER deck carries only the latter, so keying on the
NTV path alone would have left the feature dead on the one deck it matters for.

The scan is driven at rs_method = :flux rather than the SLAYER default. Eq. (100) is not
covariant and is anchored to the toroidal-flux label of the paper's Eq. (30); running it in
the midplane label would apply the formula in a coordinate it was not derived in.

Verified end to end on the DIII-D-like SLAYER deck. At its shipped psihigh = 0.9995 the
overlap point (0.999695) lies outside the domain, so the scan is recorded with applied = 0
and nothing moves. Raising psihigh to 0.99995 makes it bind: qlim 8.536 -> 8.000 at
psi 0.999756, dmlim then selects 7.200 from inside it and the q = 8 surface is filtered out.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d-burg and others added 3 commits August 28, 2026 13:17
…ational surfaces are found

The no-surface early return built LayerOverlapScan with 13 arguments for a
15-field struct (width_dr and first_overlap missing), a MethodError the
pipeline's catch downgraded to a spurious scan-failure warning. Also make the
JET reproduction fail loudly when its alpha fixed point or Psi=1.05 bracket
degenerates instead of proceeding on a silent fallback, document the
rs_method and max_layer_solves kwargs, and compress the investigation
narratives to the reasoning they support.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n-failure diagnostics

The applied flag records that the bound entered sing_lim! as an active cap,
not that it set the final psilim (dmlim/qhigh may truncate deeper) -- say so
in the schema. Carry the backtrace in the scan-failure warning so a
programming error is not indistinguishable from a physics refusal, and
compress the edge-fit floor comment.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@jhalpern30

Copy link
Copy Markdown
Collaborator

Please ping me again when this is ready for review - it seems to still be being developed. Turning off auto-notifications on this for now to avoid my inbox being blown up

@logan-nc

logan-nc commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Same @d-burg. It says

d-burg marked this pull request as ready for review 15 hours ago

above but I see no comment explaining how/why it got promoted from draft. The SLAYER stack is big rn so I am a bit lost - hold our hands through this please 🙏

@d-burg

d-burg commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

I put together a review package for this PR — the overlap criterion and cut conventions as one diagram, the always-recorded / opt-in-applied wiring, the ten-equilibrium convention evidence, and the bugs found en route.

https://claude.ai/code/artifact/71b5b47a-7b76-45e8-9714-bd43041bcde4

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

Labels

changed-results Results move or an interface breaks - read before upgrading feature New capability

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants