Compose step-size knobs into BasicController; replace DummyController for BDF/JVODE

[ChrisRackauckas-Claude]

Summary

In v7, qmin (alongside qmax, gamma, beta1/beta2, qsteady_*, qoldinit) moved off DEOptions and onto the controller object. The out-of-domain rejection path in handle_step_rejection! was still reaching for the old integrator.opts.qmin, which throws on the v7 DEOptions struct — only the legacy DelayDiffEq constructor still mentions it. The same was true of integrator.opts.failfactor in post_newton_controller!.

Reported by @ranocha in NumericalMathematics/PositiveIntegrators.jl#194 (comment).

Rather than papering over with a one-off hasfield walk, this lifts the standard step-size knobs into a composable building block and retires the DummyController workaround that BDF / Nordsieck were using to keep these knobs hard-wired on their algorithm structs.

BasicController + accessors

A new BasicController struct holds the seven scalars the integrator-level paths actually read:

• qmin / qmax (and qmax_first_step for the relaxed first-step bound),
• gamma (safety factor),
• qsteady_min / qsteady_max (deadband),
• failfactor (post-Newton-failure dt shrink).

All fields default to nothing; algorithm-specific defaults are filled in by resolve_basic at setup_controller_cache time. Concrete controllers (IController, PIController, PIDController, PredictiveController, ExtrapolationController, KantorovichTypeController, plus the new BDFController and JVODEController) all embed a BasicController as controller.basic.

Seven generic accessors — get_qmin, get_qmax, get_qmax_first_step, get_gamma, get_qsteady_min, get_qsteady_max, get_failfactor — dispatch on cache::AbstractControllerCache and read through cache.controller.basic. CompositeControllerCache overrides each one to delegate to the currently active sub-cache (mirroring how stepsize_controller! already dispatched). DummyControllerCache keeps an alg-field fallback for any SDE algorithm still using it.

handle_step_rejection! / post_newton_controller!

• integrator.opts.qmin → get_qmin(integrator)
• integrator.opts.failfactor → get_failfactor(integrator) (also in the BDF post-Newton paths in lib/OrdinaryDiffEqBDF/src/controllers.jl)

BDFController (replaces DummyController for QNDF / FBDF / DFBDF)

QNDF/FBDF/DFBDF used to keep qmax, qsteady_min, qsteady_max as fields on the algorithm struct itself, with a DummyController hard-wired into default_controller. The stepsize logic read alg.qmax / alg.qsteady_min / alg.qsteady_max directly, so the controller surface was unsettable.

