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

set! for time step #650

Merged
merged 6 commits into from
Jan 7, 2025
Merged

set! for time step #650

merged 6 commits into from
Jan 7, 2025

Conversation

milankl
Copy link
Member

@milankl milankl commented Jan 7, 2025

To be used like

spectral_grid = SpectralGrid(trunc=63)
L = Leapfrog(spectral_grid)
set!(L, Minute(10))

Also disables the adjustment of the timestep towards the desired output frequency and recalculates all dt-based fields in Leapfrog.

@milankl milankl added the time integration 🕙 Time integration of the equations label Jan 7, 2025
@glwagner
Copy link

glwagner commented Jan 7, 2025

This will probably be useful for running simulations in conditions that differ from the ones typical to Earth today, especially idealized simulations.

Copy link
Member

@maximilian-gelbrecht maximilian-gelbrecht left a comment

Choose a reason for hiding this comment

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

Nice!

Maybe just make it clear in the docs (string) that this is meant as the actual time step and not the scaled time set that we use a keyword to configure the model. Could be a bit confusing for users otherwise.

Alternatively one could also just do both versions:

set!(L; Δt = Minute(10))
set!(L; Δt_at_T31 = Minute(10))

with

set!(L, P::Period) = set!(L; Δt=P)

@milankl
Copy link
Member Author

milankl commented Jan 7, 2025

I've decided to allow for positional argument and keyword argument, i.e.

set!(L, Δt)
set!(L, Δt = Minute(10))

but feel that Δt_at_T31 should be just set when creating a Leapfrog in the first place?

@maximilian-gelbrecht
Copy link
Member

Fine with me as long as it's clear that it's unscaled, which it is now.

@milankl milankl merged commit 2d97c54 into main Jan 7, 2025
6 checks passed
@glwagner
Copy link

glwagner commented Jan 7, 2025

Out of curiosity, what is the relevance of "time step at T31" if you are not using the T31 grid?

@milankl
Copy link
Member Author

milankl commented Jan 7, 2025

Out of curiosity, what is the relevance of "time step at T31" if you are not using the T31 grid?

It just scales linearly. If you have 40min at T31, then it's 20min at T63, 10min at T127, 5min ... etc. So normally a user wouldn't need to worry about the time step, but if they want the timestep to be, say, 10% shorter, then choose 36min at T31 and whatever resolution you run at is then automatically scaled to. We just wanted to hide the issue of figuring out what a stable time step size is given some resolution as best as possible

@glwagner
Copy link

glwagner commented Jan 7, 2025

Okay, so if you are using T127 and want a shorter time-step, then a user should figure out what the T31 time-step is and shorten that by an appropriate amount to obtain the desired scaling of the T127 time step?

Is the time-step determined by a CFL limitation?

@milankl milankl mentioned this pull request Jan 8, 2025
@milankl milankl deleted the mk/settimestep branch January 8, 2025 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
time integration 🕙 Time integration of the equations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants