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

Provide a better explanation of the max_change and min_change of TimeStepWizard #3245

Merged
merged 5 commits into from
Aug 31, 2023
Merged

Conversation

iuryt
Copy link
Collaborator

@iuryt iuryt commented Aug 31, 2023

closes #3244

Copy link
Collaborator

@navidcy navidcy left a comment

Choose a reason for hiding this comment

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

the phrasing in HC example seems tad misleading to me

@navidcy
Copy link
Collaborator

navidcy commented Aug 31, 2023

btw, @iuryt if you change the first comment of the PR to say

closes #3244

then the issue will be closed automatically as soon as the PR is merged.

iuryt and others added 2 commits August 31, 2023 09:41
Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>
Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>
Co-authored-by: iury simoes-sousa <iuryt@pm.me>
@navidcy navidcy merged commit 0590a52 into CliMA:main Aug 31, 2023
Comment on lines +36 to +38
`simulation.Δt` is not adapted "too quickly". In other words, `max_change` is the maximum
and `min_change` is the minimum relative change of the time step and are, therefore, non-dimensional
(`min_change * old_Δt ≤ new_Δt ≤ max_change * old_Δt`).
Copy link
Member

Choose a reason for hiding this comment

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

Note there is a grammatical error in the above as well. If we're seeking improved clarity, I suggest changing to something like

Callback function that adapts the simulation time step to a target
advective Courant-Freidrichs-Lewy (CFL) number `cfl`, within the limits

    max(max_Δt, max_change * previous_Δt)  new_Δt  min(min_Δt, min_change * previous_Δt)

where `new_Δt` is the new time step calculated by `TimeStepWizard`.

Or something along those lines...

@iuryt iuryt mentioned this pull request Aug 31, 2023
siddharthabishnu pushed a commit that referenced this pull request Sep 11, 2023
…`TimeStepWizard` (#3245)

* Update docstring of the TimeStepWizard

* Update TimeStepWizard explanation in the horizontal_convection example

* Change symbol for 'less than or equal' and 'greater than or equal'

Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>

* Change symbol for 'less than or equal' and 'greater than or equal'

Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>

* better phrasing

Co-authored-by: iury simoes-sousa <iuryt@pm.me>

---------

Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add to the docs a better explanation about max_change and min_change of the TimeStepWizard
3 participants