Fix controller type instability in default_controller_v7 (Issue #2855)#3034
Conversation
be5c9cd to
cfcea71
Compare
…#2855) The problem was that `default_controller_v7` returned `Union{NewIController, NewPIController, NewPredictiveController}` because `ispredictive`/`isstandard` traits checked runtime Symbol values (`alg.controller === :Predictive`). Solution: Add controller type as a type parameter to algorithm structs and store controller instances in a `controller` field, enabling compile-time dispatch via `ispredictive(alg.controller)` and `isstandard(alg.controller)`. Changes: - Define controller type tags in OrdinaryDiffEqCore: PIControllerType, PredictiveControllerType, StandardControllerType (singleton types) - Add `_controller_type_from_symbol` helper for backwards compatibility (Symbol input like `:PI` still works) - Update 35 algorithm structs to include: * CT type parameter * controller::CT field storing the controller instance * SDIRK: 22 algorithms (ImplicitEuler, Trapezoid, TRBDF2, etc.) * BDF: 8 algorithms (ABDF2, QNDF, FBDF, etc.) * FIRK: 4 algorithms (RadauIIA3, RadauIIA5, RadauIIA9, AdaptiveRadau) * StabilizedIRK: 1 algorithm (IRKC) * StabilizedRK: 1 algorithm (SERK2) - Simplified `ispredictive`/`isstandard` dispatches to use `alg.controller` field instead of extracting from type parameters Now `default_controller_v7` returns a concrete type instead of a Union, improving type stability and performance. Co-Authored-By: Chris Rackauckas <[email protected]> Co-Authored-By: Claude Opus 4.5 <[email protected]>
cfcea71 to
e4000fd
Compare
| @@ -1,3 +1,24 @@ | |||
| ## Controller Type Tags for Type-Stable Dispatch | |||
There was a problem hiding this comment.
This looks unnecessary. Is there any reason why we should not simply pass the controller directly to the algorithms, such that we can move the controller cache as a field into the algorithm's cache?
There was a problem hiding this comment.
Yeah there is no need for this, I just hadn't reviewed yet.
| extrapolant::Symbol | ||
| kappa::κType | ||
| controller::Symbol | ||
| controller::CT |
There was a problem hiding this comment.
@ChrisRackauckas I just noticed in #3035 that this should not be here, as QNDF comes with its build-in controller, right?
It looks like that, for algos with built-in controller, this PR essentially tries to override the built-in controllers with an integral controller.
…#2855) The problem was that `default_controller_v7` returned `Union{NewIController, NewPIController, NewPredictiveController}` because `ispredictive`/`isstandard` traits checked runtime Symbol values (`alg.controller === :Predictive`). Solution: Add specific `default_controller_v7` dispatches for each algorithm type, enabling compile-time dispatch directly to the appropriate controller constructor. This is simpler than adding type parameters to algorithm structs. Changes: - SDIRK algorithms (28): dispatch to `NewPIController` - BDF algorithms (8): dispatch to `NewIController` - FIRK algorithms (4): dispatch to `NewPredictiveController` - IRKC: dispatch to `NewIController` - RKC: dispatch to `NewPredictiveController` Now `default_controller_v7` returns a concrete type instead of a Union, improving type stability and performance. Co-Authored-By: Chris Rackauckas <[email protected]> Co-Authored-By: Claude Opus 4.5 <[email protected]>
Updated ApproachBased on the review feedback, I've simplified the implementation: Instead of adding type parameters to algorithm structs, I've added specific ChangesAdded
Verificationusing OrdinaryDiffEq, InteractiveUtils
using OrdinaryDiffEqCore: default_controller_v7
# All algorithms now return concrete types (not Union)
@code_warntype default_controller_v7(Float64, ImplicitEuler())
# Body::OrdinaryDiffEqCore.NewPIController{Float64}
@code_warntype default_controller_v7(Float64, RadauIIA5())
# Body::OrdinaryDiffEqCore.NewPredictiveController{Float64}
@code_warntype default_controller_v7(Float64, FBDF())
# Body::OrdinaryDiffEqCore.NewIController{Float64}This approach:
|
|
This entire backport is just a mess. Can you just do it right and we do this as OrdinaryDiffEqCore breaking update? It is pretty insane like this. |
|
Let me propose something and then we will iron out the details in the PR. |
Summary
default_controllertype-unstable for most algorithms #2855:default_controller_v7was returningUnion{NewIController, NewPIController, NewPredictiveController}becauseispredictive/isstandardtraits checked runtime Symbol values:PI,:Predictive,:Standard) still worksChanges
Core Changes (OrdinaryDiffEqCore)
PIControllerType,PredictiveControllerType,StandardControllerType_controller_type_from_symbolhelper for backwards compatibilityispredictive/isstandardtrait dispatches to work on typesAlgorithm Updates
Verification
Type stability is now achieved:
Backwards compatibility maintained:
Test plan
@code_warntype🤖 Generated with Claude Code