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.

Sign up for GitHub

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

Jump to bottom

fix diagnostic PBL and convective tendencies #489

Merged
merged 9 commits into from
Sep 30, 2020
Merged

fix diagnostic PBL and convective tendencies #489

merged 9 commits into from
Sep 30, 2020

Conversation

grantfirl
Copy link
Collaborator

@grantfirl grantfirl commented Aug 18, 2020 •
edited by climbfuji
Loading

This PR fixes several tendencies within several different PBL schemes [hybrid EDMF, operational sa-moist-TKE-EDMF, MYNN, YSU, and Shin-Hong (AKA sa-YSU)]

Please see issue #490 for details on what was fixed.

Edit:

  • PBL tendencies were added for the SHOC and MYJ schemes. Generic tendency logical added to satmedmfvdif (non-Q).
  • tendencies for G-F convection were fixed (they were not multiplying by a factor)
  • minor bugfix in MYNN (double operator that the compiler complained about)

Associated PRs:

NOAA-EMC/GFDL_atmos_cubed_sphere#40
#489
NOAA-EMC/fv3atm#178
ufs-community/ufs-weather-model#208

For regression testing information, see ufs-community/ufs-weather-model#208.

@grantfirl
fix diagnostic tendencies in moninedmf.f, satmedmfvdifq.F, module_MYN…
43ba47d 
…NPBL_wrapper.F90, ysuvdif.F90, shinhongvdif.F90
@grantfirl
Copy link
Collaborator Author

Addresses #490

@grantfirl
Copy link
Collaborator Author

Diagnostic tendencies before the fix for the TWPICE case:
profiles_mean_dq_dt_PBL.pdf
profiles_mean_dT_dt_PBL.pdf
profiles_mean_du_dt_PBL.pdf
profiles_mean_dv_dt_PBL.pdf

Same for the ARM_SGP_summer_1997_A case:
profiles_mean_dq_dt_PBL.pdf
profiles_mean_dT_dt_PBL.pdf
profiles_mean_du_dt_PBL.pdf
profiles_mean_dv_dt_PBL.pdf

@grantfirl
Copy link
Collaborator Author

Diagnostic tendencies AFTER the fix for the TWPICE case:
profiles_mean_dq_dt_PBL.pdf
profiles_mean_dT_dt_PBL.pdf
profiles_mean_du_dt_PBL.pdf
profiles_mean_dv_dt_PBL.pdf

Same for the ARM_SGP_summer_1997_A case:
profiles_mean_dq_dt_PBL.pdf
profiles_mean_dT_dt_PBL.pdf
profiles_mean_du_dt_PBL.pdf
profiles_mean_dv_dt_PBL.pdf

@grantfirl
Copy link
Collaborator Author

Note to self: Check other non-operational PBL schemes: satmedmfvdif (non-Q version), SHOC, MYJ

@ligiabernardet
Copy link
Collaborator

@SamuelTrahanNOAA Since you did the original implementation, could you please take a look at this PR that extends and fixes the tendencies output?

@SamuelTrahanNOAA
Copy link
Collaborator

Do the tendencies of each tracer for all schemes add up to the total physics tendency for each tracer?

Plotting the diagnostic tendencies doesn't prove anything. If the physics scheme itself was broken, then the ugly diagnostic tendencies may have been an accurate representation of the scheme's incorrect tendencies.

@grantfirl
Copy link
Collaborator Author

Do the tendencies of each tracer for all schemes add up to the total physics tendency for each tracer?

Plotting the diagnostic tendencies doesn't prove anything. If the physics scheme itself was broken, then the ugly diagnostic tendencies may have been an accurate representation of the scheme's incorrect tendencies.

@SamuelTrahanNOAA While it's true that plotting just the PBL tendencies before and after a fix doesn't "prove" anything, a look at the "before" plots does point out that 1) several schemes do not calculate PBL tendencies and 2)some that do produce tendencies don't make sense and warrant a look at the code. After said look at the code, it was obvious that there were several issues in the calculation of the actual diagnostic, not necessarily the scheme behavior. For example, the moninedmf scheme was incorrectly dividing by timestep twice, that the moninedmf and MYNN schemes were erroneously subtracting out changes due to radiation in the T-tendencies, and that the operational scheme was erroneously multiplying by a scalar parameter related to TKE-dissipative heating in both the T- and q-tendencies.

The "after" plots show non-zero tendencies for schemes where I added code to calculate tendencies, and the tendencies look physically realistic, for the most part. But, like you said, this is progress, not proof.

Following your suggestion, I went to plot a residual for u, v, T, and qv tendencies defined by:

X_res = X_phys - (X_scheme1 + X_scheme2 + ... + X_schemeN)

where X represents the tendency of u, v, T, or qv, the X_phys term represents the tendency due to all physics processes, and X_schemeY is the tendency due to process Y. Unfortunately, this doesn't help because X_phys is defined in the new phys_tend.F90 as:

X_phys = X_scheme1 + X_scheme2 + ... + X_schemeN

IMO, this formulation hides errors if any of X_schemeY is in error. If the X_phys terms were instead calculated as:

X_phys = X1-X0

where X0 is the value of X saved at the beginning of physics, and X1 is the value of X saved at the end of physics. I switched to this formulation and then plotted X_res for a given case for the original (buggy) code:

profiles_mean_dq_dt_residual.pdf
profiles_mean_dT_dt_residual.pdf
profiles_mean_du_dt_residual.pdf
profiles_mean_dv_dt_residual.pdf

I repeated the plots after the PBL tendency fixes:

profiles_mean_dq_dt_residual.pdf
profiles_mean_dT_dt_residual.pdf
profiles_mean_du_dt_residual.pdf
profiles_mean_dv_dt_residual.pdf

Looking good, except for the GSD suite now. The profile of the residual points to the convective scheme, so I looked in cu_gf_driver.F for the G-F convective scheme and found another bug. The diagnostic tendencies lacked the same multipliers found in the code when the state is actually updated. Adding these in, I now get the following residuals:

profiles_mean_dq_dt_residual.pdf
profiles_mean_dT_dt_residual.pdf
profiles_mean_du_dt_residual.pdf
profiles_mean_dv_dt_residual.pdf

Again, looking better. The residuals are continually shrinking as bugs are found. The method appears to work. At this point, I can identify that the remaining qv-tendency residual is due to a positive-definite statement for qv when qv is updated. The other residuals are unknown at this point. @ligiabernardet Do you think that it makes sense to continue tracking down these residuals in the tendencies until they are all acceptably close to 0, or are they close enough as they are? IMO, the last set of plots show that the tendency errors have been reduced by several orders of magnitude and that they can likely safely be used at this point. It might be worth tracking down, just to understand where the remaining residuals are coming from.

@ligiabernardet
Copy link
Collaborator

ligiabernardet commented Aug 20, 2020 via email

@grantfirl
Copy link
Collaborator Author

OK, I'm regression testing this now. (minus WW3 tests that are not compiling)

@grantfirl
Copy link
Collaborator Author

grantfirl commented Aug 21, 2020 •
edited
Loading

This passed all regression tests except for the expected one: fv3_ccpp_gsd_diag3d_debug (the diagnostic tendencies have been corrected)

rt_full_minus_ww3.log

The tests with WW3=Y have been skipped due to ufs-community/ufs-weather-model#189. I have no idea why I can't compile with WW3 but Jun seems to be able to fine.

@grantfirl grantfirl marked this pull request as ready for review August 24, 2020 15:10
@grantfirl
Copy link
Collaborator Author

grantfirl commented Aug 24, 2020 •
edited
Loading

I'm re-running the WW3-based tests, now that there is a fix for ufs-community/ufs-weather-model#189

rt_ww3_only.log

@grantfirl grantfirl changed the title fix diagnostic PBL tendencies fix diagnostic PBL and convective tendencies Sep 3, 2020
@climbfuji
Copy link
Collaborator

@grantfirl this PR is next in the commit queue. It will be combined with a small change from Moorthi for fv3atm. I will orchestrate the fv3atm PR, pulling in your changes and Moorthi's changes. But for this ccpp-physics PR, can I ask you to (a) update your code to the head of the master branch, and (b) make the following change, please?

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 06ca63d..a18f0b0 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -212,7 +212,9 @@ elseif (${CMAKE_Fortran_COMPILER_ID} STREQUAL "Intel")
     endif()

     # Remove files with special compiler flags from list of files with standard compiler flags
-    list(REMOVE_ITEM SCHEMES ${SCHEMES_SFX_OPT})
+    if (SCHEMES_SFX_OPT)
+      list(REMOVE_ITEM SCHEMES ${SCHEMES_SFX_OPT})
+    endif(SCHEMES_SFX_OPT)
     # Assign standard compiler flags to all remaining schemes and caps
     SET_SOURCE_FILES_PROPERTIES(${SCHEMES} ${CAPS}
                                 PROPERTIES COMPILE_FLAGS "${CMAKE_Fortran_FLAGS_OPT}")

Thanks!

@grantfirl
Copy link
Collaborator Author

@grantfirl this PR is next in the commit queue. It will be combined with a small change from Moorthi for fv3atm. I will orchestrate the fv3atm PR, pulling in your changes and Moorthi's changes. But for this ccpp-physics PR, can I ask you to (a) update your code to the head of the master branch, and (b) make the following change, please?

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 06ca63d..a18f0b0 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -212,7 +212,9 @@ elseif (${CMAKE_Fortran_COMPILER_ID} STREQUAL "Intel")
     endif()

     # Remove files with special compiler flags from list of files with standard compiler flags
-    list(REMOVE_ITEM SCHEMES ${SCHEMES_SFX_OPT})
+    if (SCHEMES_SFX_OPT)
+      list(REMOVE_ITEM SCHEMES ${SCHEMES_SFX_OPT})
+    endif(SCHEMES_SFX_OPT)
     # Assign standard compiler flags to all remaining schemes and caps
     SET_SOURCE_FILES_PROPERTIES(${SCHEMES} ${CAPS}
                                 PROPERTIES COMPILE_FLAGS "${CMAKE_Fortran_FLAGS_OPT}")

Thanks!

OK, I'm working on this now.

@grantfirl
Copy link
Collaborator Author

@climbfuji Finished.

@climbfuji
Copy link
Collaborator

@climbfuji Finished.

Great, thank you. Will make this PR part of my ufs-weather-model and fv3atm PRs.

@climbfuji climbfuji mentioned this pull request Sep 28, 2020
Merged
This was referenced Sep 28, 2020
Merged
Merged
climbfuji
climbfuji approved these changes Sep 29, 2020
View reviewed changes
Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

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

Thanks for making the additional change in CMakeLists.txt. I will merge the PR once all testing is done.

@climbfuji climbfuji merged commit 6f12e14 into NCAR:master Sep 30, 2020
@climbfuji climbfuji mentioned this pull request Sep 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@climbfuji climbfuji climbfuji approved these changes

@JulieSchramm JulieSchramm Awaiting requested review from JulieSchramm

@llpcarson llpcarson Awaiting requested review from llpcarson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@grantfirl @ligiabernardet @SamuelTrahanNOAA @climbfuji