Repository navigation
Vacuum/ForceFreeStates - BUGFIX! - Put the vacuum and norm matrices in the plasma's Fourier frame - #503
Vacuum/ForceFreeStates - BUGFIX! - Put the vacuum and norm matrices in the plasma's Fourier frame#503jhalpern30 wants to merge 4 commits into
Conversation
…n the plasma's Fourier frame
The Vacuum module reversed the plasma contour into VACUUM's clockwise θ, which puts every harmonic in the
complex-conjugate frame. Only I_v was conjugated back, so wv (and the ForceFreeStates norm matrix N) sat in
the opposite frame to wp and L, and δW, Λ and P depended on the θ origin of up-down asymmetric equilibria.
- Vacuum: run natively in GPEC's counter-clockwise θ; the 2D basis takes each point's ζ (= -ν on ϕ = 0);
revolved 3D points sit at ϕ = ζ + ν; kernel normal flip moves to wall sources; analytic and conformal
walls follow the new direction; clockwise plasma contours (2D or 3D) are rejected and clockwise wall
files reversed with a warning; the I_v conjugation is removed.
- ForceFreeStates: N[m, m'] = J_{m-m'} as in Fortran free.f (was its transpose).
- Tests: θ-origin shift, clockwise inputs, 2D/3D agreement for nonzero ν, N as the ∮J|ξ|² quadratic form.
github-actions
Bot
added
bugfix
Up-down symmetric control: the fix leaves it unchanged
Run on Feynman with the forward integrator (the deck's Galerkin integrator skips the free-boundary stage), n = 1, develop
For contrast, the Solov'ev harness cases moved up to 0.35 % because that example's traced boundary is up-down asymmetric by 2.6 mm at its Reproduce: |
|
@logan-nc this is the Fortran + Julia bugfix I mentioned. I wanted your review on this because I see the best way forward to avoid confusion in the future to be finally making VACUUM use the same CCW theta convention as DCON/GPEC, which is what this PR does (I plan to do something that's a bit easier in the Fortran). I remember, however, that when I mentioned this at one point, you flagged that it might mess with something upstream - potentially how DCON chooses the helicity to make the resonant mode positive (I might be remembering completely incorrectly)? But I haven't been able to reproduce any issue and every test I can throw at it seems numerically sound. Let me know if anything comes to mind that I haven't thought of |
|
@jhalpern30 just a shot in the dark but have you tried running with non-hamada coordinate systems and seeing if that affects anything? |
Yep, this fix is for a general coordinate system 
|
|
@claude review this pr |
|
Review MERGE GATE: THIS PR MUST NOT BE MERGED WITHOUT APPROVAL FROM A HUMAN REVIEWER. THIS IS REQUIRED AND NON-NEGOTIABLE. A reviewer (logan-nc) and an assignee are set, which is good. This change moves δW, Λ, P, Δ′ and NTV, so it needs a physics reviewer. A review by a bot does not satisfy this requirement. Removing the VACUUM clockwise-θ frame makes the vacuum and plasma matrices share one Fourier frame. The removed pieces are the input Code quality / possible issues
Tests
Regression harness
🤖 Generated with Claude Code |
logan-nc
left a comment
logan-nc
left a comment
There was a problem hiding this comment.
Thanks @jhalpern30. I definitely agree with the strategy of removing this tripwire to make the src self consistent throughout.
Should we take it all the way and use turns instead of radians?
|
@jhalpern30 none of the claude points look particularly good/important to me. I will approve after you confirm/deny my suggestions above. |
Co-authored-by: Nikolas Logan <nikolas.logan@columbia.edu>
Oooooh this could be a very satisfying fix so I am tempted. I will sketch out what it would look like today, but I don't think it would be too difficult. Could be nice to make everything officially use the same angle convention for the first time ever and remove some of the difficult unit-tracking that has taken me months to pin down |
|
From Claude: TL;DR - going to wait on the radians to turns convention change What it would involve.Vacuum isn't the only place using radians. The FourierTransform / compute_fourier_coefficients utilities also use a radian θ grid and radian ζ. So does the Fourfit metric (metric.ys .= rzphi_ys .* 2π, plus the 2π endpoint check in FourierCoefficients). And rzphi_nu is in radians while rzphi_offset is in turns. Making the repo consistent has three levels:
Why not in this PR.This PR is a BUGFIX! that moves results (δW, Λ, P, Δ′, NTV by 0.5–6 %). The unit change should move nothing, and it's much easier to review as "regression harness shows 0 changed vs the merged head". Mixed into this PR, that check is lost. To get the goal of "standardized use of angle convention repo-wide" would be much wider than just a vacuum change, so not worth it for this PR. |
…a-frame Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # test/runtests_vacuum.jl
|
@logan-nc this is all set for your final approval |
This branch has not been deployed
3 participants
Release note
VacuumInputcontours, and the ζ = 0 cross-section of 3D boundaries, must now run counter-clockwise in (R, Z) — clockwise input throws anArgumentError. Clockwise wall files still work and are reversed with a warning.FourierTransform(...; ν=)is nowζ=and takes each point's toroidal angle (pass-νfor a contour on the ϕ = 0 plane).SurfaceGeometriespoints ingpec.h5are now stored counter-clockwise.The free-boundary energy δW, the plasma inductance Λ and the permeability P changed when the poloidal-angle origin of an up-down asymmetric equilibrium was moved. They no longer do: the vacuum response and the edge norm are now in the same Fourier frame as the plasma matrices. The same defect is in Fortran DCON.
Regression report
Develop was merged into the PR (develop
6fc89d021, head94cdf5f39) after the full 16-case run.diiid_n1,diiid_n1_riccatiandsolovev_n1were rerun on Landau at the merged head (develop6fc89d021vs PR head94cdf5f39, run2026-10-09_pr503_merge, julia 1.12.7, pinned manifest). The other 13 cases were not rerun: their rows and reports are from the earlier run on Feynman, develop0e68a0553vs60af2dda5(runs2026-10-05_pr503_mainand_errfield). In the three reruns the changed/unchanged counts and the size of the PR's moves match the earlier run (21/32, 6/11, 8/14). The develop baseline itself moved between the two stamps through other develop PRs (DIII-Det+0.03 %, Solov'evet0.596 → 0.513,PE toroidal torque5.1e‑2 → 1.2e‑4), and the PR head moved with it.The DIII‑D
ev/ep"(all)" vectors move up to 35 % because the eigenvectors of W_t redistribute energy between plasma and vacuum once W_v changes; the eigenvalues themselves (et) move ≤ 0.04 %. The Solov'ev moves match its known up-down asymmetry atetol = 1e-7.Full reports
Reports for the 13 cases not rerun after the develop merge are from develop
0e68a0553vs60af2dda5;diiid_n1,diiid_n1_riccatiandsolovev_n1are from develop6fc89d021vs94cdf5f39.Notes for reviewers
Review page (figures, evidence, derivations): https://claude.ai/artifact/QArXT5chN3CVwujaUUQrbW
The bug. The Vacuum (VAC) module reversed the plasma contour into VACUUM's clockwise θ. Reversing θ (with ζ) complex-conjugates every harmonic, so the vacuum operators came back in the conjugate frame. Only
I_vwas conjugated back;wvwas not. Separately, the ForceFreeStates (FFS) normNcopied Fortran'sjmat(jpert−ipert), which is the transpose of ∮J|ξ|² and putsNin the same wrong frame relative toL. Under a θ-origin shift bya,wp,LandI_vtransform asD·A·D†whilewvandNtransformed asD†·A·D(D = diag(e^{ima})), sowp + wvwas not a congruence of itself and the eigenvalues moved.How we know it is a bug. On an up-down asymmetric case δW is 0.1995 / 0.2176 / 0.3593 at origins 0, 1/16, 1/4 turn (JPEC and Fortran DCON alike); an up-down symmetric D shape rolled by a quarter turn gives 0.4291 instead of 0.2613. Moving θ = 0 is a relabeling, so neither may happen.
The fix. VAC now runs in GPEC's counter-clockwise θ with no reversal and no frame conversion:
rzphi_nu); revolved 3D points sit at ϕ = ζ + νconj!(I_v)is removed (2D), and the 3D partner conjugation keeps only the pairing partN[m, m′] = J_{m−m′}, the ∮J|ξ|² form (Fortranfree.fhas the transpose too)How we know it works. δW = 0.1986531 at every origin (PEST), 0.1986784 (Hamada); the rolled symmetric case returns 0.2613307; all nine matrices fit
D·A·D†after the fix. The Fortran counterpart is PrincetonUniversity/GPEC#295: it keeps VACUUM's reversed θ and conjugateswvafterwards, and transposes the norm. It is origin-independent too and agrees with this PR to the codes' existing 0.4 % offset. New tests pin the frame (θ-origin shift), the ν sign (2D vs 3D with ν ≠ 0), clockwise inputs, andNas the ∮J|ξ|² quadratic form; each fails on the old code. Fourier 35/35, vacuum 393/393, coordinate-invariance 27/27.Not covered. n < 0, the edge-scan path, and the frame of the coil forcing Φx that multiplies P. Nothing pins the conformal wall staying outside the plasma.
Before this leaves draft. Name an assignee and reviewers.