Skip to content

Conversation

@termi-official
Copy link
Contributor

@termi-official termi-official commented Oct 26, 2025

Resolves #2883 .

TODO

  • Upgrade non-standard controllers to new interface
  • Introduce multi-controller for algorithms with stiffness switching
  • Add appropriate documentation of the full API

Not addressed in this PR / Remaining things to be addressed

  • Reformulate PI controller to remove inverses (break semantics)
  • Remove redundant variables from integrator (breaking change)
  • Move controller cache into algorithm cache whenever appropriate (and maybe add the controller to the algorithm)
  • Remove q from step_accept_controller!(integrator, controller_cache::AbstractControllerCache, q)
  • Remove DummyController
  • Update DelayDiffEq and friends to use the new interface

erracc::QT
dtacc::tType
# TODO ^^^ remove these
controller_cache::CC
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at some of the algorithms, we even might want to think moving the controller into the algorithm cache together with some helpers to construct standard controllers.

@termi-official termi-official marked this pull request as ready for review October 27, 2025 18:25
@oscardssmith
Copy link
Member

@ChrisRackauckas what would you think about removing the Legacy controllers (the renamed version of the current ones). I don't think this version is less breaking, and the new controllers aren't breaking (except for people making their own controllers which I don't think is common)

@termi-official
Copy link
Contributor Author

[..] Legacy controllers (the renamed version of the current ones).

Please note that I have changed the names back. They now differ only by an intermediate supertype. So PIController <: AbstractLegacyController is the old one and NewPIController <: AbstractController is the new one.

@ChrisRackauckas
Copy link
Member

@ChrisRackauckas what would you think about removing the Legacy controllers (the renamed version of the current ones). I don't think this version is less breaking, and the new controllers aren't breaking (except for people making their own controllers which I don't think is common)

I think it's best to just have a deprecation path. v7 is coming soon and we just delete them with that. So if it's not too hard to have a deprecation path that would definitely help people with the upgrading. But yes the main user is likely @ranocha and the Trixi crew, I at least don't see any use cases publicly in the wild.

@ChrisRackauckas
Copy link
Member

While doing this refactor, maybe it would be good to add the vector of EEst to the controller? Right now we usually call this atmp, but it's not uniform across the methods. Basically, it would be good to make that vector of all error estimates be something that is uniformly handled, not just the norm(vector_EEst). The reason is I'm going to want to set it up with logging really soon, because knowing which value is causing step rejections is really helpful for knowing why a solver haulted. But in the current form you'd need to implement that separately per solver. It would be good to make sure this can be more nicely handled in the new controller interface, as that would then make it straightforward to add logging about error control in the controller interface rather than at per-solver level.

@termi-official
Copy link
Contributor Author

termi-official commented Oct 30, 2025

Should this also go into this PR or a separate one? If I understand you correctly, then we would like to have changes of the form

@cache mutable struct ImplicitEulerCache{
    uType, rateType, uNoUnitsType, N, AV, StepLimiter} <:
                      SDIRKMutableCache
    u::uType
    uprev::uType
    uprev2::uType
    fsalfirst::rateType
    atmp::uNoUnitsType # Remove this variable here
    nlsolver::N
    algebraic_vars::AV
    step_limiter!::StepLimiter
end

and analogue

mutable struct IControllerCache{C, T, uNoUnityType} <: AbstractControllerCache
    controller::C
    q::T
    dtreject::T
    # atmp::uNoUnityType
    # I believe this should go here or in the algorithm cache, but not in the integrator itself.
    # EEst::T
end

together with the respective change in perform_step! and the constant cache?

Also, I think it makes sense to move the controller cache into the algorithm cache and the controller should be part of the algorithm constructor. E.g.

struct ImplicitEuler{CS, AD, F, F2, P, CT, FDT, ST, CJ, StepLimiter} <:
       OrdinaryDiffEqNewtonAdaptiveAlgorithm{CS, AD, FDT, ST, CJ}
    linsolve::F
    nlsolve::F2
    precs::P
    extrapolant::Symbol
    controller::CT # -> was "controller::Symbol" before
    step_limiter!::StepLimiter
    autodiff::AD
end

which defaults to e.g. PIController, while we get for the caches

@cache mutable struct ImplicitEulerCache{
    uType, rateType, uNoUnitsType, N, AV, StepLimiter, CC} <:
                      SDIRKMutableCache
    u::uType
    uprev::uType
    uprev2::uType
    fsalfirst::rateType
    atmp::uNoUnitsType
    nlsolver::N
    algebraic_vars::AV
    step_limiter!::StepLimiter
    controller_cache::CC
end

This could help to simplify the init logic a little bit, as we hand down the initialization of the controller to the algorithm -- and ultimately the algorithm knows best if the passed controller is compatible or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Interaction between controller api and time loop

4 participants