ForceFreeStates - BUGFIX! - Snap the periodic theta endpoint before integrating GGJ geometry - #439
Open
adrianaghiozzi wants to merge 2 commits into
Open
ForceFreeStates - BUGFIX! - Snap the periodic theta endpoint before integrating GGJ geometry#439adrianaghiozzi wants to merge 2 commits into
adrianaghiozzi wants to merge 2 commits into
Conversation
…ntegrating GGJ geometry
Contributor
|
This pull request is missing an assignee. If you are not ready to name them, mark this pull request as a draft. |
matt-pharr
requested changes
Sep 2, 2026
matt-pharr
left a comment
Collaborator
There was a problem hiding this comment.
@adrianaghiozzi Looks reasonable, but the comments are not really necessary, are they? The git blame will still point to this pr with reasoning. At most if you really think this needs comments, one curt line suffices
Collaborator
Author
|
@matt-pharr Verbose Claude comments strike again lol I just shortened them in cefa010 |
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
resist_geometry(ResistEval.jl) andresist_eval(Resist.jl) build the GGJtheta-integrand independently at each theta node, including both periodic
endpoints, then integrate with
cubic_interp(...; bc=PeriodicBC()). The twoendpoint evaluations can differ by machine epsilon, which a stricter
PeriodicBC(endpoint=:inclusive)validator now rejects outright, abortingforce-free-state prep on some equilibria (mine hit it via a DIII-D case
driven from TokToxStability).
What
Snap
ff[end, :] .= ff[1, :]right before eachcubic_interpcall, in bothForceFreeStates/Surfaces/ResistEval.jland the siblingForceFreeStates/Surfaces/Resist.jl(same GGJ-integrand pattern, same latentbug -- not yet observed in the wild but a spot check shows it will trip the
same way on the right input).
This mirrors the fix already in
Equilibrium.jl's GS-residual integrator:Context
endpoint closure at the data-construction site, not
check=false) wasapplied everywhere in PR FastInterpolations bump and fix #243 except
ForceFreeStates/Surfaces/.periodicity issue (KineticForces: preserve periodic vpar spline and enumerate bounce roots exactly #341, KineticForces - BUGFIX! - Fit the parallel-velocity spline periodically #390) -- that's a different spline (
tspl/vparin
Torque.jl/BounceAveraging.jl), not the GGJ geometry integrand here.Regression report
Also ran the existing
test/runtests_resist_eval.jlsuite: 63/63 passed.Notes for reviewers
No behavior change expected or observed -- this only prevents a false-positive
validator abort on machine-epsilon endpoint drift, matching the fix pattern
already established for #240.