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

Only call set_precomputed_quantities! once in the implict stage #2015

Closed
Tracked by #1428
simonbyrne opened this issue Aug 24, 2023 · 4 comments · Fixed by #2187
Closed
Tracked by #1428

Only call set_precomputed_quantities! once in the implict stage #2015

simonbyrne opened this issue Aug 24, 2023 · 4 comments · Fixed by #2187
Assignees

Comments

@simonbyrne
Copy link
Member

Currently we call set_precomputed_quantities! in the Wfact!:

set_precomputed_quantities!(Y, p, t)

and once again in the implicit_tendency!:
set_precomputed_quantities!(Y, p, t)

But the state should be the same in both cases, so this should be unnecessary.

@simonbyrne
Copy link
Member Author

simonbyrne commented Aug 24, 2023

This appears to break the SSP tests, since they don't quite follow the same logic.

I think the general solution here is to move set_precomputed_quantities! so that it is applied after each stage update: in this way, our cache will always be in sync with the current state (which means that we don't need to call it in the diagnostics). This will mean that we don't have to do it in diagnostics.

@simonbyrne
Copy link
Member Author

We also call it twice in the explicit stage: once in limited_tendencies!:
https://github.com/CliMA/ClimaAtmos.jl/blob/f751e691c05ae935ff30d82b7ef804021f5355ea/src/prognostic_equations/limited_tendencies.jl

and once in remaining_tendencies!:

set_precomputed_quantities!(Y, p, t)

With the ARS343, the second is called directly after the first. Can we safely get rid of it in the second one?

@simonbyrne
Copy link
Member Author

@dennisYatunin ?

@dennisYatunin
Copy link
Member

Yes, the two explicit tendencies are computed for the same state, so their precomputed quantities should be identical. The order in which the tendencies are evaluated is implementation-dependent, though, so it would definitely be safer to separate out the precomputed quantities computation and guarantee that it will only be called once.

bors bot added a commit that referenced this issue Sep 27, 2023
2161: Update manifest files, overload ClimaTimeSteppers r=charleskawczynski a=charleskawczynski

This PR upgrades to the latest dependencies and overloads ClimaTimeSteppers methods so that we can more easily iterate on fixing #2015.

Co-authored-by: Charles Kawczynski <[email protected]>
bors bot added a commit that referenced this issue Sep 27, 2023
2161: Update manifest files, overload ClimaTimeSteppers r=charleskawczynski a=charleskawczynski

This PR upgrades to the latest dependencies and overloads ClimaTimeSteppers methods so that we can more easily iterate on fixing #2015.

Co-authored-by: Charles Kawczynski <[email protected]>
bors bot added a commit that referenced this issue Sep 27, 2023
2161: Update manifest files, overload ClimaTimeSteppers r=charleskawczynski a=charleskawczynski

This PR upgrades to the latest dependencies and overloads ClimaTimeSteppers methods so that we can more easily iterate on fixing #2015.

Co-authored-by: Charles Kawczynski <[email protected]>
bors bot added a commit that referenced this issue Sep 27, 2023
2161: Update manifest files, overload ClimaTimeSteppers r=charleskawczynski a=charleskawczynski

This PR upgrades to the latest dependencies and overloads ClimaTimeSteppers methods so that we can more easily iterate on fixing #2015.

Co-authored-by: Charles Kawczynski <[email protected]>
bors bot added a commit that referenced this issue Sep 27, 2023
2161: Update manifest files, overload ClimaTimeSteppers r=charleskawczynski a=charleskawczynski

This PR upgrades to the latest dependencies and overloads ClimaTimeSteppers methods so that we can more easily iterate on fixing #2015.

Co-authored-by: Charles Kawczynski <[email protected]>
bors bot added a commit that referenced this issue Sep 27, 2023
2161: Update manifest files, overload ClimaTimeSteppers r=charleskawczynski a=charleskawczynski

This PR upgrades to the latest dependencies and overloads ClimaTimeSteppers methods so that we can more easily iterate on fixing #2015.

Co-authored-by: Charles Kawczynski <[email protected]>
bors bot added a commit that referenced this issue Sep 27, 2023
2161: Update manifest files, overload ClimaTimeSteppers r=charleskawczynski a=charleskawczynski

This PR upgrades to the latest dependencies and overloads ClimaTimeSteppers methods so that we can more easily iterate on fixing #2015. (cc `@simonbyrne)`

Co-authored-by: Charles Kawczynski <[email protected]>
bors bot added a commit that referenced this issue Sep 27, 2023
2161: Update manifest files, overload ClimaTimeSteppers r=charleskawczynski a=charleskawczynski

This PR upgrades to the latest dependencies and overloads ClimaTimeSteppers methods so that we can more easily iterate on fixing #2015. (cc `@simonbyrne)`

Co-authored-by: Charles Kawczynski <[email protected]>
bors bot added a commit that referenced this issue Sep 27, 2023
2161: Update manifest files, overload ClimaTimeSteppers r=charleskawczynski a=charleskawczynski

This PR upgrades to the latest dependencies and overloads ClimaTimeSteppers methods so that we can more easily iterate on fixing #2015. (cc `@simonbyrne)`

Co-authored-by: Charles Kawczynski <[email protected]>
bors bot added a commit that referenced this issue Sep 27, 2023
2161: Update manifest files, overload ClimaTimeSteppers r=charleskawczynski a=charleskawczynski

This PR upgrades to the latest dependencies and overloads ClimaTimeSteppers methods so that we can more easily iterate on fixing #2015. (cc `@simonbyrne)`

Co-authored-by: Charles Kawczynski <[email protected]>
bors bot added a commit that referenced this issue Sep 27, 2023
2161: Update manifest files, overload ClimaTimeSteppers r=charleskawczynski a=charleskawczynski

This PR upgrades to the latest dependencies and overloads ClimaTimeSteppers methods so that we can more easily iterate on fixing #2015. (cc `@simonbyrne)`

Co-authored-by: Charles Kawczynski <[email protected]>
bors bot added a commit that referenced this issue Sep 27, 2023
2161: Update manifest files, overload ClimaTimeSteppers r=charleskawczynski a=charleskawczynski

This PR upgrades to the latest dependencies and overloads ClimaTimeSteppers methods so that we can more easily iterate on fixing #2015. (cc `@simonbyrne)`

Co-authored-by: Charles Kawczynski <[email protected]>
bors bot added a commit that referenced this issue Sep 28, 2023
2161: Update manifest files, overload ClimaTimeSteppers r=charleskawczynski a=charleskawczynski

This PR upgrades to the latest dependencies and overloads ClimaTimeSteppers methods so that we can more easily iterate on fixing #2015. (cc `@simonbyrne)`

Co-authored-by: Charles Kawczynski <[email protected]>
bors bot added a commit that referenced this issue Sep 28, 2023
2161: Update manifest files, overload ClimaTimeSteppers r=charleskawczynski a=charleskawczynski

This PR upgrades to the latest dependencies and overloads ClimaTimeSteppers methods so that we can more easily iterate on fixing #2015. (cc `@simonbyrne)`

2164: limit mixing length by z - z_sfc r=szy21 a=szy21



Co-authored-by: Charles Kawczynski <[email protected]>
Co-authored-by: Zhaoyi Shen <[email protected]>
bors bot added a commit that referenced this issue Oct 4, 2023
2186: Call `set_precomputed_quantities!` in `step!`, remove from callbacks r=charleskawczynski a=charleskawczynski

This PR adds a call to `set_precomputed_quantities!` in `step!` and removes all of them from the callbacks. This PR cherry-picked the first commit in #2175, but is somewhat separate in that this PR will potentially be behavior changing.

Once this PR is merged (assuming there are no changes, or they are negligible), we should be able to follow through with closing out #2015.

Co-authored-by: Charles Kawczynski <[email protected]>
@bors bors bot closed this as completed in 3a80801 Oct 4, 2023
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 a pull request may close this issue.

3 participants