Skip to content
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

feature/transition-to-capgen-1: remove DDT dependency in radiation physics, correct units #506

Merged

Conversation

climbfuji
Copy link
Collaborator

@climbfuji climbfuji commented Oct 6, 2020

This PR contains

updated with the latest code in feature/transition-to-capgen-1

Associated PRs:

#506
NCAR/ccpp-framework#327
NCAR/fv3atm#64
NCAR/ufs-weather-model#65

For regression testing information, see NCAR/ufs-weather-model#65.

grantfirl and others added 28 commits January 17, 2020 14:21
@climbfuji climbfuji requested a review from mzhangw October 7, 2020 15:28
rog, rocp, con_rd, xlat_d, xlat, xlon, coslat, sinlat, tsfc, slmsk, &
prsi, prsl, prslk, tgrs, sfc_wts, phy_f3d_mg_cld, phy_f3d_reffr, &
phy_f3d_cnvw, phy_f3d_cnvc, qgrs, aer_nm, & !inputs from here and above
coszen, coszdg, phy_f3d_leffr, phy_f3d_ieffr, phy_f3d_seffr, &
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@climbfuji Shall I fix the phy_f3d naming stuff and push a PR to your branch?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like only dropping phy_f3d from the names might be confusing for some variables. How about the following? It looks like these variables are mainly used to shuttle data into local copies for calculation, with the exception of the inout ones that are used directly. So, appending intents might make sense? I'm open to other/better suggestions too.

phy_f3d_mg_cld = mg_cld
phy_f3d_reffr = effrr_in
phy_f3d_cnvw = cnvw_in
phy_f3d_cnvc = cnvc_in
phy_f3d_leffr = effrl_inout
phy_f3d_ieffr = effri_inout
phy_f3d_seffr = effrs_inout

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have done that yesterday afternoon before running the tests, I forgot to follow up. Let's do the following: Merge as is, change the names when creating the PRs from transition-to-capgen-1 to the authoritative branches. Which names would you like me to use?

On Oct 7, 2020, at 9:56 AM, grantfirl @.***> wrote: @grantfirl commented on this pull request. In physics/GFS_rrtmg_pre.F90 <#506 (comment)>: > - clouds1, clouds2, clouds3, clouds4, clouds5, clouds6, & - clouds7, clouds8, clouds9, cldsa, cldfra, & - mtopa, mbota, de_lgth, alpha, alb1d, errmsg, errflg) + subroutine GFS_rrtmg_pre_run (im, levs, lm, lmk, lmp, n_var_lndp, & + imfdeepcnv, imfdeepcnv_gf, me, ncnd, ntrac, num_p3d, npdf3d, ncnvcld3d,& + ntqv, ntcw,ntiw, ntlnc, ntinc, ncld, ntrw, ntsw, ntgl, ntwa, ntoz, & + ntclamt, nleffr, nieffr, nseffr, lndp_type, kdt, imp_physics, & + imp_physics_thompson, imp_physics_gfdl, imp_physics_zhao_carr, & + imp_physics_zhao_carr_pdf, imp_physics_mg, imp_physics_wsm6, & + imp_physics_fer_hires, julian, yearlen, lndp_var_list, lsswr, lslwr, & + ltaerosol, lgfdlmprad, uni_cld, effr_in, do_mynnedmf, lmfshal, & + lmfdeep2, fhswr, fhlwr, solhr, sup, eps, epsm1, fvirt, & + rog, rocp, con_rd, xlat_d, xlat, xlon, coslat, sinlat, tsfc, slmsk, & + prsi, prsl, prslk, tgrs, sfc_wts, phy_f3d_mg_cld, phy_f3d_reffr, & + phy_f3d_cnvw, phy_f3d_cnvc, qgrs, aer_nm, & !inputs from here and above + coszen, coszdg, phy_f3d_leffr, phy_f3d_ieffr, phy_f3d_seffr, & @climbfuji https://github.com/climbfuji Shall I fix the phy_f3d naming stuff and push a PR to your branch? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#506 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB5C2RLTFRMGEWIFCI2HLYLSJSFTVANCNFSM4SGRKGRA.

I put some suggestions in the comment in the file. I'm not sure it's the best, but it shouldn't really matter a whole lot what they're called locally.

Thanks, I agree with your suggestion.

Copy link
Collaborator

@mzhangw mzhangw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tedious DDT cleanup is amazing. Great job!

@climbfuji
Copy link
Collaborator Author

climbfuji commented Oct 7, 2020 via email

@grantfirl
Copy link
Collaborator

We should have done that yesterday afternoon before running the tests, I forgot to follow up. Let's do the following: Merge as is, change the names when creating the PRs from transition-to-capgen-1 to the authoritative branches. Which names would you like me to use?

On Oct 7, 2020, at 9:56 AM, grantfirl @.***> wrote: @grantfirl commented on this pull request. In physics/GFS_rrtmg_pre.F90 <#506 (comment)>: > - clouds1, clouds2, clouds3, clouds4, clouds5, clouds6, & - clouds7, clouds8, clouds9, cldsa, cldfra, & - mtopa, mbota, de_lgth, alpha, alb1d, errmsg, errflg) + subroutine GFS_rrtmg_pre_run (im, levs, lm, lmk, lmp, n_var_lndp, & + imfdeepcnv, imfdeepcnv_gf, me, ncnd, ntrac, num_p3d, npdf3d, ncnvcld3d,& + ntqv, ntcw,ntiw, ntlnc, ntinc, ncld, ntrw, ntsw, ntgl, ntwa, ntoz, & + ntclamt, nleffr, nieffr, nseffr, lndp_type, kdt, imp_physics, & + imp_physics_thompson, imp_physics_gfdl, imp_physics_zhao_carr, & + imp_physics_zhao_carr_pdf, imp_physics_mg, imp_physics_wsm6, & + imp_physics_fer_hires, julian, yearlen, lndp_var_list, lsswr, lslwr, & + ltaerosol, lgfdlmprad, uni_cld, effr_in, do_mynnedmf, lmfshal, & + lmfdeep2, fhswr, fhlwr, solhr, sup, eps, epsm1, fvirt, & + rog, rocp, con_rd, xlat_d, xlat, xlon, coslat, sinlat, tsfc, slmsk, & + prsi, prsl, prslk, tgrs, sfc_wts, phy_f3d_mg_cld, phy_f3d_reffr, & + phy_f3d_cnvw, phy_f3d_cnvc, qgrs, aer_nm, & !inputs from here and above + coszen, coszdg, phy_f3d_leffr, phy_f3d_ieffr, phy_f3d_seffr, & @climbfuji https://github.com/climbfuji Shall I fix the phy_f3d naming stuff and push a PR to your branch? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#506 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB5C2RLTFRMGEWIFCI2HLYLSJSFTVANCNFSM4SGRKGRA.

I put some suggestions in the comment in the file. I'm not sure it's the best, but it shouldn't really matter a whole lot what they're called locally.

standard_name = in_number_concentration
long_name = IN number concentration
units = kg-1?
standard_name = ice_nucleation_number
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that this is better, but it can be resolved in the CCPP standard name cleanup that Ligia and I are doing.

Copy link
Collaborator

@grantfirl grantfirl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks.

@climbfuji climbfuji merged commit cf34191 into NCAR:feature/transition-to-capgen-1 Oct 7, 2020
@climbfuji climbfuji deleted the ddt_removal_radiation branch June 27, 2022 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants