Oceananigans.jl
Oceananigans.jl copied to clipboard
CliMA
(0.91.1) Remove instances of `previous_Δt` and fix a bug setting `last_Δt` in RK3
This PR fixes a bug where last_Δt was incorrect for RK3. This bug was found by @tomchor, cc @jagoosw
Also it removes previous_Δt from the QuasiAdamsBashforth2 time-stepper. Note, this affects checkpointing. I also correctly restore last_Δt from the checkpoint.
I also updated some docstrings just so we have uniform language, replacing "previous" with "last" in a few places.
Do we not want the substep length rather than the total time step length? For example on the cases where we're explicitly stepping a boundary condition they're getting stepped every substep.
Do we not want the substep length rather than the total time step length? For example on the cases where we're explicitly stepping a boundary condition they're getting stepped every substep.
It's not a question of having one or the other. The question is whether "last_Δt" means "last time step".
We need to save the previous time step for various reasons --- for AB2 it's needed to decide whether to re-initialize with an Euler step.
If we need the previous substep that was taken during RK3 multi-stage stepping, that's fine. It's the road to insanity if last_Δt means "last substep" in some cases, and "last time step" in others. Given the choice, why deliberately drive ourselves insane when we can simply save both the substep and the total time step?
I guess it is also worth nothing that in principle the substep could be inferred from the stage and total time-step. I guess. Perhaps it's better to save the whole time-step though.
I wonder if the clock even needs to know what type of time stepping scheme is being used, in fact, but we can do one thing at a time.
So just to clarify @jagoosw can you confirm that you would like the clock to record substep intervals? I will implement that if so and call it something like last_stage_Δt (as a nod to the stage property that the clock has already). We can flag this PR as a breaking change, as well.
Yeah this makes sense, and yes, I think in some cases we will need the last substep interval yes.
So that we can write code that works with both timesteppers I think it would make sense for last_\Delta t to be filled for AB2 as well?
So that we can write code that works with both timesteppers I think it would make sense for
last_\Delta tto be filled for AB2 as well?
Yes, we do that here https://github.com/CliMA/Oceananigans.jl/blob/02549527330b48ef7e1023e0fc8de191b3d66b6c/src/TimeSteppers/quasi_adams_bashforth_2.jl#L94
