New Controller Interface #2899
New Controller Interface #2899
Conversation
| err1, err2, err3 = cache.err | ||
|
|
||
| k = min(alg_order(alg), alg_adaptive_order(alg)) + 1 | ||
| dt_factor = err1^(beta1 / k) * err2^(beta2 / k) * err3^(beta3 / k) |
There was a problem hiding this comment.
use fastpow and inv(k) to save computation
There was a problem hiding this comment.
We have some evidence in Trixi.jl CI tests that fastpow (via Float32) reduces reproducibility of computational results across different systems, while the impact on computational efficiency is minimal (if at all noticeable). The PID controllers are used only by a few algorithms designed for large-scale PDE discretizations, where the runtime performance of computing the time step size is negligible. In contrast, stability and reproducibility are essential to us.
There was a problem hiding this comment.
I am also a bit confused. Even without fast powers etc the controller code should never become the bottleneck if we have more than a few nonlinear ODEs, right?
There was a problem hiding this comment.
Large PDEs is the best case scenario here so it's not a good testing case. The worst case scenario is a small non-stiff ODE. Test again with something like a 2 ODE one compartment model. What is the overhead? At least last it was timed at around 5x.
I would be curious to know what you mean by "reproducibility" here because there is nothing that is platform-dependent and no system libraries used
There was a problem hiding this comment.
Lorenz equations or other similarly sized chaotic systems are also good examples to test here.
There was a problem hiding this comment.
We typically test many examples: run an ODE solver for a few time steps and record the norms of the solution minus a reference (e.g., the initial condition). With fastpow, we observed that the results seem to differ more significantly when running on different systems (likely due to the reduced precision during the computation).
There was a problem hiding this comment.
None of the default methods uses a PIDController, which is the only one not using fastpow at the moment. I would like to keep this state for backward compatibility in Trixi.jl.
There was a problem hiding this comment.
This PR definitely shouldn't change any code. It's a refactor? Any substance changes would need to go anywhere else. I didn't see that this wasn't to the PI
We typically test many examples: run an ODE solver for a few time steps and record the norms of the solution minus a reference (e.g., the initial condition). With fastpow, we observed that the results seem to differ more significantly when running on different systems (likely due to the reduced precision during the computation).
That's definitely odd. I used test have some checks on chaotic systems that you'd reproduce across OS's and chips . @oscardssmith can you look into this?
There was a problem hiding this comment.
xref #2899 (comment) .
| erracc::QT | ||
| dtacc::tType | ||
| # TODO ^^^ remove these | ||
| controller_cache::CC |
There was a problem hiding this comment.
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. |
4 participants
Resolves #2883 .
TODO
Not addressed in this PR
step_accept_controller!(integrator, controller_cache::AbstractControllerCache, q)DummyController