Skip to content

(compat): Upgrade FastInterpolations to v0.4 and AdaptiveArrayPools to v0.3#207

Merged
jhalpern30 merged 9 commits into
developfrom
refac/fast_interp_v0.4
Apr 2, 2026
Merged

(compat): Upgrade FastInterpolations to v0.4 and AdaptiveArrayPools to v0.3#207
jhalpern30 merged 9 commits into
developfrom
refac/fast_interp_v0.4

Conversation

@mgyoo86
Copy link
Copy Markdown
Collaborator

@mgyoo86 mgyoo86 commented Mar 31, 2026

Summary

Migrate all interpolation call sites across the codebase to the FastInterpolations v0.4 API and bump AdaptiveArrayPools to v0.3. This is a mechanical but codebase-wide change (22 files, all modules).

FastInterpolations v0.4

Full migration guides:
📖 v0.2->v0.3
📖 v0.3->v0.4

Key changes reflected in this PR:

  • Series() wrapper required for multi-column (series) interpolants:

    # Before
    cubic_interp(xs, matrix; bc=CubicFit())
    # After
    cubic_interp(xs, Series(matrix))
  • Saner defaults — less boilerplate: CubicFit() is now the default BC and AutoSearch() is the default search policy (automatically selects the optimal algorithm internally). Explicit bc=CubicFit() and search=LinearBinarySearch() are no longer needed:

    # Before
    cubic_interp(xs, fs; bc=CubicFit(), search=LinearBinary(), extrap=ExtendExtrap())
    # After
    cubic_interp(xs, fs; extrap=ExtendExtrap())
  • Derivative dispatch: Val((1, 0)) replaced by DerivOp(1, 0) for ND derivatives; integer deriv=1 replaced by deriv=DerivOp(1) for series interpolants.

  • Renamed BC: NaturalBC()ZeroCurvBC().

  • Simplified imports: Selective import FastInterpolations: cubic_interp, deriv1, ... replaced with using FastInterpolations — all public symbols are now properly exported.

AdaptiveArrayPools v0.3

  • Reduced pool overhead and improved performance.
  • Significantly enhanced compile-time and runtime safety checks (escape detection, structural mutation guards, etc.). Further adoption of these new safety features will follow in a subsequent PR.

⚠️ Bug Fix: Pool-backed arrays escaping scope in Vacuum

While upgrading, I discovered a misuse of AdaptiveArrayPools in compute_vacuum_response: arrays allocated via zeros!(pool, ...) (wv, grri, grre, plasma_pts, wall_pts) were being returned to the caller, escaping their @with_pool scope. Once the scope ends, the pool may reclaim and reuse that memory, leading to silent data corruption. Fixed by replacing with standard zeros() allocations.

AdaptiveArrayPools v0.3 now includes compile-time escape detection that catches most of these patterns automatically at macro-expansion time. Enabling RUNTIME_CHECK mode for test suites and adopting the related safety skills will be addressed in a follow-up PR.

mgyoo86 added 2 commits March 31, 2026 11:41
Bump FastInterpolations compat to 0.4 and AdaptiveArrayPools to 0.3.
Update all interpolation call sites across the codebase:
- Val((x,y)) → DerivOp(x,y) for 2D derivatives
- deriv=N → deriv=DerivOp(N) for 1D derivatives
- Multi-column matrices → Series(matrix) for series interpolants
- Remove now-default search=LinearBinary() and bc=CubicFit()
- NaturalBC() → ZeroCurvBC()
- Simplify selective imports to plain 'using FastInterpolations'
… vacuum response

Pool-allocated arrays from zeros!(pool, ...) must not outlive the @with_pool
scope, but compute_vacuum_response returns these arrays to the caller.
Use standard zeros() instead to avoid use-after-return of pooled memory.
@mgyoo86 mgyoo86 requested review from jhalpern30 and logan-nc March 31, 2026 19:27
mgyoo86 and others added 4 commits March 31, 2026 15:05
…paratrix finder

The zsep loop in equilibrium_separatrix_find! used unbounded Newton
iteration to find dZ/dθ = 0 extrema. Newton diverged when d²Z/dθ²
passed through zero (inflection point), sending θ to values like ±1e9.
Due to sin/cos periodicity the diverged θ still produced plausible Z
values, silently returning wrong extrema depending on floating-point
luck. This caused kappa ≈ 0 or kappa < 0 for some equilibria.

Fix: use bracketed Brent method on dZ/dθ with θ ∈ [0.5,1.0] (bottom)
and θ ∈ [0.0,0.5] (top). This eliminates d²Z/dθ² entirely, is 4x
faster, and guarantees convergence to the correct extremum.

Adds regression tests for kappa with Solovev equilibria (e=1.6, e=1.0).
…force tests

Convert the rsep loop (outboard/inboard midplane finding) from unbounded
Newton to bracketed Brent for consistency with the zsep fix. Remove
unused mtheta and vector variables that were only needed for Newton
initial guesses.

