Skip to content

KineticForces - BUGFIX! - Drop the spurious major radius from the omega_D prefactor - #391

Merged
logan-nc merged 7 commits into
developfrom
bugfix/kf-omega-d-major-radius
Sep 4, 2026
Merged

KineticForces - BUGFIX! - Drop the spurious major radius from the omega_D prefactor#391
logan-nc merged 7 commits into
developfrom
bugfix/kf-omega-d-major-radius

Conversation

@logan-nc

@logan-nclogan-nc commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator

Release note

  • Audience: users
  • Numerical impact: omega_D changes by a factor of R0, moving NTV torque and delta-W on any machine with R0 != 1 m (diiid_n1: torque +4.55%, dW +15.85%; the other 45 quantities are exactly unchanged); Solovev fixtures are unaffected only because they set r0 = 1 (harness @ 4b0853e)
  • Migration: Re-baseline saved NTV torque and kinetic-energy comparisons — the precession frequency was previously too large by a factor of R0.

The general-aspect-ratio precession prefactor carried an extra factor of the major radius, leaving omega_D with units of m/s rather than a frequency. Torque and kinetic-energy results move on any machine whose major radius is not 1 m; the overall sign convention is a separate question and is not touched here.


What

One factor in the general-aspect-ratio precession prefactor (src/KineticForces/BounceAveraging.jl):

- wdbar = ro^2 * bo * wdfac * wbbar * 2 * (2 - sigma) * total_wd+ wdbar = ro * bo * wdfac * wbbar * 2 * (2 - sigma) * total_wd

Julia analog of Fortran GPEC PR #281.

Why

wbbar = ro·2π/((2−σ)·I₁) already carries one factor of ro, which its own normalization bhat = √(2T/m)/ro cancels. Reusing wbbar inside wdbar imports that ro a third time, while dhat = (T/q)/(B₀·ro²) removes only the two written explicitly.

Dimensionally: both bounce integrals carry the J·b·dθ = dl length, so I₁ is a length and I₂/I₁ is 1/Wb. With T/q in volts and V/Wb = 1/s, the prefactor 4π·wdfac·(I₂/I₁)·(T/q) is already a frequency — the surviving ro left omega_D in m/s.

Verification

Holding the physics fixed and varying only the machine size (both bounce integrals scale as the length they carry, so I₂/I₁ is ro-independent):

omega_D ratio when ro is doubled
before (ro^2)2.0still carries a length
after (ro^1)1.0frequency, ro-invariant

omega_b, built from the same I₁, is untouched and keeps its correct v_th/L scaling (ratio 0.5) — that is the control showing the change is confined to the precession prefactor.

Upstream measured the same correction against an independently validated guiding-centre precession operator: the least-squares slope of omega_D against the reference moves from -6.494 (median over 7 surfaces; -ro = -6.411 m on that ITER equilibrium) to -1.021, with omega_b agreeing to within [0.9964, 1.0011] both before and after.

Impact — this is not a small change

omega_D changes by a factor of ro on every machine (≈6.4× on ITER scale). It feeds the resonance denominator ℓ_eff·ω_b·√x + n·(ω_E + ω_D·x), so all NTV torque and kinetic-δW results move. Every kinetic regression case will need re-baselining; that is expected, not a regression.

Only the magnitude is addressed here. The overall sign of the precession is a separate convention question (it depends on whether P_φ = ±q·ψ_p, which cannot be read off from the native chart) and is deliberately not touched.

Validation

  • runtests_kinetic.jl: 277/277. Not sufficient alone — those unit tests do not exercise the pitch-integration / compute_bounce_data paths.
  • Regression harness: pending. Draft until the kinetic cases are run and the new baselines are recorded with the shift explained.

🤖 Generated with Claude Code

