InnerLayer.SLAYER - BUGFIX! - Normalize D by the electron diamagnetic share, not the temperature ratio - #434
Open
d-burg wants to merge 5 commits into
Open
InnerLayer.SLAYER - BUGFIX! - Normalize D by the electron diamagnetic share, not the temperature ratio#434d-burg wants to merge 5 commits into
d-burg wants to merge 5 commits into
Conversation
… share, not the temperature ratio D_norm used sqrt(tau/(1+tau)) with tau = T_i/T_e. Fitzpatrick defines the ion-sound-radius normalization as D = S^(1/3) iota_e^(1/2) (d_beta/r_s), and iota_e reduces to 1/(1+tau), not tau/(1+tau) -- the same expression with the electron and ion temperatures transposed. The two agree only at T_i = T_e. Use the iota_e that slayer_parameters already computes from omega_*e and omega_*i. That is Fitzpatrick's own definition, iota_e = Q_e/(Q_e - Q_i), and it is strictly more general than either closed form: it carries the ratio of the electron and ion pressure-gradient scale lengths rather than assuming they match. Guard against a non-positive iota_e so a same-sign diamagnetic input raises a clear error instead of a DomainError under the square root. Results move on any deck with T_i != T_e, including the shipped SLAYER deck. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…entary Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Put the explicit D = S^(1/3)*iota_e^(1/2)*(d_beta/r_s) statement back above the code so the formula-to-theory traceability survives next to what it computes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d-burg
requested review from
jhalpern30
and removed request for
logan-nc and
matt-pharr
August 28, 2026 20:09
…site-drift diamagnetic signs The shared test inputs set omega_e and omega_i both positive, which no plasma produces: electrons and ions drift oppositely, the pipeline computes omega_*e < 0 < omega_*i (all six shipped-deck surfaces have Q_e > 0 > Q_i, matching TJ), and same-sign inputs put iota_e = 2 outside its physical (0, 1) range. Flip omega_e to -1e4 so iota_e = 2/3, matching the hand-built small-D fixture, and update the two assertions that encoded the old values. Branch selection cannot move: iota_e appears on both sides of the regime test and cancels. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d-burg
marked this pull request as ready for review
August 28, 2026 20:58
jhalpern30
approved these changes
Sep 2, 2026
jhalpern30
left a comment
Collaborator
There was a problem hiding this comment.
Trusting your verification on the physics here, looks ok. Just clean up the LLM comments and its good to go
| ) | ||
| ) | ||
| iota_e = Q_e / Q_e_minus_Q_i | ||
| # D below takes sqrt(iota_e). iota_e lies in (0,1) whenever ω_*e and ω_*i have |
Collaborator
There was a problem hiding this comment.
No need for this comment - the error text is self explanatory
| # Normalized beta-related width and Δ-normalization | ||
| d_beta = c_beta * d_i | ||
| D_norm = (d_beta / rs) * lu^(1.0 / 3.0) * sqrt(tau / (1.0 + tau)) | ||
| # D = S^(1/3)·iota_e^(1/2)·(d_beta/r_s), Fitzpatrick's ion-sound-radius normalization. |
Collaborator
There was a problem hiding this comment.
This comment also feels kinda "Claude explaining code that it removed that quickly becomes out of date".
Not sure if this even needs a comment, or if it does, maybe just a reference to the relevant equation in Fitzpatrick's paper if desired
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release note
D_normmoves on every deck with T_i ≠ T_e, including the shippeddiiid_slayer_n1case (D_norm 20.4%, γ_Hz 9.8% ondiiid_slayer_n1). The ion-sound-radius normalization had the electron and ion temperatures transposed. (harness @ 9b44f8c)D_norm,Q_root,ω_Hzandγ_Hzfrom earlier runs are not comparable to new ones, so archived numbers and any figures built from them need regenerating. No API change. One behavioural change to watch for: a caller passing same-signω_*e/ω_*i(unphysical, but previously silently accepted) now raises anArgumentErrorfromslayer_parametersinstead of producing a value.D_normused√(τ/(1+τ))withτ = T_i/T_e. Fitzpatrick's definition isD = S^(1/3)·ι_e^(1/2)·d̂_β, andι_ereduces to1/(1+τ), notτ/(1+τ). The two agree only atT_i = T_e. GPEC already computes the correctι_eand did not use it.The defect
From TJ
Documentation/Layer.tex:D = S^(1/3) · ι_e^(1/2) · d̂_βι_e = Q_e/(Q_e − Q_i)ι = (T_e/T_i)·((1+η_e)/(1+η_i)), withι_e = ι/(1+ι)With
η_e = η_ithe third line givesι = T_e/T_i = 1/τ, henceGPEC used
τ/(1+τ) = T_i/(T_e + T_i)— the same expression with the two temperatures swapped. Equal atτ = 1, divergent in opposite directions otherwise.Note that (282) is exactly the
iota_eGPEC already computes and stores from ω_*e/ω_*i. The fix is to use it.Why the computed
iota_eand not1/(1+τ)1/(1+τ)is only theη_e = η_ilimit. The fullιalso carries(1+η_e)/(1+η_i), the ratio of the electron and ion pressure-gradient scale lengths — TJ's own text callsι"the ratio of the electron to the ion pressure gradient at the rational surface". GPEC'siota_e = Q_e/(Q_e − Q_i), built from the actual ω_*e and ω_*i spline derivatives, captures that automatically. It is both TJ-faithful and strictly more general than either closed form. TJ itself hardcodesι_e = 0.5(Rational.cpp:198), so this is a case where GPEC can do better than the reference.Impact on the shipped deck
examples/DIIID-like_ideal_example/TkMkr_D3Dlike_Hmode_kinetic.h5hasT_i ≠ T_e— 2534.73 vs 1952.59 eV at index 200, soτ ≈ 1.298:√(τ/(1+τ))√ι_e(η-equal estimate 0.435)D_normratioSo this is not latent — unlike the companion n-factor PR, it changes
diiid_slayer_n1today.Guard
√ι_erequiresι_e > 0, which holds whenever ω_*e and ω_*i have opposite signs — the physical case, and the sign convention theslayer_parametersdocstring already states. Same-sign inputs with|Q_i| > |Q_e|would makeι_e < 0; that now raises a clearArgumentErrorrather than a bareDomainError, mirroring the existing degenerate-Q_e == Q_iguard immediately above it.Validation
runtests_slayer_params49/49 — the one failure this produced wasruntests_slayer_params.jl:88, the assertion that pins the definition ofD_norm; it is updated to the corrected formula in this PR. That is a definition change, not a loosened tolerance.runtests_slayer_riccati29/29 — the large-D/small-D branch selection assertions still hold. Worth knowing: that fixture passes same-sign ω_*e/ω_*i (1e4,5e3), which is unphysical and givesiota_e = 2.0, soD_normroughly doubles there. The branch inequalities survive because the fixture sits well inside its regime, but the fixture arguably wants fixing separately — flagging rather than quietly retuning it.julia -t 4 --project=. test/runtests.jlata5dea9fed: 62 testsets, 2345 assertions, 0 failuresRegression report
diiid_slayer_n1,develop(e09795119) vs this head (a5dea9fed), both sides--force, identical pinned environments (julia 1.11.6, manifest7e5c34ad).The report is self-verifying
The change is surgical, and the table proves it rather than asserting it:
iota_eis0.0e+00. It was already computed correctly ondevelop— this PR only starts using it. That is the single strongest line in the table: it separates "we changed which quantityDis built from" from "we changediota_e".rs, shear, LundquistS,tauk,P_perp, the surface and mode-number lists. Nothing upstream ofDmoved.D_normmoves 20.4%, andγ_Hzfollows at 9.8%. That is the physical consequence: the ion sound radius sets the drift-tearing layer response.The γ shift is real, not solver noise.
diiid_slayer_n1's γ line has a known reproducibility floor from the threaded root search — 0.076/0.122/0.145 Hz per surface (#418), which showed up as 0.02% in #431. Here it is 77.45 Hz, 9.84% — nearly three orders of magnitude above that floor.On my pre-run estimate: I predicted ≈12% on
D_normfrom a single mid-radius point assuming η_e = η_i. The measured 20.4% is larger because τ varies across the six surfaces and the computediota_ealso carries the (1+η_e)/(1+η_i) factor that the closed form drops. The direction and order of magnitude held; the point estimate was not a prediction of the max.Note
Harness re-run at
9b44f8cfcafter the formula-citation comment restore touchedsrc/and staled the8df7535e3stamp (itself a re-run after an earlier comment-only commit). Each run reproduces the same numbers — the commits changed comments only — but the stamp must name a commit the harness actually ran at, so eachsrc/touch gets a real run rather than a re-pointed stamp.Notes for reviewers
Opened as a draft. This one changes shipped numbers and wants a physics sign-off before it goes anywhere near ready.
Found while auditing whether the #431 shear sign was load-bearing elsewhere. Independent of the companion n-factor PR — different file, no textual conflict, and the missing
ncancels iniota_e, so theiota_ethis PR starts using is already correct ondevelop.Cross-checks worth confirming against Park 2022 (
docs/resources/2022-Park-Parametric dependencies...) and Burgess 2026, which I have not read for this: TJ is one source and the SLAYER lineage may have intended a different closure.Suggested reviewer @matt-pharr (InnerLayer focus); assignee @d-burg. @logan-nc may want to see this too given it moves a harness case.