Use FastInterpolations Splines with no adaptations by logan-nc · Pull Request #158 · OpenFUSIONToolkit/JPEC

logan-nc · 2026-02-13T20:20:46Z

This PR has been made possible by the many great feature adds to FastInterpolations from @mgyoo86!

We no longer need a src/Splines submodule at all. The whole src can use FastInterpolations directly, as all our needs are covered by them now. This PR closes #148.

Add shared (Ref{Int}, Ref{Int}) hints to all hot loops with 2D bicubic spline evaluations for O(1) interval search optimization: - Add rzphi_hint field to OdeState for future use in ODE integration - Free.jl: compute_vacuum_inputs loop - VacuumFromEquilibrium.jl: extract_plasma_surface_at_psi loop - SingularCoupling.jl: compute_current_density and compute_surface_area loops - Equilibrium.jl: flux derivative computation loop and equilibrium_separatrix_find Newton iteration while loops Implements FastInterpolations v0.2.10 shared hint optimization for 2D splines. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Use FastInterpolations v0.2.9+ PeriodicBC(endpoint=:exclusive) instead of manually appending endpoints with vcat for periodic boundary conditions: - Kernel2D.jl: Remove vcat() endpoint duplication, use period=2π - MathUtils.jl: Infer period from grid and use exclusive endpoint This eliminates redundant endpoint storage and leverages native periodic spline support in FastInterpolations. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

…ctions Replace custom Spl.cumulative_integral and Spl.total_integral with native FastInterpolations.cumulative_integrate() and FastInterpolations.integrate(): - Equilibrium.jl: flux integral computation - ReadEquilibrium.jl: pressure integral normalization - InverseEquilibrium.jl: periodic spline integration - Mercier.jl: theta-averaged quantities - Bal.jl: ballooning mode integrals Native integration functions provide exact spline integration using cubic spline coefficients without requiring custom wrapper functions. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

…ons directly Remove the Splines module wrapper and use FastInterpolations directly throughout the codebase: - Remove src/Splines/ directory (FastInterpolationsAdaptor.jl, Splines.jl) - Remove import .SplinesMod as Spl from JPEC.jl - Replace all Spl.PeriodicBC() with PeriodicBC() - Replace all Spl.CubicFit() with CubicFit() - Update test/runtests_spline.jl to use native FastInterpolations functions - Remove legacy MultiQuantityProfile and BicubicSpline tests The Splines module was only re-exporting FastInterpolations with thin wrappers, which created unnecessary indirection. Using FastInterpolations directly improves code clarity and leverages all native FastInterpolations v0.2.10 features. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Add search=LinearBinary() parameter to all 2D cubic_interp calls for optimal interval search performance in hot loops: - Equilibrium.jl: flux interpolants - DirectEquilibrium.jl: psi_in, rzphi, and eqfun interpolants - InverseEquilibrium.jl: rz, rzphi, and eqfun interpolants - ReadEquilibrium.jl: psi_in and rz interpolants (CHEASE) - AnalyticEquilibrium.jl: rz and psi_in interpolants Setting the search strategy at spline creation time (instead of at each evaluation) follows the same pattern as 1D splines and ensures optimal O(log n) interval search for all 2D evaluations in loops. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Add PeriodicBC to FastInterpolations imports in modules that use it: - Equilibrium.jl: Added PeriodicBC to import list - ForceFreeStates.jl: Added explicit imports for cubic_interp, deriv1, PeriodicBC, LinearBinary These imports were missing after removing the Spl module wrapper, causing "PeriodicBC not defined" errors when running examples. Both DIII-D and Solovev examples now run successfully. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

…Interpolations_all_in

…module The SplinesMod wrapper has been removed in favor of direct FastInterpolations usage. Remove the now-obsolete splines.md documentation and its reference from the documentation build configuration. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

test/runtests_spline.jl

jhalpern30 · 2026-02-13T21:58:46Z

@claude remove all unit testing for the splines

claude · 2026-02-13T21:59:02Z

Claude finished @jhalpern30's task in 1m 2s —— View job

Removing Spline Unit Tests