…ga_D prefactor
The general-aspect-ratio precession prefactor carried ro^2 where it should
carry ro. wbbar = ro*2pi/((2-sigma)*I1) already contains one factor of ro that
its own normalization bhat = sqrt(2T/m)/ro cancels; reusing wbbar inside wdbar
imports that ro a third time, while dhat = (T/q)/(bo*ro^2) removes only the two
written explicitly.
Dimensionally, both bounce integrals carry the J*b*dtheta = dl length, so I1 is
a length and I2/I1 is 1/Wb. With T/q in volts and V/Wb = 1/s, the prefactor
4*pi*wdfac*(I2/I1)*(T/q) is already a frequency, and the surviving ro left
omega_D in m/s. Verified by holding the physics fixed and varying only the
machine size: the old form scales as ro (ratio 2.0 when ro is doubled), the
corrected form is ro-invariant (ratio 1.0). omega_b, built from the same I1, is
untouched and keeps its correct v_th/L scaling.
Julia analog of Fortran GPEC PR #281, which measured the same correction
against an independently validated guiding-centre precession operator: the
least-squares slope of omega_D against the reference moves from -6.49 (= -ro on
that ITER equilibrium) to -1.02, with omega_b agreeing to within [0.9964,
1.0011] both before and after as the control.
This changes omega_D by a factor of ro on every machine, so it moves the
resonance denominator and all NTV torque. Only the magnitude is addressed here;
the overall sign convention is a separate question and is not touched.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@logan-nc
logan-nc marked this pull request as ready for review August 15, 2026 18:33
@logan-nclogan-nc self-assigned this Aug 15, 2026
@logan-nc

Copy link
Copy Markdown
CollaboratorAuthor

Ready for review; not yet ready to merge.

The physics/code argument is complete and self-contained, so review can start now. The regression-harness table is still outstanding and is a merge gate per CLAUDE.md — I'll post it here before this is merged.

Note the machine currently has other regression runs in flight, so the kinetic cases are queued behind the PR #339 re-baseline rather than run in parallel.

@logan-nc

Copy link
Copy Markdown
CollaboratorAuthor

Regression harness — develop (b3abe074) vs this branch

regress --cases diiid_n1,solovev_kinetic_ntv,solovev_kinetic_nuzero,solovev_kinetic_calculated --refs develop,bugfix/kf-omega-d-major-radius

CaseR₀Result
diiid_n11.743 m2 changed (both NTV), 46 unchanged
solovev_kinetic_ntv1.0 m6 unchanged
solovev_kinetic_nuzero1.0 m5 changed, all ~1e-7 (FP noise)
solovev_kinetic_calculated1.0 m6 changed, all ~1e-7 (FP noise)

The only physically meaningful movement in the whole suite:

diiid_n1 develop branch Diff
NTV torque FGAR [N·m] 5.591200e-01 5.336824e-01 2.544e-02 (4.55%)
NTV kinetic energy dW FGAR [J] 6.909782e-02 7.997363e-02 1.088e-02 (15.74%)

All 46 other diiid_n1 quantities — energies, q profile, singular surfaces, Δ′, island widths, ODE steps — are exactly unchanged, which is the containment check: this patch moves omega_D and nothing else. The response is nonlinear in omega_D (it enters the resonance denominator ℓ_eff·ω_b·√x + n·(ω_E + ω_D·x)), so a 1.74× change in the frequency gives a few-percent shift in the integrated torque rather than a proportional one.

⚠️ The regression suite is nearly blind to this bug

The Solovev fixtures set r0 = 1.0 exactly (examples/Solovev_kinetic_calculated_example/gpec.toml:73). With ro = 1, ro² ≡ ro, so this fix is an algebraic no-op on every Solovev case — the ~1e-7 movements above are floating-point reassociation from writing ro * instead of ro^2 *, nothing more.

That means:

  • diiid_n1 (R₀ = 1.743 m) is the only tracked case that exercises this at all, and only through its two NTV quantities.
  • A unit-magnitude major radius in the primary kinetic fixtures hides this entire class of dimensional error. Upstream needed an ITER case (R₀ = 6.411 m) to see it as a 6.4× discrepancy; at R₀ = 1.74 m it is a 1.74× error in omega_D, and at R₀ = 1.0 m it is invisible.

