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

Split implicit and explicit precomputed quantities #3663

Merged
merged 1 commit into from
Mar 6, 2025

Conversation

dennisYatunin
Copy link
Member

Purpose

This PR splits the precomputed quantities that need to be handled implicitly (i.e., updated on each Newton iteration) from those that can be handled explicitly (i.e., held constant throughout each Newton solve).

This change should improve performance for all simulations with more than one Newton iteration, since we are currently spending a significant chunk of time updating precomputed quantities that can be handled explicitly. Specifically, this should reduce the performance penalty that we have observed in #3662 from increasing the number of Newton iterations for configurations with implicit diffusion.


  • I have read and checked the items on the review checklist.

@dennisYatunin dennisYatunin requested review from charleskawczynski and szy21 and removed request for charleskawczynski February 28, 2025 08:20
@dennisYatunin dennisYatunin force-pushed the dy/split_precomputed branch 7 times, most recently from ddb3f54 to 03e24f2 Compare February 28, 2025 10:21
@szy21
Copy link
Member

szy21 commented Feb 28, 2025

tagging @costachris because there are prognostic edmf changes

@szy21 szy21 requested a review from costachris February 28, 2025 17:52
@szy21
Copy link
Member

szy21 commented Feb 28, 2025

Is it expected to change MSE?

Copy link
Member

@charleskawczynski charleskawczynski left a comment

Choose a reason for hiding this comment

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

Overall the changes look fine, but I'm also concerned that there are mse changes. Any idea what is wrong there?

@dennisYatunin
Copy link
Member Author

Yes, we do expect MSE changes for simulations with implicit diffusion. We were previously updating the entire cache before computing the implicit tendency, but now we are only updating quantities related to velocity and thermodynamics. In particular, we are no longer updating the diffusivity, so its value in the implicit tendency is now reused from the previous explicit tendency. Since we are computing the implicit tendency with a different diffusivity, we expect the results to change. However, the diffusivity is mostly constant with respect to the state, so we only have small MSE changes and no meaningful differences in the summary plots.

I am going to add a simple heat equation test with fixed diffusivity in a separate PR, and then I will be able to confirm that these changes have no effect on implicit diffusion when diffusivity is constant.

@dennisYatunin dennisYatunin force-pushed the dy/split_precomputed branch 4 times, most recently from d65aa7b to 15ae1c0 Compare March 1, 2025 07:08
@dennisYatunin dennisYatunin force-pushed the dy/split_precomputed branch from 15ae1c0 to b954eee Compare March 5, 2025 18:48
@szy21
Copy link
Member

szy21 commented Mar 5, 2025

@costachris Could you take a look at the prognostic edmf relevant changes in this PR and see if it makes sense to you and/or make sure you understand the change?

@dennisYatunin dennisYatunin force-pushed the dy/split_precomputed branch from b954eee to 2569a49 Compare March 6, 2025 01:23
@dennisYatunin dennisYatunin force-pushed the dy/split_precomputed branch from 2569a49 to ff088a4 Compare March 6, 2025 02:38
@dennisYatunin dennisYatunin enabled auto-merge March 6, 2025 02:39
@dennisYatunin dennisYatunin added this pull request to the merge queue Mar 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 6, 2025
@dennisYatunin dennisYatunin added this pull request to the merge queue Mar 6, 2025
Merged via the queue into main with commit dd2f543 Mar 6, 2025
17 of 20 checks passed
@dennisYatunin dennisYatunin deleted the dy/split_precomputed branch March 6, 2025 11:35
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.

4 participants