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

Add DSS before implicit solve, fix T_imp approximation and callbacks #352

Merged
merged 2 commits into from
Jan 25, 2025

Conversation

dennisYatunin
Copy link
Member

@dennisYatunin dennisYatunin commented Jan 22, 2025

Purpose

This PR undoes a small change that was accidentally introduced in #348. We were previously approximating the implicit tendency as (U_after_implicit_solve - U_before_implicit_solve) / (Δt * γ), but in that PR I ended up changing it to (DSS(U_after_implicit_solve) - U_before_implicit_solve) / (Δt * γ). In the absence of roundoff error, these would be exactly identical. Unfortunately, we do not satisfy DSS(DSS(U)) == DSS(U) at a discrete level, so they are not quite identical. Reverting to the original approximation of T_imp might marginally improve stability.

Update: After testing this change with AMIP, we have found that the original call to DSS before the implicit solve (which was removed in #348) is still necessary, even if we fix the approximation of T_imp. Without this call to DSS, we are effectively including the effects of DSSing T_exp and T_lim on previous stages in the approximation of T_imp on the current stage, which is not physically meaningful and appears to hurt stability. It might also be a good idea to apply DSS on each Newton iteration (as HOMME appears to do), but this doesn't have any noticeable effect when the number of Newton iterations is small, as is currently the case for AMIP and all our other simulations. So, for now we will only apply DSS before and after the implicit solve, but we may need additional calls to DSS if we ever use more Newton iterations (e.g., with implicit EDMF).

Along with the fixes to DSS and the approximation of T_imp, this PR now also fixes how the stage callbacks post_explicit! and post_implicit! are used. With post_explicit! called at the end of each stage and post_implicit! called before each Newton iteration, we can rename them to something like post_stage! and pre_newton_iteration! (as per my comment on #323) and update ClimaAtmos accordingly. I have also updated the values of t_exp and t_imp that are passed to DSS and the stage callbacks, in order to accurately reflect when each of these operations is being performed.


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

@charleskawczynski
Copy link
Member

I'm not opposed to these changes, but having moved these pieces around myself, I remember just how complex some of the edge cases are, and I'm a bit uneasy about the current test situation in the code. I'd really like to fix #106 so that it's simple and clear what is being changed here.

I don't want to slow down progress, so please feel free to merge if we get a green light on CI and atmos tests look fine. I'll bump the priority of fixing #106.

@dennisYatunin dennisYatunin force-pushed the dy/T_imp_approximation branch 4 times, most recently from c96efbb to 3eca3b3 Compare January 24, 2025 02:06
@dennisYatunin dennisYatunin changed the title Move T_imp approximation before DSS Add DSS before implicit solve, fix T_imp approximation and callbacks Jan 24, 2025
@dennisYatunin dennisYatunin force-pushed the dy/T_imp_approximation branch from 3eca3b3 to ccdcbde Compare January 24, 2025 02:08
@dennisYatunin dennisYatunin force-pushed the dy/T_imp_approximation branch from ccdcbde to 96b7067 Compare January 24, 2025 02:09
@dennisYatunin
Copy link
Member Author

@charleskawczynski, I have made some significant changes to the PR based on our AMIP tests (see PR description for details). In particular, some of the changes may affect your work on #323, so let me know if those make sense to you.

@charleskawczynski
Copy link
Member

In particular, some of the changes may affect your work on #323, so let me know if those make sense to you.

How would this affect #323?

@dennisYatunin
Copy link
Member Author

How would this affect #323?

This changes the order in which post_explicit! and post_implicit! are called. Before there was no meaningful pattern, but now post_explicit! is always called at the end of each stage (before explicit tendencies and implicit tendencies that are treated explicitly) and post_implicit! is always called before each newton iteration (before implicit tendencies that are treated implicitly).

@charleskawczynski
Copy link
Member

How would this affect #323?

This changes the order in which post_explicit! and post_implicit! are called. Before there was no meaningful pattern, but now post_explicit! is always called at the end of each stage (before explicit tendencies and implicit tendencies that are treated explicitly) and post_implicit! is always called before each newton iteration (before implicit tendencies that are treated implicitly).

Ok, in that case, can we please rename them to have pre-prefixes?

@dennisYatunin dennisYatunin force-pushed the dy/T_imp_approximation branch from 31acf25 to be13e01 Compare January 24, 2025 23:16
@dennisYatunin
Copy link
Member Author

Following an offline discussion with @charleskawczynski, I've renamed post_explicit! to cache! and post_implicit! to cache_imp!, and I've added a docstring for ClimaODEFunction to clarify when these are called and what default assumptions we make. I also renamed post_implicit!/call_post_implicit! in newtons_method.jl to prepare_for_f!, which clarifies how this callback is treated by JFNK and the standard Newton's method.

Since the keyword arguments for ClimaODEFunction have changed, this will be a breaking release for v0.8.0. This PR supercedes #323.

@dennisYatunin dennisYatunin force-pushed the dy/T_imp_approximation branch from dd59823 to a946b10 Compare January 25, 2025 00:20
@dennisYatunin dennisYatunin force-pushed the dy/T_imp_approximation branch from a946b10 to 9048ca6 Compare January 25, 2025 00:50
@dennisYatunin dennisYatunin merged commit 7b367ab into main Jan 25, 2025
8 of 9 checks passed
@dennisYatunin dennisYatunin deleted the dy/T_imp_approximation branch January 25, 2025 01:55
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.

2 participants