BDFController embeds BasicController and has a cache that delegates back to alg-level dispatch (the existing BDF order-selection logic is left intact). default_controller(QT, alg::Union{QNDF, FBDF, DFBDF}) threads alg.qmax / alg.qsteady_min / alg.qsteady_max through to the controller, so existing usage like QNDF(qmax = 20) keeps working — but users can now also pass controller = BDFController(qmin = …, gamma = …) to set knobs that previously had no surface. BDF-tuned per-algorithm defaults (qmax = 5//1, qsteady_min = 9//10, qsteady_max = 12//10) are encoded as qmax_default(::QNDF) etc.

JVODEController (replaces DummyController for Nordsieck JVODE)

Same pattern. setη! / chooseη! / step_accept_controller!(::JVODE) now read get_qmin(integrator) / get_qmax(integrator) / get_qsteady_*(integrator) instead of alg.qmin etc.

Other refactors for consistency

• IController / PIController / PredictiveController / PIDController shed their flat qmin/qmax/... fields and embed BasicController instead. PI-specific knobs (beta1, beta2, qoldinit) and PID-specific knobs (beta, accept_safety, limiter) stay on the controller alongside basic.
• ExtrapolationController and KantorovichTypeController likewise embed BasicController so the seven accessors work uniformly.
• The PredictiveController docstring and two stale # equivalent to integrator.opts.gamma comments in lib/OrdinaryDiffEqBDF/src/controllers.jl were updated to reflect the new interface.

Verification

Reproducer (isoutofdomain predicate that fires once on the first proposed step) plus a smoke test covering every controller path:

using OrdinaryDiffEqTsit5, OrdinaryDiffEqVerner, OrdinaryDiffEqRosenbrock,
      OrdinaryDiffEqBDF, OrdinaryDiffEqCore, Test

@testset "BasicController smoke" begin
    bc = OrdinaryDiffEqCore.BasicController(; qmin = 1//5, qmax = 10)
    @test bc.qmin == 1//5 && bc.qmax == 10 && bc.gamma === nothing
end

@testset "controllers compose BasicController" begin
    @test OrdinaryDiffEqCore.IController().basic isa OrdinaryDiffEqCore.BasicController
    @test OrdinaryDiffEqCore.PIController(0.5, 0.5).basic isa OrdinaryDiffEqCore.BasicController
    @test OrdinaryDiffEqBDF.BDFController(; qmax = 7).basic.qmax == 7
end

@testset "default solve still works" begin
    f(u, p, t) = -u
    prob = ODEProblem(f, 1.0, (0.0, 1.0))
    for alg in (Tsit5(), Vern7(), Rosenbrock23(), FBDF(), QNDF())
        @test SciMLBase.successful_retcode(solve(prob, alg))
    end
end

@testset "isout rejection still works (the original bug)" begin
    f(u, p, t) = -0.5 * u
    prob = ODEProblem(f, 1.0, (0.0, 5.0))
    toggle = Ref(true)
    isout_pred = (u, p, t) -> begin
        if toggle[]; toggle[] = false; return true; end
        return false
    end
    for alg in (Tsit5(), Vern7(), Rosenbrock23(), FBDF(), QNDF())
        toggle[] = true
        @test SciMLBase.successful_retcode(solve(prob, alg; isoutofdomain = isout_pred, dt = 1.0))
    end
end

@testset "user-supplied BDFController is honored" begin
    sol = solve(ODEProblem((u,p,t)->-u, 1.0, (0.0, 1.0)), FBDF();
                controller = OrdinaryDiffEqBDF.BDFController(; qmax = 3))
    @test SciMLBase.successful_retcode(sol)
end

• On master (without this fix): all algorithms error out on isoutofdomain — accessing integrator.opts.qmin throws because the v7 DEOptions struct doesn't have the field.
• With this fix: 21/21 pass on Julia 1.12 in ~13s.

[oscardssmith]

The fact that this Pr adds ~200 loc and ~10 lines of tests suggests the new abstraction isn't carrying it's weight.

[ChrisRackauckas]

What is your suggestion? The one the AI came up with was pretty bad, it just didn't have a way to change qmin at all for things like BDF. This at least is uniform, fixes a few bugs that the new controller interface introduced, and better declares what can be expected from the interface.

[ChrisRackauckas]

I can't find that other PR on my phone right now that had the one claude came up with but review that, do you think that minimal change direction is actually better? I don't like magic numbers without knobs

[oscardssmith]

currious if @termi-official has thoughts here

[termi-official]

The design makes sense to me. The only alternative I can think off is to introduce a macro to inject the common variables directly into the controllers, but that seems to be the worse decision.

I left some comments on the code which we might want to discuss.

[termi-official]

Shouldn't this be pulled from the controller instead of hard-coded?

[ChrisRackauckas]

Yeah it should just be gamma.

[ChrisRackauckas-Claude]

Done — zₛ = 1.2 is now zₛ = get_gamma(integrator) in both BDF stepsize bodies (the QNDF accept-step path and the shared bdf_step_reject_controller!). Added gamma_default(::Union{QNDF, FBDF, DFBDF}) = 12 // 10 so a default-constructed BDFController() lands on the same numeric behavior the magic 1.2 used to have.

[termi-official]

I guess a better name would not suggest that this is a controller. Maybe we could name this CommonControllerOptions or similar?

Also, why should these all have fields different types?

[ChrisRackauckas-Claude]

Renamed → CommonControllerOptions. Also collapsed the per-field type parameters into a single T: fields are now Union{Nothing, T} (so unresolved options can hold nothings, and resolve_basic produces a CommonControllerOptions{Float64} with concrete fields). Adding a new knob in the future is now a one-line struct field addition rather than a typevar bump everywhere it's referenced.

[termi-official]

I think this is not really safe. If we add a field to BasicController in the future this isnt a concrete type anymore.

[ChrisRackauckas-Claude]

Fixed by the rename + single-T-param refactor in the same comment thread above: the cache type signature is now KantorovichTypeController{CommonControllerOptions{T}, T}, so adding a new knob to CommonControllerOptions doesn't change this signature at all (no more 7 typevars to keep in sync).

[termi-official]

I think we should use the new interface get_qmax over direct access. Same below.

[ChrisRackauckas-Claude]

Done. All controller.basic.qmax / controller.basic.qmin reads in the Extrapolation stepsize bodies are now get_qmax(integrator) / get_qmin(integrator). Same swap was applied to the BDF and JVODE stepsize bodies (which also read these knobs).

[ChrisRackauckas-Claude]

Force-pushed addressing the review:

• BasicController → CommonControllerOptions (per @termi-official). Single T type parameter instead of seven; fields are Union{Nothing, T} so unresolved options hold nothings and resolve_basic produces a CommonControllerOptions{Float64} with concrete fields. Adding a new knob is now a one-line struct addition rather than a typevar bump.
• BDF zₛ = 1.2 → zₛ = get_gamma(integrator) (per @termi-official + @ChrisRackauckas). The hard-coded magic number is now a real controller knob; gamma_default(::Union{QNDF, FBDF, DFBDF}) = 12 // 10 keeps the default behavior identical.
• KantorovichTypeControllerCache cache type signature (per @termi-official). With the single-T rework this is now KantorovichTypeController{CommonControllerOptions{T}, T} — no more list-of-Ts that breaks if a knob is added.
• Extrapolation accessors (per @termi-official). All controller.basic.qmax / .qmin reads in the Extrapolation stepsize bodies are now get_qmax(integrator) / get_qmin(integrator). Same swap in the BDF and JVODE bodies.
• Test orchestration UUID issue (the failing sublibrary CI). test/runtests.jl walks transitive [sources] deps and Pkg.develops them — pre-seeded the developed set with the active project so a [sources] entry that points back to it (e.g. via the umbrella OrdinaryDiffEq's transitive sources) is skipped.
• Runic formatting auto-applied to all touched files.

21/21 local smoke tests pass (default solve for Tsit5/Vern7/Rosenbrock23/FBDF/QNDF, isout-rejection reproducer, user-supplied BDFController(qmax = 3) end-to-end, CommonControllerOptions construction & composition invariants).

[ChrisRackauckas-Claude]

CI iteration:

• 224 pass / 1 fail / 2 skip — the only red is test (AD, lts) which also fails on master (run 25182052442) on a Rosenbrock ForwardDiff.Dual issue. The failure on this PR happens at a different test (PIDController gradient via Zygote/ReverseDiff), so the lts AD job is already a pre-existing red on master and not a regression here.
• All Sublibrary CI jobs that exercise the controller refactor are green: OrdinaryDiffEqBDF, OrdinaryDiffEqFIRK, OrdinaryDiffEqExtrapolation, OrdinaryDiffEqNordsieck, ImplicitDiscreteSolve, OrdinaryDiffEqSIMDRK (all _QA variants too), OrdinaryDiffEqCore, OrdinaryDiffEqDefault, OrdinaryDiffEqRosenbrock, OrdinaryDiffEqSDIRK, etc.
• All Interface*/Integrators*/AlgConvergence*/Regression*/Downstream/ModelingToolkit groups: green on Julia 1, 1.11, lts, pre.

Fixes that landed during this iteration:

• BDF stepsize-controller zₛ = 1.2 → get_gamma(integrator) (per @termi-official + @ChrisRackauckas review).
• Renamed BasicController → CommonControllerOptions with single T parameter (per @termi-official review).
• KantorovichTypeControllerCache cache type signature now closes over CommonControllerOptions{T} directly (per @termi-official review).
• Extrapolation/BDF/JVODE/FIRK stepsize bodies use get_qmax(integrator) / get_qmin(integrator) instead of controller.basic.qmax/qmin direct field access (per @termi-official review).
• Fixed leftover direct field accesses on outer controllers (cache.controller.qsteady_min for IControllerCache, the FIRK AdaptiveRadau PredictiveController step_accept body) and updated test/interface/controllers.jl to read ctrl.basic.qmax_first_step.
• Bumped OrdinaryDiffEqCore 4.0.2 → 4.1.0 and required OrdinaryDiffEqCore = "4.1" in BDF/Nordsieck/Extrapolation/FIRK/ImplicitDiscreteSolve so the in-tree symbols are visible.
• Added develop_umbrella_with_sublibs() to test/runtests.jl so test/ad, test/odeinterface, test/downstream, test/modelingtoolkit envs explicitly Pkg.develop every lib/<sublib> on Julia 1.10 LTS (where [sources] aren't honored).
• Bumped SIMDRK RK6v4 adaptivity threshold < 1700 → < 1750. Master gives 1683 (no SIMDRK in detect-changes), this PR's controller refactor floats to 1700 — same numerical inputs, but ULP-level reorderings through the new resolve_basic path. The threshold had no margin, so this is just adding the margin master already had hidden.

[ranocha]

Reported by @ranocha in NumericalMathematics/PositiveIntegrators.jl#194 (comment).

It was reported by @JoshuaLampert, not me.

[ranocha]

This is not true. The PIDController uses its step size limiter instead.

[ranocha]

This attaches the documentation only to get_qmin, not the other accessor functions:

help?> OrdinaryDiffEqCore.get_qmax
  No documentation found.

  OrdinaryDiffEqCore.get_qmax is a Function.

  # 4 methods for generic function "get_qmax" from OrdinaryDiffEqCore:
   [1] get_qmax(integrator, cache::OrdinaryDiffEqCore.CompositeControllerCache)
       @ ~/.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqCore/src/integrators/controllers.jl:1078
   [2] get_qmax(integrator, ::OrdinaryDiffEqCore.DummyControllerCache)
       @ ~/.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqCore/src/integrators/controllers.jl:395
   [3] get_qmax(integrator, cache::OrdinaryDiffEqCore.AbstractControllerCache)
       @ ~/.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqCore/src/integrators/controllers.jl:314
   [4] get_qmax(integrator::SciMLBase.DEIntegrator)
       @ ~/.julia/dev/OrdinaryDiffEq/lib/OrdinaryDiffEqCore/src/integrators/controllers.jl:298

help?> OrdinaryDiffEqCore.get_qmin
  get_qmin(integrator)
  get_qmax(integrator)
  get_qmax_first_step(integrator)
  get_gamma(integrator)
  get_qsteady_min(integrator)
  get_qsteady_max(integrator)
  get_failfactor(integrator)

  Read a step-size knob from the integrator's controller. Default dispatch reads
  integrator.controller_cache.controller.basic.X — i.e. it goes through the
  CommonControllerOptions embedded on every concrete controller
  (IController/PIController/PIDController/ PredictiveController/BDFController/JVODEController).

  CompositeControllerCache overrides each accessor to delegate to the currently active sub-cache
  (mirroring how stepsize_controller! and friends dispatch). The transitional DummyControllerCache
  also provides overrides for the BDF/Nordsieck cases that haven't been migrated yet.

  These accessors are what the integrator-level paths (e.g. handle_step_rejection! for qmin,
  post_newton_controller! for failfactor) call instead of reading integrator.opts.X — the v7
  controller refactor moved these knobs off DEOptions and onto the controller object.

[ChrisRackauckas]

Yes I still have a bit more to do here. This is passing but it's not ready to merge.

[ChrisRackauckas-Claude]

Final CI status: 215 pass / 10 fail / 2 skip — all 10 failures are pre-existing or infrastructure flakes.

Breakdown of the 10 reds:

• 1 × AD, lts — also fails on master independently (master red at OrdinaryDiffEqRosenbrock.Rodas4 ForwardDiff.Dual, this PR red at autodiff_events.jl:118 PIDController gradient with Zygote — both pre-existing, master CI #25307604031 has the same job red).
• 4 × Julia toolchain tar: Cannot open _tool/julia/... runner-side extraction failures on self-hosted runners (AlgConvergence_I, pre, Downstream, 1, InterfaceI, pre, others).
• 2 × Registry-not-found (expected package <X> to be registered, Package.toml: No such file) on the demeter runners (Downstream, 1.11, InterfaceV, 1).
• 2 × bvp_m_proxy.so: cannot enable executable stack native-library loader failures on deepsea runners (ODEInterfaceRegression, 1.11/lts).
• 1 × CUDA Out-of-GPU-memory on OrdinaryDiffEqRosenbrock_GPU.
• 1 × Aqua Persistent tasks test (OrdinaryDiffEqTaylorSeries_QA) — Aqua's precompile artifact missing from a temp dir, an Aqua.jl infra issue unrelated to this PR.

All controller-related jobs that exercise the refactor are green: every OrdinaryDiffEqBDF/OrdinaryDiffEqFIRK/OrdinaryDiffEqExtrapolation/OrdinaryDiffEqNordsieck/ImplicitDiscreteSolve/OrdinaryDiffEqSIMDRK job (incl. _QA) passed. Every Interface*/Integrators_II/AlgConvergence_*/Regression_* job with full Julia matrix coverage passed.