Update Thompson MP and RUC LSM from gsl/develop #600
Conversation
Update gsl/develop from master 2021/02/22
computation of number concentration of liquid water. 2. Replaced dry-air density with the moist-air density for consistency with Thompson MP.
…into thomp_nc_density
Fix gthe units of water-friendly aerosols in the call to make_DropletNumber.
Thomp nc density fix submodule pointer rrtmgp
…GFS_suite_interstitial.F90 and physics/GFS_rrtmgp_thompsonmp_pre.F90
Bugfixes for thomp_nc_density
Remove inconsistencies in computation of air density with Thompson MP
…psnWRF/ccpp-physics into update_thompsonMP_20210213_merged_into_gsl_develop
…aware version (nc not allocated), reduce noise written to stdout
…it -> init_thompson, apply bounds after calls to calc_effectRad in radiation, set intent(out) variables in calc_effectRad
…tialize cloud effective radii
…hys_time_vary.scm.F90
…instead of aborting the model run
|
This also contains #586. |
| @@ -1206,7 +1221,7 @@ SUBROUTINE mp_gt_driver(qv, qc, qr, qi, qs, qg, ni, nr, nc, & | |||
| m = RSHIFT(ABS(rand_perturb_on),1) | |||
| if (MOD(m,2) .ne. 0) rand2 = rand_pert(i,1,j)*2. | |||
| m = RSHIFT(ABS(rand_perturb_on),2) | |||
| if (MOD(m,2) .ne. 0) rand3 = 0.1*(rand_pert(i,1,j)+ABS(min_rand)) | |||
| if (MOD(m,2) .ne. 0) rand3 = 0.25*(rand_pert(i,1,j)+ABS(min_rand)) | |||
There was a problem hiding this comment.
If the 0.25 value is subject to change across versions of this code, would it make sense to define a variable or constant for it?
There was a problem hiding this comment.
This is just a scale factor to push the values of the random variable to be produce more impactful results for this perturbation. It has only appeared in the original version I wrote and after the study was done, the value was increased. I don't foresee this value being manipulated in version after version. The same basic point could be made about the item two lines higher up.
There was a problem hiding this comment.
I'll approve if the PR originators have already regression tested and want to merge, but in my opinion, it would be best to use a consistent value of the "eps" physical constant when doing the density calculations in this PR. Also, there were several instances, especially in module_mp_thompson.F90 where hard-coded constants are changed between the new version and old. From a future maintenance (and code understandability) point-of-view, if these values are changing between versions, that is a pretty good sign that they should be made variables or parameters rather than being hard-coded. But, that is more of a style thing, so it is up to the author.
Some parameters being hard-coded, particularly all those related to radiative effective radius is an outcome of this PR - done exactly for the reasons you suggest: avoid changing one but forgetting in another place. But I don't see a need to go overboard with the idea where it isn't really needed. Constants are visible and their sources are quite often known, such as the 0.622 value itself. |
|
Thank you all for your reviews. Regarding the discussion around 0.622, what would be an acceptable name for a constant? I know that the greek epsilon is widely used for Rd/Rv, but it is also used in many other cases. I don't have a strong preference wha to use, whether to change at all or not. |
The constant already exists in both FV3 and the SCM and is called "ratio_of_dry_air_to_water_vapor_gas_constants". What it is called locally is entirely up to the authors, but I have always seen it given the greek letter epsilon in textbooks, etc. If we don't change the code to use the pre-defined constant, then we need to provide a valid reason why the conversion used in this PR should use a different value for this constant than the rest of physics. I mean, the discussion is a little silly, because the difference in practice comes down to like the 4th significant digit, but the principle of physics using a single, consistent set of physical constants is still a good one, in my opinion, and we need to stick to it where we can. |
|
Sorry, accidentally pushed the close button. |
|
@DomHeinzeller @grantfirl I think we could keep 0.622 for now. If we want to make a variable for Rd/Rv, them for consistency we'll have to change many physics subroutines (you could grep for 0.622 in the ccpp/physics/physics to see how widely this number is used). |
@tanyasmirnova In my opinion, just because the inconsistency exists in other physics doesn't mean we need to continue to propagate it with new code. You could also search for "ratio_of_dry_air_to_water_vapor_gas_constants" in the same directory and see how often it is used. The variable already exists. It's just a matter of passing it in and using it, which is consistent with the CCPP rules of using one, consistent set of physical constants that is controlled by the host application. |
|
@grantfirl You are right. The variable "ratio_of_dry_air_to_water_vapor_gas_constants' does exist and also widely used. And it has a name in GFS_typedefs.meta -> con_eps. I think we could use this con_eps. |
Ok, I'll make this change before we kick off the baseline creation. That will be quite a few more files with changes. |
|
Note. I'll shortly push a commit that replaces several of the hardcoded 0.622 with |
Thanks for doing that @climbfuji. To be clear, I was only referring to changing it within this PR code since inconsistencies with this and other constants exist throughout the CCPP physics, like you said, mostly due to legacy reasons and needing to maintain b4b with non-CCPP physics for so long. Ideally, Xia would find/fix these kinds of things as part of her physical constants work, right? |
These 0.622 values escaped my attention. I will find/fix those values. Thanks. |
May I ask to first create an issue in this repository? I also think it is better to wait with fixing this, right now we have a large number of commits with many changes, we need to focus on transitioning to |
|
That makes sense. Thanks Dom. |
* Enhanced stability of drag_suite module and added diagnostics * Updated standard_names for do_gsl_drag_ls_bl, do_gsl_drag_ss, do_gsl_drag_tofd and ugwp_seq_update * Assumed-shape in gwdps.f * Added logical flag 'ugwp_seq_update' to gfs_physics_nml namelist * Modified calculation of dtfac in drag_suite.F90 and revised various long_name's of diagnostic variables * Removed passage of variables 'ak' and 'bk' to drag_suite -- these are no longer used * Removed unused variables from GFS_typedefs.F90 and added clarifying comments in ccpp-physics
This PR contains the following changes:
-init=snan,arraysto Intel DEBUG compiler flags for ccpp-physics in fv3atmcon_eps(= Rd/Rv) in CCPP entry points and several other placesphysics/GFS_rrtmgp_thompsonmp_pre.metaFixes #590.
Associated PRs:
NCAR/ccpp-framework#357
#600
NOAA-EMC/fv3atm#267
ufs-community/ufs-weather-model#498
For regression testing, see ufs-community/ufs-weather-model#498