Skip to content
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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

glwagner
Copy link
Member

@glwagner glwagner commented May 9, 2024

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.

@jagoosw
Copy link
Collaborator

jagoosw commented May 10, 2024

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.

@glwagner
Copy link
Member Author

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?

@glwagner
Copy link
Member Author

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.

@glwagner
Copy link
Member Author

glwagner commented May 10, 2024

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.

@jagoosw
Copy link
Collaborator

jagoosw commented May 11, 2024

Yeah this makes sense, and yes, I think in some cases we will need the last substep interval yes.

@glwagner glwagner requested a review from jagoosw May 13, 2024 20:56
@jagoosw
Copy link
Collaborator

jagoosw commented May 14, 2024

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?

@glwagner
Copy link
Member Author

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?

Yes, we do that here

@glwagner glwagner changed the title Remove instances of previous_Δt and fix a bug setting last_Δt in RK3 (0.91.1) Remove instances of previous_Δt and fix a bug setting last_Δt in RK3 May 16, 2024
Copy link
Collaborator

@jagoosw jagoosw left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

None yet

2 participants