-
Notifications
You must be signed in to change notification settings - Fork 50
Introduce Thompson MP namelist options to adjust rain and snow max terminal fall speeds; Enable RRTMGP working with nesting; Introduce SASAS namelist options to control convective adjustment time #332
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
base: ufs/dev
Are you sure you want to change the base?
Introduce Thompson MP namelist options to adjust rain and snow max terminal fall speeds; Enable RRTMGP working with nesting; Introduce SASAS namelist options to control convective adjustment time #332
Conversation
nesting (moving-nesting) configurations, while reading in lw/sw_file_gas/clouds files.
options of tc_rain and tc_snow for adjustments of max terminal fall speeds of rain and snow under TC conditions.
dustinswales
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to RRTMGP look good to me. Thanks!
|
@AndersJensen-NOAA FYI. We will need to bring these HAFS changes introduced here into TEMPO to stay up-to-date |
| !> | ||
| subroutine rrtmgp_lw_cloud_optics_init(rrtmgp_root_dir, rrtmgp_lw_file_clouds, & | ||
| nrghice, mpicomm, mpirank, mpiroot, errmsg, errflg) | ||
| nrghice, mpicomm, mpirank, mpiroot1, errmsg, errflg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dustinswales @BinLiu-NOAA I am worried about this change. The host model defines mpiroot, not the scheme. You are hardcoding it in the RRTMGP scheme to be zero. This is a dangerous assumption in my opinion. Can you not just pass in the value you need (i.e. zero) from the host model?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@climbfuji I got a similar concern on this. @Qingfu-Liu and @dustinswales, not sure if it's easy to come up a more generalized solution for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is not an ideal solution. Sorry for not proposing something cleaner.
The problem was related to nested domains and initialization. Setting mpiroot=0 in the scheme avoided the situation where the nested domain(s) would try to reset this global data already being set by the parent domain.
@BinLiu-NOAA @climbfuji Hang tight and I'll work on a fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I just realized that the UWM PR does not add any new testing for the HAFS_v2 suites being introduced to NOAA-EMC/ufsatm.
@BinLiu-NOAA If you can you update the UWM PR to include a test, I can provide a more elegant solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dustinswales We do plan to update some existing HAFS RTs in UWM to be aligned with future HAFS v2.2 configurations/suites. However, these HAFS RTs related updates will most likely come from a later/future PR, since the HAFSv2.2 final configurations are still evolving (hopefully will be finalized in the next few weeks).
Meanwhile, definitely can provide you a canned test case for developing/testing alternative solutions before the HAFS RT update PR. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is not an ideal solution. Sorry for not proposing something cleaner.
The problem was related to nested domains and initialization. Setting mpiroot=0 in the scheme avoided the situation where the nested domain(s) would try to reset this global data already being set by the parent domain.
@BinLiu-NOAA @climbfuji Hang tight and I'll work on a fix
In terms alternative solutions, I recall @SamuelTrahanNOAA and @JiayiPeng-NOAA worked/implemented an approach for a similar issue related nesting/moving-nesting support for the stochastics physics. Please feel free to comment/suggest if you have some ideas. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dustinswales We do plan to update some existing HAFS RTs in UWM to be aligned with future HAFS v2.2 configurations/suites. However, these HAFS RTs related updates will most likely come from a later/future PR, since the HAFSv2.2 final configurations are still evolving (hopefully will be finalized in the next few weeks).
Meanwhile, definitely can provide you a canned test case for developing/testing alternative solutions before the HAFS RT update PR. Thanks!
@BinLiu-NOAA That works for me. (No rush as I will be off until 1/5/26.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed RE: the need for a more general solution. Do the nest physics always get initialized after the parent domain physics? If so, would a logical is_initialized-type variable in GFS_typedefs/control DDT do the trick? Or even a scheme module-level variable like is used in other schemes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@climbfuji @grantfirl We could just set Init_parm%master = 0 in fv3/atmos_model.F90:
#ifdef MOVING_NEST
Init_parm%master = 0
#endif
And revert all these changes to the GP scheme files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this completely, but what @grantfirl suggests seems to make the most sense - keep track of the initialization through a host model (or scheme module) variable?
physics/MP/Thompson/mp_thompson.F90
Outdated
| qiten3=qiten3, niten3=niten3, nrten3=nrten3, ncten3=ncten3, & | ||
| qcten3=qcten3, pfils=pfils, pflls=pflls) | ||
| qcten3=qcten3, pfils=pfils, pflls=pflls, & | ||
| tc_rain=tc_rain, tc_snow=tc_snow) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BinLiu-NOAA Just to clarify, if I set tc_rain = tc_snow = 1.0 in the host model (namelist or hardcoded), then this PR doesn't change the answer, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@climbfuji, As @WeiguoWang-NOAA replied down below, using the default settings of tc_rain, tc_snow=1.0 does not change the model answer.
|
tc_rain=tc_snow=1.0 does not change the answer. Default values=1.0
…On Tue, Dec 30, 2025 at 12:02 PM Dom Heinzeller ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In physics/Radiation/RRTMGP/rrtmgp_lw_cloud_optics.F90
<#332 (comment)>
:
> @@ -48,7 +48,7 @@ module rrtmgp_lw_cloud_optics
! ######################################################################################
!>
subroutine rrtmgp_lw_cloud_optics_init(rrtmgp_root_dir, rrtmgp_lw_file_clouds, &
- nrghice, mpicomm, mpirank, mpiroot, errmsg, errflg)
+ nrghice, mpicomm, mpirank, mpiroot1, errmsg, errflg)
@dustinswales <https://github.com/dustinswales> @BinLiu-NOAA
<https://github.com/BinLiu-NOAA> I am worried about this change. The host
model defines mpiroot, *not* the scheme. You are hardcoding it in the
RRTMGP scheme to be zero. This is a dangerous assumption in my opinion. Can
you not just pass in the value you need (i.e. zero) from the host model?
------------------------------
In physics/MP/Thompson/mp_thompson.F90
<#332 (comment)>
:
> @@ -858,7 +864,8 @@ subroutine mp_thompson_run(ncol, nlev, con_g, con_rd, &
tprv_rev=tprv_rev, tten3=tten3, &
qvten3=qvten3, qrten3=qrten3, qsten3=qsten3, qgten3=qgten3, &
qiten3=qiten3, niten3=niten3, nrten3=nrten3, ncten3=ncten3, &
- qcten3=qcten3, pfils=pfils, pflls=pflls)
+ qcten3=qcten3, pfils=pfils, pflls=pflls, &
+ tc_rain=tc_rain, tc_snow=tc_snow)
@BinLiu-NOAA <https://github.com/BinLiu-NOAA> Just to clarify, if I set
tc_rain = tc_snow = 1.0 in the host model (namelist or hardcoded), then
this PR doesn't change the answer, correct?
—
Reply to this email directly, view it on GitHub
<#332 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFA3EYHGOFS6OFXJWGIAVX34EKVZHAVCNFSM6AAAAACO34ZLL2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTMMJYHAYDKNJYGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
| qrten3, qsten3, qgten3, qiten3, niten3, & | ||
| nrten3, ncten3, qcten3 | ||
| ! TC adjustment | ||
| real(wp), INTENT (IN) :: tc_rain, tc_snow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be optional arguments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We just changed them to optional arguments.
| logical :: debug_flag | ||
| integer :: nu_c | ||
|
|
||
| real(wp) :: ymaxw, ytmp1 ! tc djustment tmp variable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ytmp1 should be renamed to fallspeed_adjustment_factor or something along those lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WeiguoWang-NOAA It is more meaningful to use the name @AndersJensen-NOAA suggested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We just changed that following your good suggestion.
| vtck(k) = 0. | ||
| vtnck(k) = 0. | ||
| enddo | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you don't want to repeat the maxval calculation you can do this once before line 3779:
ymaxw = 0.
if (ANY(L_qr .eqv. .true.) .or. ANY(L_qs .eqv. .true.)) ymaxw = maxval(w1d)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have removed the w-dependent adjustment part.
physics/MP/Thompson/mp_thompson.F90
Outdated
| real(kind=kind_phys), intent(inout), dimension(:,:), optional :: pfl_lsan | ||
|
|
||
| ! TC adjustment | ||
| real(kind_phys), intent(in) :: tc_rain, tc_snow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that these probably should be optional, here, and in module_mp_thompson.F90.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making these optional at the CCPP entry point and the underlying scheme module would require adding more logic to the section that calls mp_gt_driver -- one call with the new optional arguments for when they are present and one call without them with they are not present. Since the argument list is so long, this kinda makes the code uglier, IMO. I wonder if we could introduce local values of tc_rain and tc_snow that are set to 1.0 and always pass them in to mp_gt_driver()? Before the call, if tc_rain and tc_snow are present, set the local values from the input values, otherwise, they keep their default values. This would avoid adding separate calls to mp_gt_driver at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have updated the code and used a local variable, which is equal to 1.0 or equal to the input value if present.
physics/MP/Thompson/mp_thompson.meta
Outdated
| type = logical | ||
| intent = inout | ||
| [tc_rain] | ||
| standard_name = tc_adjustment_rain_fall_speed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkavulich May want to chime in with respect to these new standard names. In the past, we've used something like multiplicative_tuning_parameter_for_X. In this case, maybe multiplicative_tuning_parameter_for_rain_fall_speed makes sense? Although these are being added in a tropical cyclone context, I don't think that this is truly limited to only tropical cyclones, correct? So, I don't think we need to include tc in it (which is also an unclear abbreviation that isn't widely used, as far as I know). The same comment works for the snow fall speed below too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grantfirl I agree with your assessment, but since there will be a metadata overhaul soon I don't think it's necessary to hold up this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have changed the names following other similar variables using multiplicative_tuning_parameter_for_X
|
@BinLiu-NOAA @Qingfu-Liu I opened two PRs, one into this branch, and another into the UFSATM supermodule. These changes remove the local mpiroot variable and hardcoding of it to 0 and introduces initialization flags that are passed from the host. |
Add RRTMGP initialization flags and revert hardcoding of mpiroot
….meta so that convective adjustment time can be tunable, introducing two parameters cat_adj_deep and cat_adj_shal for this
|
Regarding the naming convention for related namelist variables and some related variables, @WeiguoWang-NOAA is making some updates as suggested by reviewers, we will try to commit the changes/updates in this PR branch tomorrow (Friday). Thanks! |
rain and snow in Thompson MP scheme
RuiyuSun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look fine to me
Description of Changes:
One or more paragraphs describing the problem, solution, and required changes.
This pull request try to bring the following HAFS related physics updates/changes back to the ufs/dev branch.
Tests Conducted:
Explicitly state what tests were run on these changes, or if any are still pending (for README or other text-only changes, just put "None required". Make note of the compilers used, the platform/machine, and other relevant details as necessary. For more complicated changes, or those resulting in scientific changes, please be explicit!
OR Add any links to tests conducted. For example, "See ufs-community/ufs-weather-model#<pr_number>"
Technical and retrospective tests have been conducted with the UFS hurricane application (HAFS).
Dependencies:
Add any links to parent PRs (e.g. SCM and/or UFS PRs) or submodules (e.g. rte-rrtmgp). For example:
Documentation:
Does this PR add new capabilities that need to be documented or require modifications to the existing documentation? If so, brief supporting material can be provided here. Contact the CODEOWNERS if your PR requires extensive updates to the documentation. See https://github.com/NCAR/ccpp-doc for Technical Documentation or https://dtcenter.org/community-code/common-community-physics-package-ccpp/documentation for the latest Scientific Documentation.
N/A
Issue (optional):
If this PR is resolving or referencing one or more issues, in this repository or elewhere, list them here. For example, "Fixes issue mentioned in #123" or "Related to bug in NCAR/other_repository#123"
Contributors (optional):
If others have contributed to this work aside from the PR author, list them here
@WeiguoWang-NOAA, @Qingfu-Liu, @dustinswales, @JunghoonShin-NOAA, et al.