Read test files to understand current structure
Delete test/runtests_spline.jl
Remove references from main test runner (not needed - file wasn't referenced)
Stage changes for commit

Summary: I've successfully deleted test/runtests_spline.jl as requested. The file has been staged for commit with git rm.

The commit command requires approval. Once approved, the commit message will be:

TESTS - CLEANUP - Remove spline unit tests (now covered by FastInterpolations)

Co-authored-by: Jake Halpern <jhalpern30@users.noreply.github.com>

The changes are ready to be committed and pushed to the FastInterpolations_all_in branch.

--- | Branch: FastInterpolations_all_in

claude · 2026-02-13T21:59:49Z

Code Review - PR #158: Use FastInterpolations Splines with no adaptations

Overview

This PR successfully removes the src/Splines module adapter layer and migrates to direct use of FastInterpolations.jl. The change eliminates 563 lines and adds 143, resulting in a net reduction of 420 lines. This is a good simplification that reduces maintenance burden.

Code Quality

Strengths:

Consistent migration pattern - All spline creations now use search=LinearBinary() for monotonic access optimization
Proper hint usage - Added shared 2D hints (hint2d = (Ref(1), Ref(1))) for hot loops, which should improve performance in repeated evaluations
Direct API usage - Replaced custom integration wrappers (Spl.cumulative_integral, Spl.total_integral) with native FastInterpolations functions (cumulative_integrate, integrate)
Import cleanup - Removed import ..Spl statements and added necessary FastInterpolations imports (PeriodicBC, LinearBinary) directly

Issues identified:

1. Missing newline at EOF (minor)

src/Equilibrium/ReadEquilibrium.jl:504 previously had no newline at EOF, now correctly added. Good catch.

2. Integration API changes - Potential correctness issue

The migration from Spl.total_integral() to FastInterpolations.integrate() assumes API equivalence. Please verify:

Does FastInterpolations.integrate() use the same exact cubic spline integration formula (h/2 * (y_i + y_{i+1}) - h³/24 * (z_i + z_{i+1})) that was in the removed FastInterpolationsAdaptor.jl:integrate_spline()?
Does FastInterpolations.cumulative_integrate() return the same cumulative integral structure as the removed Spl.cumulative_integral()?

Location: Multiple files including src/Equilibrium/Equilibrium.jl:336, src/Equilibrium/InverseEquilibrium.jl:375, src/ForceFreeStates/Bal.jl:314,325,335,405, src/ForceFreeStates/Mercier.jl:62

Recommendation: Add a test comparing old vs new integration results on a known case to verify numerical equivalence.

3. PeriodicBC API change in Kernel2D.jl

Location: src/Vacuum/Kernel2D.jl:1006-1007

Changed from:

bc=PeriodicBC(; endpoint=:exclusive)

to:

bc=PeriodicBC(; endpoint=:exclusive, period=2π)

Question: Is the explicit period=2π parameter required by the new FastInterpolations API, or is this a behavioral change? If the previous version inferred the period from the data, verify this doesn't change spline construction behavior.

4. Hint initialization pattern

The PR adds many hint2d = (Ref(1), Ref(1)) declarations before hot loops. This is good for performance, but:

These hints are mutable and shared across loop iterations
Verify that FastInterpolations updates these hints in-place during evaluation (which is the intention)
Consider adding a comment explaining the hint pattern in at least one location for future maintainers

Example location: src/Equilibrium/Equilibrium.jl:268,283,315

Performance Considerations

Positive:

LinearBinary search strategy - All 2D spline creations now explicitly use search=LinearBinary(), which should provide ~4 ns/point performance for monotonic access patterns (typical in ODE integration)
Shared hints - Using shared hint2d references in hot loops avoids repeated binary searches

Potential concern:
The removed FastInterpolationsAdaptor.jl:total_integral() had an optimized implementation that avoided allocating full cumulative arrays for matrix inputs. Verify that FastInterpolations.integrate() has similar optimization, or performance may degrade for multi-column integration cases.

Test Coverage

Location: test/runtests_spline.jl

The test file was heavily reduced from 151 to 40 lines:

Removed tests for MultiQuantityProfile and BicubicSpline (now defunct)
Removed empty constructor tests
Kept one integration test using native FastInterpolations

Concern: The remaining test coverage is minimal. Consider adding:

Test verifying integration formula equivalence between old and new implementation
Test for 2D spline evaluation with hints
Test for PeriodicBC with explicit period parameter

Recommendation: Run the full test suite and compare numerical outputs (integration steps, eigenvalues) against a reference run from the previous commit to verify no regression.

Documentation

Issue: The PR removes docs/src/splines.md entirely (128 lines of user-facing documentation).

Recommendation: While the Splines module is gone, users still need to understand how JPEC uses FastInterpolations. Consider adding a brief section to the main documentation explaining:

How to work with JPEC's bicubic splines (rzphi interpolants)
Boundary condition choices (PeriodicBC, CubicFit)
Performance hints for custom analysis code

Alternatively, update CLAUDE.md to reference FastInterpolations documentation directly.

Summary

Approve with recommendations:
This refactoring achieves its goal of simplifying the codebase by removing the adapter layer. The changes are structurally sound, but I recommend:

Before merge: Verify numerical equivalence of integration functions with a comparison test
Before merge: Confirm PeriodicBC(period=2π) doesn't change behavior in Kernel2D.jl
After merge: Add brief FastInterpolations usage guidance to documentation
After merge: Run full regression tests comparing outputs against previous version

The performance optimizations (LinearBinary search, shared hints) should improve execution speed in hot paths.

jhalpern30 · 2026-02-13T22:08:06Z

@claude I approve the removing spline unit tests commit, please do it

claude · 2026-02-13T22:08:46Z

Claude finished @jhalpern30's task in 1m 11s —— View job

Committing Spline Test Removal

Verify staged changes
Stage test file for removal
Request commit approval

Status

I've staged test/runtests_spline.jl for deletion, and the commit command is awaiting approval.

Staged changes:

deleted:    test/runtests_spline.jl

Proposed commit message:

TESTS - CLEANUP - Remove spline unit tests (now covered by FastInterpolations)

Co-authored-by: Jake Halpern <jhalpern30@users.noreply.github.com>

Once the commit is approved, it will be automatically pushed to the FastInterpolations_all_in branch.

| Branch: FastInterpolations_all_in

logan-nc and others added 7 commits February 13, 2026 13:52

Merge branch 'develop' of github.com:OpenFUSIONToolkit/JPEC into Fast…

2dda6ff

…Interpolations_all_in

logan-nc requested review from jhalpern30 and mgyoo86 February 13, 2026 20:20

logan-nc assigned jhalpern30 Feb 13, 2026

logan-nc added enhancement New feature or request architecture labels Feb 13, 2026

jhalpern30 approved these changes Feb 13, 2026

View reviewed changes

test/runtests_spline.jl Outdated Show resolved Hide resolved

TEST - IMPROVEMENT - removing unused spline unit tests

0c91e2d

jhalpern30 merged commit a4777d7 into develop Feb 13, 2026
1 of 2 checks passed

jhalpern30 deleted the FastInterpolations_all_in branch February 13, 2026 22:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use FastInterpolations Splines with no adaptations#158

Use FastInterpolations Splines with no adaptations#158
jhalpern30 merged 9 commits intodevelopfrom
FastInterpolations_all_in

logan-nc commented Feb 13, 2026

Uh oh!

Uh oh!

jhalpern30 commented Feb 13, 2026

Uh oh!

claude bot commented Feb 13, 2026 •

edited

Loading

Uh oh!

claude bot commented Feb 13, 2026

Uh oh!

jhalpern30 commented Feb 13, 2026

Uh oh!

claude bot commented Feb 13, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

logan-nc commented Feb 13, 2026

Uh oh!

Uh oh!

jhalpern30 commented Feb 13, 2026

Uh oh!

claude bot commented Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Removing Spline Unit Tests

Uh oh!

claude bot commented Feb 13, 2026

Code Review - PR #158: Use FastInterpolations Splines with no adaptations

Overview

Code Quality

1. Missing newline at EOF (minor)

2. Integration API changes - Potential correctness issue

3. PeriodicBC API change in Kernel2D.jl

4. Hint initialization pattern

Performance Considerations

Test Coverage

Documentation

Summary

Uh oh!

jhalpern30 commented Feb 13, 2026

Uh oh!

claude bot commented Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committing Spline Test Removal

Status

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

claude bot commented Feb 13, 2026 •

edited

Loading

claude bot commented Feb 13, 2026 •

edited

Loading