RRTMGP w/ GFS SDFs + some housekeeping #446
Conversation
dustinswales
requested review from
climbfuji,
grantfirl,
JulieSchramm and
llpcarson
as code owners
|
These changes look fine to me at first look. |
|
I think that all these PRs (#446, NCAR/fv3atm#46, PR for ufs-weather-model missing, NCAR/ccpp-scm#181) should go to NCAR ccpp-physics master, NOAA-EMC fv3atm develop, ufs-community ufs-weather-model develop, and NCAR gmtb-scm master, now that RRTMGP is part of the authoritative repositories. |
|
This may also require that someone updates gmtb-scm master to work with NCAR ccpp-physics master and NCAR ccpp-framework master (unless this has been done already). |
NCAR/ccpp-scm#182 now exists which updates gmtb-scm to work with masters of ccpp-physics/framework. |
|
An update on this PR: To test this with the SCM, I just merged in masters of gmtb-scm and ccpp-physics to their respective PRs from Dustin and ran one case (TWPICE) with the GFSv15p2 suite against the GFSv15p2_RRTMGP suite. The test completely successfully (a few plots attached for the deep convective period). @dustinswales , @climbfuji I'm not 100% up-to-date on the status of masters vs dtc/develop, but is it OK to merge in masters to Dustin's PRs or do his changes need to be moved over to start from masters instead of dtc/develop? profiles_bias_T.pdf |
|
For ccpp-physics, just change the base (target branch), merge in the current master and push to the PR. For fv3atm and ufs-weather-model, the PRs need to be recreated, because they go to a different fork (and the commit history is slightly different). Also, EMC requires all PRs to be up to date with the current version of the code (for good reasons). dtc/develop is now three or so commits behind develop (master), hence pulling in the latest versions to update the PRs is necessary. |
…w cloud-overlap scheme, not complete.
Dtc/develop
modified file
physics/drag_suite.F90
physics/module_sf_noahmp_glacier.f90
The codes have been tested on Hera.
Modified codes:
physics/drag_suite.F90
physics/module_sf_noahmp_glacier.f90
…ing over to SCM for debugging.
… the RRTMGP radiation scheme. This is based on NCAR/ccpp-physics PR 477, but for the GP scheme.
There was a problem hiding this comment.
I am having a bit of a hard time to understand what the PR is doing with the existing Zhao-Carr MP - is it replacing the Zhao-Carr MP hooks with the ones for GFDL-MP, or adding GFDL-MP to the existing Zhao-Carr MP support. As such, I can't really judge how much code is duplicated. I flagged a view issues from a CCPP perspective.
physics/GFS_cloud_diagnostics.F90
Outdated
| ! ######################################################################################## | ||
| module GFS_cloud_diagnostics | ||
| use machine, only: kind_phys | ||
| use physcons, only: con_pi, con_rog, decorr_con |
There was a problem hiding this comment.
these constants should come in via the argument list
There was a problem hiding this comment.
Really? I need to pass around physical constants such as pi? I understand the decorr_con, as it's not a physical constant, but rather an algorithmic parameter.
There was a problem hiding this comment.
Yes, "unfortunately" constants shall be defined by the host model and passed in via the argument list as per CCPP requirements. I know this makes less sense for con_pi, but it does make sense for other constants. As far as I am concerned, you can leave pi for this PR but pass the other two via the argument list.
There was a problem hiding this comment.
I will eradicate them all in my next commit.
|
|
||
| ! ######################################################################################### | ||
| ! ######################################################################################### | ||
| subroutine gethml(plyr, ptop1, cldtot, cldcnv, dz, de_lgth, alpha, IX, NLAY, clds, mtop, mbot) |
There was a problem hiding this comment.
Is this a duplication of code? There is already a gethml in the existing RRTMG code.
There was a problem hiding this comment.
This is.
But this brings up a larger question about organization that I'd like to get your feedback on.
In RRTMG gethml() is called from within each progcldX(), but it's outputs are cloud-diagnostics and don't have any impact on the radiation. These cloud diagnostics are dependent on the cloud microphysics choices provided by the model, but not the radiation calculation, so I isolated them to a single suite/scheme for GP.
So yes the gethml() is redundant and I can (should) pull it over with a "use" statement from radiation_clouds.f. But I also want for GP to stand on its own and not be dependent on code from RRTMG. I know having two versions of something is a terrible nasty horrible idea, so maybe it's best to use the G infrastructure in GP when applicable and make note.
There was a problem hiding this comment.
I don't have any good answer here. There is a need for a greater cleanup of radiation codes and radiation-microphysics interaction, in a way that works with both RRTMG and RRTMGP. You have two options, keep the separate gethml as you wrote it, or take gethml out of radiation_clouds.f and put it in its own module. I think we can save that for later, so maybe just leave it as it is now.
| ! Store surface temperature for next iteration | ||
| sktp1r(:) = skt(:) | ||
| else | ||
| if (frac_grid) then |
There was a problem hiding this comment.
Where is this block of code comping from? Was this in GFS_composites_{pre,inter}?
There was a problem hiding this comment.
It's new. The code in the "if" conditional is new, whereas the code in the "else" was already there.
As an option, RRTMGP can output the Jacobian of the upwelling LW fluxes, which can be used to adjust the upwelling surface LW radiation in between time-steps. For RRTMG it is a bit more involved.
There was a problem hiding this comment.
Weird. The GitHub diff view is telling me that the code between lines 256 and 294 is new.
There was a problem hiding this comment.
Ok, I finally see the problem. You replicated the block in lines 256 to 293 (it is identical to the block in lines 295 to 331). We shouldn't change this as part of this PR anymore, rather I create an issue and fix it next time. Question is if the part in the "else" block should remain (256-293) or the one entirely outside (295-331). We want to make sure that RRTMGP will also work with the fractional grid. See issue #485
| @@ -137,6 +137,9 @@ module physcons | |||
| real(kind=kind_phys),parameter:: rhosnow = 100._kind_phys !< density of snow (kg/m^3) | |||
| real(kind=kind_phys),parameter:: rhoair = 1.28_kind_phys !< density of air near surface (kg/m^3) | |||
|
|
|||
| ! Decorrelation length constant (km) for iovrlw/iovrsw = 4 or 5 and idcor = 0 | |||
There was a problem hiding this comment.
Please define the metadata for this variable in GFS_typedefs.meta in fv3atm so that it can be passed to CCPP schemes via the argument list (rather than being imported).
There was a problem hiding this comment.
Will do.
FYI. All of this business for the decorrelation length constant is a recent change for new cloud overlap assumptions in RRTMG, one you may see again #477
There was a problem hiding this comment.
I know, and it's causing me quite some headache.
| module rrtmgp_gfdlmp_lw_cloud_sampling | ||
| use machine, only: kind_phys | ||
| use mo_gas_optics_rrtmgp, only: ty_gas_optics_rrtmgp | ||
| use physparam, only: isubclw, iovrlw |
There was a problem hiding this comment.
We should pass those physparam variables via the argument list.
| module rrtmgp_gfdlmp_sw_cloud_sampling | ||
| use machine, only: kind_phys | ||
| use mo_gas_optics_rrtmgp, only: ty_gas_optics_rrtmgp | ||
| use physparam, only: isubcsw, iovrsw |
There was a problem hiding this comment.
Ditto. Via the argument list.
…atements and added physical/algorithmic constants to the Interstitial type.
|
@climbfuji Here's what I did in the SCM to add physical constants as interstitial fields: But before I do this for all of the other GP modules, let me know if this seems reasonable/crazy... |
@dustinswales there is an easier way. In SCM, the physical constants and their metadata are defined in |
…re also passed via argument lists in the RRTMGP suite level files (still need to add constants to scheme level code).
I figured this much. Thanks. I removed all of the GFS DDTs from the RRTMGP suite level code, and added physical constants to the arguments lists. I still have a bit more to do. |
|
@dustinswales Thanks for working on my comments and suggestions. Please let me know when you think the code is ready to be reviewed again. Thanks! |
…ve been added to the argument lists. Some cleanup.
|
@climbfuji |
Thanks, @dustinswales . Yes, if we can leave out the Diag%fluxr array for the moment, that will be best. I will look at the code changes again in ccpp-physics. Please also let me know when you are done with the fv3atm and ufs-weather-model PRs. I need to run the regression tests to make sure that only the RRTMGP tests change. |
|
|
||
| ! ######################################################################################### | ||
| ! ######################################################################################### | ||
| subroutine gethml(plyr, ptop1, cldtot, cldcnv, dz, de_lgth, alpha, IX, NLAY, clds, mtop, mbot) |
There was a problem hiding this comment.
I don't have any good answer here. There is a need for a greater cleanup of radiation codes and radiation-microphysics interaction, in a way that works with both RRTMG and RRTMGP. You have two options, keep the separate gethml as you wrote it, or take gethml out of radiation_clouds.f and put it in its own module. I think we can save that for later, so maybe just leave it as it is now.
physics/GFS_rrtmgp_gfdlmp_pre.F90
Outdated
| ! Subroutine to compute cloud-overlap parameter, alpha, for decorrelation-length cloud | ||
| ! overlap assumption. | ||
| ! ######################################################################################### | ||
| subroutine get_alpha_dcorr(nCol, nLev, lat, deltaZ, de_lgth, cloud_overlap_param) |
There was a problem hiding this comment.
Similar to gethtml, Mike Iacono added a get_alpha to radiation clouds. We should take both out and move in one separate module after this commit.
| enddo | ||
| enddo | ||
| endif | ||
| if (save_diag) then |
There was a problem hiding this comment.
@dustinswales @grantfirl I the Diag%flur array has now been commented out. We should discuss if it is "acceptable" to pass in the entire fluxr(:,:) array with one standard name or if this should be broken up into 40-ish individual components (seems crazy to me). Grant, you are working on removing the dependency on the GFS DDTs from RRTMG - what is your plan?
There was a problem hiding this comment.
In the PR that I started months ago, I apparently hadn't gotten to handling diag%fluxr yet. As distasteful as it is, IF ALL 40ish COMPONENTS ARE STILL NEEDED for diagnostics, and if they all represent different physics quantities, I don't think that we have a choice other than to split them up. Why would we treat the ridiculous proliferation of diagnostic quantities in radiation any different than other physics and break the CCPP rules? Or, maybe we say that diagnostics don't need to follow the rules? We already have user-configurable aux2d, aux3d, etc. I think the difference here is that the indices in fluxr are hard-coded right now, whereas variables in aux2d (potentially) aren't?
We may need to punt until we think about this some more as part of the capgen.py cleanup. As long as arrays like these 1) aren't used elsewhere in physics and 2) aren't DDTs that require the host model to know about them, I suppose what harm are they doing to interoperability?
| ! Store surface temperature for next iteration | ||
| sktp1r(:) = skt(:) | ||
| else | ||
| if (frac_grid) then |
There was a problem hiding this comment.
Weird. The GitHub diff view is telling me that the code between lines 256 and 294 is new.
| @@ -137,6 +137,9 @@ module physcons | |||
| real(kind=kind_phys),parameter:: rhosnow = 100._kind_phys !< density of snow (kg/m^3) | |||
| real(kind=kind_phys),parameter:: rhoair = 1.28_kind_phys !< density of air near surface (kg/m^3) | |||
|
|
|||
| ! Decorrelation length constant (km) for iovrlw/iovrsw = 4 or 5 and idcor = 0 | |||
There was a problem hiding this comment.
I know, and it's causing me quite some headache.
| ! Store surface temperature for next iteration | ||
| sktp1r(:) = skt(:) | ||
| else | ||
| if (frac_grid) then |
There was a problem hiding this comment.
Ok, I finally see the problem. You replicated the block in lines 256 to 293 (it is identical to the block in lines 295 to 331). We shouldn't change this as part of this PR anymore, rather I create an issue and fix it next time. Question is if the part in the "else" block should remain (256-293) or the one entirely outside (295-331). We want to make sure that RRTMGP will also work with the fractional grid. See issue #485
This PR is an accumulation of recent changes to RRTMGP in the ccpp-physics.
*) New suite definition files using GFDL microphysics enabled with RRTMGP: GFS_FV3_v15p2_RRTMGP and GFS_FV3_v16beta_RRTMGP
*) Changes to namelist. New switches for the cloud optical scheme. Ability to use the Jacobian of the upwelling longwave surface radiation for flux adjustments between radiation calls.
*) Cleaner separation between GFDL MP and RRTMGP scheme. The cloud sampling routines were renamed to reflect this new organization. Cloud (and precipitation) overlap parameters are accessible using the interstitial type and passed INTO the cloud sampling routines. Currently precipitation is sampled in the same manner as clouds, but this can/should change.
*) Propagate heating tendency naming change from #179 to GP routines.
*) physics/ submodule rte-rrtmgp was recently updated. The RRTMGP gas-optics initialization schemes needed to be modified to work w/ new routine interfaces. There is also additional information contained within the rrtmgp data files that needed to be ingested by the initialization routines. (This currently live in it's own branch on the rte-rrtmgp repo, ccpp-dev, merge into ccpp branch when complete.)
*) Bug fix for band ordering in SW cloud-optics. RRTMGP expects the SW bands to be ordered monotonically, whereas this convention does not hold true in RRTMG.