-
Notifications
You must be signed in to change notification settings - Fork 145
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
Metadata bug fixes, cleanup of active_gases_array use in RRTMGP, cleanup of Ferrier-Aligo variables, remove redundant consistency checks #749
Conversation
…g-array-alloc-ccpp-caps
…g-array-alloc-ccpp-caps
@dustinswales @SMoorthi-emc please review this PR if you have time - it should be all straightforward, but it contains quite some cleanup and simplification, in particular on the RRTMGP side. Thanks! |
@mzhangw Please review my code changes for Ferrier-Aligo. I stripped out unnecessary variables from the argument list that were used and can be declared locally without affecting the scheme. Thanks! |
character(len=*), intent(out) :: errmsg | ||
integer, intent(out) :: errflg | ||
|
||
! Initialize CCPP error handling variables | ||
errmsg = '' | ||
errflg = 0 | ||
|
||
! Consistency check that the number of albedo components in array |
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.
Consistency checks are no longer required - the CCPP framework does this automatically in the auto-generated caps when compiled in DEBUG mode.
@tanyasmirnova please review the change in |
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
All of the GP changes look fantastic to me. Thanks for cleaning up my mess
@climbfuji |
@@ -54,10 +52,6 @@ subroutine mp_fer_hires_init(ncol, nlev, dtp, imp_physics, & | |||
logical, intent(in) :: restart | |||
character(len=*), intent(out) :: errmsg | |||
integer, intent(out) :: errflg | |||
real(kind_phys), intent(out) :: f_ice(:,:) |
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 doubt if we should omit cwm and "fractions" in FA scheme, since these variables have not been fully tested in UFS yet. Based on our email thread with @ericaligo-NOAA @ligiabernardet and HWRF team on Aug 19, 2019, these variables are essential in operational HWRF model, which uses FA scheme. @ligiabernardet confirmed that we should proceed with also outputting total condensate (cwm) and fractional arrays.
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.
The point is that these arrays are used nowhere in the host model (FV3) at the moment. Further, they were part of the GFS_interstitial
derived data type, which doesn't make sense because this data is not persistent. In other words, the fields couldn't be used in FV3 or the dycore, they couldn't be written to disk, etc. Lastly, because they were part of the GFS_interstitial
DDT, there standard names have not been updated yet.
If we decide in the future that these arrays are needed in the host model, then we can put them back in and use the correct DDTs in FV3, and standard names that are derived from the rules for creating standard names.
The |
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.
Lots of nice simplification in here. I didn't take too much effort to understand the RRTMGP changes (since @dustinswales already signed off), but it looks good to me.
@@ -32,9 +32,7 @@ module mp_fer_hires | |||
!! \htmlinclude mp_fer_hires_init.html | |||
!! | |||
subroutine mp_fer_hires_init(ncol, nlev, dtp, imp_physics, & | |||
imp_physics_fer_hires, & | |||
restart, & | |||
f_ice,f_rain,f_rimef, & |
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.
From my knowledge of Ferrier microphysics that I used to work with about a decade ago, these variables are needed in the radiation_clouds.
May be this has changed now.
This PR contains the following changes:
active_gases_array
in RRTMGPdtend
array, and two arrays in RAS)inGFS_radiation_surface
andGFS_rrtmg_setup
(no longer required due to the new debugging features in the auto-generated caps, see ccpp-framework PR Rename --debug flag to --verbose, add new debugging capability to detect array size mismatches, pretty-print auto-generated caps ccpp-framework#404)Associated PRs:
#749
NCAR/ccpp-framework#404
https://github.com/NOAA-EMC/fv3atm/pull/407/files
NOAA-PSL/stochastic_physics#48
ufs-community/ufs-weather-model#850
For regression testing, see
ufs-community/ufs-weather-model#850