Add rsep value-level checks (amean ≈ 0.33, rmean ≈ 1.0) and circular
cross-section rext symmetry test (rext[1] ≈ rext[2]).
EQUIL - BUG FIX - Fix kappa calculation in separatrix finder
@logan-nc logan-nc added enhancement New feature or request splines performance labels Apr 2, 2026
@logan-nc
Copy link
Copy Markdown
Collaborator

logan-nc commented Apr 2, 2026

@mgyoo86 I am out Th,F then traveling to KSTAR for experiments the next two weeks, so forgive me if I am slow to review this - it is not for lack of appreciation!!

@jhalpern30 please help me out with reviewing this

@logan-nc
Copy link
Copy Markdown
Collaborator

logan-nc commented Apr 2, 2026

@claude review

@claude
Copy link
Copy Markdown

claude Bot commented Apr 2, 2026

Code review posted — see below for details.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 2, 2026

Placeholder — deleting this, real review below.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 2, 2026

Code Review

Summary: This PR correctly migrates FastInterpolations v0.4 and AdaptiveArrayPools v0.3, and fixes a genuine memory-safety bug in Vacuum.jl. However, several Series() wrappers for multi-column interpolants were missed, which will cause runtime errors on those code paths.

Bug 1 - InverseEquilibrium.jl (~line 304, ffac-normalization branch)
The two other sq constructions in the function use Series(sq_fs), but this one was missed: cubic_interp(sq_xs, sq_fs; extrap=ExtendExtrap()) needs Series(sq_fs).

Bug 2 - ForceFreeStates/Free.jl (~line 158)
wv_flat is a matrix: cubic_interp(psi_array, wv_flat; extrap=ExtendExtrap()) needs Series(wv_flat).

Bug 3 - ForceFreeStates/Bal.jl (~lines 292, 372)
Both hcat(1 ./ bsq, jac .* b1 ./ bsq) and spl1_fs are matrices passed without Series(). The deriv1() calls on these will also fail. These code paths have no test coverage so the failures will only appear at runtime.

Separatrix refactor: hardcoded brackets
Brent avoids Newton divergence but requires a valid bracket. The brackets are hardcoded (outboard midplane at theta near 0, inboard near 0.5, etc.) and Brent will throw an error rather than gracefully failing if the root is outside the bracket — e.g. for a highly triangular equilibrium. A descriptive error message wrapping find_zero would help diagnose this. The midplane find_zero calls also lack atol/rtol that the Z-extremum calls pass — these should be consistent.

Positive notes

  • Pool-escape fix in Vacuum.jl is correct and important.
  • @views additions and itp_2d_opts deduplication are good.
  • The new separatrix/kappa tests are a useful addition.

Copy link
Copy Markdown
Collaborator

@jhalpern30 jhalpern30 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgyoo86 looks good, thanks for doing this! Let me know what you think of Claude's review comments regarding the missing Series and the separatrix Brent solve. Once those are resolved, I can merge.

mgyoo86 added 3 commits April 2, 2026 10:11
…rpolants in FastInterpolations v0.4

InverseEquilibrium.jl: sq_fs in ffac-normalization branch
Free.jl: wv_flat vacuum matrix interpolant
Bal.jl: bf_fs, spl1_fs, bg_fs, spl2_fs, spl3_fs ballooning coefficient matrices
…ive error messages for all separatrix find_zero calls
@mgyoo86
Copy link
Copy Markdown
Collaborator Author

mgyoo86 commented Apr 2, 2026

@jhalpern30
I've addressed the Claude's findings:

  • added the missing Series() wrappers for matrix interpolants in Bal.jl, Free.jl, and InverseEquilibrium.jl
  • improved the separatrix solver with consistent atol/rtol tolerances and descriptive error messages.

I didn't widen the bracket values, however, the current brackets already span a full half-period each, which should be sufficient and safe to find solutions.

Thanks!

@logan-nc have a safe trip! :)

@jhalpern30 jhalpern30 merged commit edff6e8 into develop Apr 2, 2026
5 checks passed
@jhalpern30 jhalpern30 deleted the refac/fast_interp_v0.4 branch April 2, 2026 17:29
logan-nc added a commit that referenced this pull request Apr 2, 2026
Incorporates:
- PR #207: FastInterpolations v0.4 API (Series() wrappers, ZeroCurvBC, remove explicit defaults)
- PR #208: Separatrix finder Brent method fix
- PR #198: Analysis module improvements (plot_eigenvalues, plot_delta_prime, plot_ffs_summary)
- AdaptiveArrayPools v0.3.5, FastInterpolations v0.4.7

Conflict resolved in src/Analysis/ForceFreeStates.jl: retained our
plot_edge_stability_scan alongside develop's new plot_eigenvalues,
plot_delta_prime, and plot_ffs_summary functions.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants