-
-
Notifications
You must be signed in to change notification settings - Fork 235
New Controller Interface #2899
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
New Controller Interface #2899
Conversation
| erracc::QT | ||
| dtacc::tType | ||
| # TODO ^^^ remove these | ||
| controller_cache::CC |
There was a problem hiding this comment.
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.
…rollers in the new interface
|
@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) |
Please note that I have changed the names back. They now differ only by an intermediate supertype. So |
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. |
|
While doing this refactor, maybe it would be good to add the vector of EEst to the controller? Right now we usually call this |
|
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
endand 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
endtogether with the respective change in 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
endwhich 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
endThis 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. |
Resolves #2883 .
TODO
Not addressed in this PR / Remaining things to be addressed
step_accept_controller!(integrator, controller_cache::AbstractControllerCache, q)DummyController