Worth considering as a follow-up: either a Solovev kinetic variant with r0 ≠ 1 (cheap — the analytic equilibrium takes r0 directly), or a note in the case TOMLs that r0 = 1 makes them insensitive to major-radius scaling errors. I have not changed the fixtures in this PR, since re-baselining them here would obscure the containment result above.

Validation summary

  • runtests_kinetic.jl 277/277.
  • Dimensional argument verified independently of upstream: holding physics fixed and doubling ro, omega_D scales as 2.0 before the fix (i.e. carries a length) and 1.0 after; omega_b, built from the same bounce integral, keeps its correct v_th/L scaling as the control.

@logan-nclogan-nc changed the title KineticForces - BUG FIX - drop the spurious major radius from the omega_D prefactorKineticForces - BUGFIX! - Drop the spurious major radius from the omega_D prefactorAug 18, 2026
@github-actionsgithub-actionsBot added bugfix Something was wrong and now is not changed-results Results move or an interface breaks - read before upgrading labels Aug 18, 2026
@logan-nc

Copy link
Copy Markdown
CollaboratorAuthor

Regression re-run after re-syncing with develop

PR #339 merged, so this branch was updated to develop@c42d558e and the harness re-run at the new head. Baseline and branch now both contain the multi-ion work.

diiid_n1	Summary: 2 changed, 45 unchanged
solovev_kinetic_ntv	Summary: 6 unchanged
solovev_kinetic_nuzero	Summary: 6 changed, 8 unchanged
solovev_kinetic_calculated	Summary: 7 changed, 7 unchanged

Every changed row on the two Solovev kinetic cases is at 0.00% relative (absolute diffs ~1e-6 to 1e-8 on O(1) eigenvalues, plus one-step ODE count moves) — they cross the harness's tight 1e-10 eigenvalue threshold but are not physically meaningful.

The one substantive movement is on diiid_n1, and it reproduces the earlier measurement almost exactly even though the baseline moved 56+ commits:

NTV torque FGAR [N·m] 5.549513e-01 -> 5.296763e-01 (4.55%)
NTV kinetic energy dW FGAR [J] 6.840635e-02 -> 7.924971e-02 (15.85%)

The other 45 diiid_n1 quantities are exactly unchanged, which is the containment check: this patch moves omega_D and nothing else.

jhalpern30 pushed a commit that referenced this pull request Aug 20, 2026
The metadata job read only requested_reviewers, which holds pending review
requests. GitHub removes a reviewer from that list as soon as they submit a
review, so the check went quiet while nobody had reviewed and fired once
somebody did -- nagging precisely the pull requests furthest along.
Treat a submitted review from anyone other than the author as evidence that a
reviewer was named. Assignment alone still suffices; the reviewer never has to
act. Verified against all open pull requests: silences #390, #391, #392, #407,
leaves every pending-request PR untouched, and still nags the nine with nobody
named.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Dh1NCejnd3fYMmcRKoQRcG
@logan-nc

Copy link
Copy Markdown
CollaboratorAuthor

Note on the harness stamp. The branch was updated to develop@4e430f170 after the regression run, so the _(harness @ …)_ stamp now predates the head and the conventions check will flag it as stale.

That delta is #424, and it cannot change any tracked quantity. Its entire src/ footprint is three PerturbedEquilibrium files, and the only non-comment line in the whole diff is an HDF5 long_nameattribute string:

- "Energies/toroidal_torque" => (; long_name="net toroidal torque on the plasma", units="N*m")
+ "Energies/toroidal_torque" => (;
+ long_name="boundary-response toroidal torque ...", units="N*m")

Everything else is docstrings and comments. Re-running the harness would reproduce the posted table exactly, so the numbers above stand. Title and release note is not a required check, so this does not block the merge; Documentation and Tests are the gates and are running on the updated head.

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

Labels

bugfixSomething was wrong and now is notchanged-resultsResults move or an interface breaks - read before upgrading

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@logan-nc@krystophny