Widen Optim compat; replace NLsolve; add RK4 field-line integrator - #306
Merged
Conversation
NLsolve is unmaintained and pinned NLSolversBase to v7, which blocked upgrading Optim to 2.x. Its only use was the implicit-midpoint root-find in trace_field_line, now solved with SimpleNonlinearSolve's SimpleTrustRegion (mirrors NLsolve's trust-region default). Relax Bx_By_Bz / Br_Bphi_Bz signatures from 'where T<:Real' (which coupled all args to one type) to per-arg '::Real', so the ForwardDiff Jacobian's mixed Float64/Dual arguments flow through. Add test/runtests_fields.jl characterization tests (constant-field straight line, rotation-field radius invariant, D3D snapshot) pinning trajectory behavior across the swap. Also widen Roots compat to '2, 3'.
trace_field_line now takes a method kwarg: :implicit_midpoint (default; the unchanged 2nd-order symplectic path solved via SimpleNonlinearSolve) or :rk4 (explicit 4th-order, no per-step nonlinear solve). Introduce an abstract FieldLineIntegrator with ImplicitMidpointState and a dedicated RK4State, sharing the angle/step bookkeeping via _advance!. RK4 is ~3.6x faster and ~3x lighter on allocations and avoids the per-step solve entirely; the implicit path is kept for exact flux-surface (symplectic) preservation. Widen Optim compat to '1, 2' (resolves to 2.2.1 now that NLsolve is gone). Add RK4 characterization tests (straight line, radius conservation, D3D snapshot) plus an unknown-method guard. Note: ImplicitMidpointState/RK4State carry the existing in-place mutation of start_point (struct aliases it); unchanged here.
Both integrators solve the same ODE dx/ds = B̂(x), so on a benign analytic field (unit rotation → circle) their trajectories must agree to within the lower-order method's truncation error (~1e-4 here).
Drop the RK4State / _advance! / _integrator / abstract-type scaffolding: :rk4 now dispatches to a self-contained _trace_rk4 (one loop, no state object, no per-step solve), and the implicit path moves to _trace_implicit_midpoint (still using ImplicitMidpointState for its residual/solver). Both helpers force specialization on the field/stop closures. Tried SVector arithmetic in the loop but it was ~2x slower with no allocation benefit, so kept plain Vector. A/B benchmark shows per-step allocations (~5 KiB) are dominated by the Interpolations.gradient evaluation inside vector_field (4 calls/step), not the RK4 arithmetic — so loop-level changes can't reduce them; that would need a non-allocating field evaluation (separate change). Speed matches the prior struct version (~1 ms/trace, ~5x faster than implicit).
Extract the shared _toroidal_angle helper used by both integrators (clamping the acos argument against round-off), and copy start_point in the implicit path so it no longer mutates the caller's array, matching _trace_rk4.
Compare both integrators against the exact-circle solution: RK4 is far more accurate in position (4th- vs 2nd-order), while implicit midpoint conserves the radius invariant to ~machine precision. Includes a convergence-order check and a non-mutation regression test.
There was a problem hiding this comment.
Pull request overview
This PR updates IMAS’s field-line tracing implementation to remove the NLsolve dependency (unblocking a wider Optim compat range), and adds an explicit RK4 tracing option with new characterization/regression tests to pin solver behavior.
Changes:
- Replaced
NLsolveusage in the implicit-midpoint field-line integrator withSimpleNonlinearSolve. - Added an RK4 field-line integrator and exposed it via
trace_field_line(...; method=...). - Added a new
runtests_fields.jlsuite and wired it into the default test runner; updated dependency/compat bounds (Optim,Roots, solver deps).
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/physics/fields.jl |
Swaps nonlinear solver backend, adds RK4 tracing path, and centralizes toroidal-angle accounting. |
src/physics.jl |
Updates imports to reflect solver dependency change. |
Project.toml |
Removes NLsolve, adds SimpleNonlinearSolve, and widens compat bounds. |
test/runtests.jl |
Includes the new field-line test suite in the default run. |
test/runtests_fields.jl |
Adds characterization/regression/accuracy tests for both implicit-midpoint and RK4 tracing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Pins critical_energy(...; approximate=false) on the bundled sample — the only Roots.find_zero call site in the package, which no other test exercised. The value is bit-identical under Roots 2.3.0 and 3.0.0, locking in the widened Roots = "2, 3" compat.
This was referenced Jun 23, 2026
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.
Summary
The goal was to widen the
Optimcompat bound.NLsolvewas blocking it, so it is replaced withSimpleNonlinearSolvein the one routine that used it (theimplicit-midpoint field-line solver).In addition, an explicit RK4 integrator was added as a faster, more accurate option for field-line tracing.
What changed
Dependencies: widen
Optim(andRoots); dropNLsolve, addSimpleNonlinearSolve.Field-line tracing:
SimpleNonlinearSolve(same algorithm, slightly more accurate but a bit slower; this path is rarely used).
Tests / docs: added field-line tracing tests covering both methods and their
accuracy trade-off; fixed a couple of stale docstrings.