-
Notifications
You must be signed in to change notification settings - Fork 173
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
(0.91.1) Remove instances of previous_Δt
and fix a bug setting last_Δt
in RK3
#3595
base: main
Are you sure you want to change the base?
Conversation
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 " 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 |
I guess it is also worth nothing that in principle the substep could be inferred from the 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 |
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 |
Yes, we do that here
|
previous_Δt
and fix a bug setting last_Δt
in RK3previous_Δt
and fix a bug setting last_Δt
in RK3
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.
LGTM!
This PR fixes a bug where
last_Δt
was incorrect for RK3. This bug was found by @tomchor, cc @jagooswAlso it removes
previous_Δt
from the QuasiAdamsBashforth2 time-stepper. Note, this affects checkpointing. I also correctly restorelast_Δt
from the checkpoint.I also updated some docstrings just so we have uniform language, replacing "previous" with "last" in a few places.