Feature/gfdlmpv3 (ready for review) #195
Conversation
… into a subdir v1
… the associated files
… into feature/gfdlmpv3
… into feature/gfdlmpv3
dustinswales
requested review from
grantfirl,
Qingfu-Liu and
mzhangw
as code owners
|
@dustinswales This is great and thanks!! I agree that the scheme name in SDF and different namelist name are enough to distinguish the two versions of the scheme. So I think it is good idea to remove the imp_physics_gfdl_v3 flag. Did you forget to remove the flag in the gfdl_cloud_microphysics_v3.F90 file? What is next? |
|
@RuiyuSun Good call. I just removed the flag from the scheme. Next we need to test that the RTs that use GFDLMP v1 are unchanged (I can do this next week). I can't speak to how the v3 results should differ wrt v1 (I will leave this piece to you) |
@dustinswales Sounds good. |
|
@dustinswales How do I check out this code? Thanks! |
git clone --recursive --branch feature/gfdlmpv3 https://github.com/dustinswales/ufs-weather-model.git |
|
@grantfirl This is ready for review. |
… into feature/gfdlmpv3
dustinswales
changed the title
… into feature/gfdlmpv3
| ! GFDL MP common (v1/v3) parameters, with different default values | ||
| ! ##################################################################################### | ||
| #ifdef GFDLMP_V3 | ||
| real(kind_phys) :: tice = 273.15 !< freezing temperature (K): ref: GFDL, GFS (DJS: V3=273.15) |
There was a problem hiding this comment.
Why does this need to be redefined from the one that is already provided by the host (temperature_at_zero_celsius)? Does it really make that much of a difference to 1) use a different value for each version and 2) use something different than the host and rest of physics?
|
|
||
| implicit none | ||
|
|
||
| ! DH* TODO: CLEANUP, all of these should be coming in through the argument list |
There was a problem hiding this comment.
Yes, and consider the comment RE: temperature_at_zero_celsius
| do k=1,levs | ||
| kk = levs-k+1 | ||
| do i=1,im | ||
| if (imp_physics==imp_physics_gfdl) then |
There was a problem hiding this comment.
Why is this necessary? Shouldn't the check above already have been executed and returned with an error if imp_physics is wrong?
| !*********************************************************************** | ||
| !* GNU Lesser General Public License | ||
| !* | ||
| !* This file is part of the FV3 dynamical core. |
There was a problem hiding this comment.
It's a little weird to have this in a physics scheme in the CCPP, but I'm guessing that it's not our call.
| ! physics constants | ||
| ! ----------------------------------------------------------------------- | ||
|
|
||
| real, parameter :: grav = 9.80665 ! acceleration due to gravity (m/s^2), ref: IFS |
There was a problem hiding this comment.
Yikes. Lots of redefining constants here. Why does GFDL MP need to have a different gravity or pi than the host or rest of physics?
| real, parameter :: eps = rdgas / rvgas ! 0.6219934994582882 | ||
| real, parameter :: epsm1 = rdgas / rvgas - 1. ! -0.3780065005417118 | ||
|
|
||
| real, parameter :: tice = 273.15 ! freezing temperature (K): ref: GFDL, GFS |
There was a problem hiding this comment.
This is double redefining. It was redefined in the GFDL MP specific module then redefined here. Why?
| ! major cloud microphysics driver | ||
| ! ----------------------------------------------------------------------- | ||
|
|
||
| call mpdrv (hydrostatic, ua, va, wa, delp, pt, qv, ql, qr, qi, qs, qg, qa, & |
There was a problem hiding this comment.
This is another scheme with 3 layers of drivers. Not ideal, but I assume that there is a good reason to keep all of them?
| ! unit convert to mm/day | ||
| ! ----------------------------------------------------------------------- | ||
|
|
||
| convt = 86400. * rgrav / dts |
There was a problem hiding this comment.
redefining # of seconds in a day
| if (consv_checker) then | ||
| if (abs (sum (te_end_d (i, :)) + te_b_end_d (i) - sum (te_beg_d (i, :)) - te_b_beg_d (i)) / & | ||
| (sum (te_beg_d (i, :)) + te_b_beg_d (i)) .gt. te_err) then | ||
| print*, "GFDL-MP-DRY TE: ", & |
There was a problem hiding this comment.
Consider wrapping print statements with a debug flag to reduce standard out spam for normal execution.
| case ("qg") | ||
| q = qg | ||
| case default | ||
| print *, "gfdl_mp: qflag error!" |
There was a problem hiding this comment.
Should one pass down and set ccpp_err_msg and ccpp_err_flag and return? Or is this error not that egregious?
| case ("qg") | ||
| qg (k) = q (k) | ||
| case default | ||
| print *, "gfdl_mp: qflag error!" |
There was a problem hiding this comment.
Same question RE: error handling
| case ("qg") | ||
| q = qg | ||
| case default | ||
| print *, "gfdl_mp: qflag error!" |
There was a problem hiding this comment.
Same question RE: error handling
| case ("qg") | ||
| q = qg | ||
| case default | ||
| print *, "gfdl_mp: qflag error!" |
There was a problem hiding this comment.
Same question RE: error handling
| case ("qg") | ||
| qg = q | ||
| case default | ||
| print *, "gfdl_mp: qflag error!" |
| a4 (4, n) * r3 * (pr * (pr + pl) + pl ** 2) | ||
| qm (k) = qm (k) * (ze (k) - ze (k + 1)) | ||
| k0 = n | ||
| goto 555 |
There was a problem hiding this comment.
GOTOs are generally frowned upon for CCPP schemes. Consider trying to use a different control structure if possible.
| te (k) = rgrav * te (k) * delp (k) | ||
| tw (k) = rgrav * (qv (k) + q_cond) * delp (k) | ||
| enddo | ||
| te_b = (dte + (lv00 * c_air * vapor - li00 * c_air * (ice + snow + graupel)) * dts / 86400 + sen * dts + stress * dts) |
There was a problem hiding this comment.
redefining # of seconds in a day again
|
@RuiyuSun @dustinswales I finished my initial review. Mostly minor comments. The biggest things for me are that there are a lot of physical constants being redefined that are introducing potential inconsistencies with the rest of the code base (host and rest of physics) and the potential need to extend CCPP error handling downward into the actual physics code to catch and return errors. Overall, nice job! I don't see anything that absolutely HAS to be changed, although addressing my comments would definitely help CCPP compatibility. |
Description (*updated on 11/18/2024)
This PR is intended to supersede #162
@RuiyuSun I refactored to code so that GFDL v1 and v3 share as much common code as possible.
For the refactoring, all of the configuration/namelist info is moved into a common module that is referenced by both MP versions and a shared fv_sat_adj.F90. There are default values for the scheme configurations defined within this module, and overwritten by the namelist, if provided. Both the v1 and v3 rely on some of the same parameters, but with different default values. This is handled by introducing a pre-processor directive into the new configuration module.
I also removed the imp_physics_gfdl_v3 flag. GFDL v1 and v3 have their own distinct namelists, so there is no need to have another switch to distinguish v1/v3.
The only differences to run with GFDLv1 or v3 are:
Testing
Tested on Hera using Intel
No changes to GFDL MP v1 enabled baselines (control_iovr4 and hafs_regional_atm)