NCAR / ccpp-physics

GFS physics for CCPP
Other
58 stars 145 forks source link

Scientific documentation updates from CCPP v6.0.0 release and code cleanup #944

Closed mzhangw closed 2 years ago

mzhangw commented 2 years ago

This PR is aimed at:

All RTs passed on Hera.

mjiacono commented 2 years ago

I recommend the following changes in the radiation documentation:

On this page: https://dtcenter.ucar.edu/GMTB/v6.0.0/sci_doc/_g_f_s__r_r_t_m_g.html

Under “Description”: 1) In the second paragraph, change “and the vertical radiative flux divergence” to “and the vertical radiative net flux divergence” 2) In the first sentence of the third paragraph, I recommend replacing “(RRTMG_LW v2.3 and RRTMG_SW v2.3)” with “(RRTMG_LW and RRTMG_SW)”. It’s probable that v2.3 of the codes were the versions originally transferred to NOAA about 20 years ago, but over time, the codes have been kept closely aligned with later AER releases, so it’s misleading to reflect the outdated version numbers here. Note that the equivalent modern code versions are listed on the specific RRTMG_LW Main and RRTMG_SW Main pages. 3) At the end of the third paragraph, change “A maximum-random cloud overlap method is used…” to “Several cloud overlap methods, including maximum-random, exponential, and exponential-random, are available…”

On this page: https://dtcenter.ucar.edu/GMTB/v6.0.0/sci_doc/group__module__radlw__main.html#gen_lwrad

Under “Modules” 1) For module_radlw_kgb01, change “10-250 cm-1” to “10-350 cm-1”. The spectral ranges for bands 1 and 2 were adjusted many years ago, and the data and code in the model reflect the proper (newer) wavenumber ranges for these bands. Note that the corresponding descriptions under modules “taugb01” and “taugb02” also reflect the correct spectral ranges for these bands. 2) For module_radlw_kgb02, change “250-500 cm-1” to “350-500 cm-1”

mzhangw commented 2 years ago

Thanks for your comments. All your comments are addressed.

grantfirl commented 2 years ago

@mzhangw Please amend the description of this PR to indicate that several empty subroutines were removed since that is definitely not documentation as the title suggests. Should I interpret these changes as being "complete"; i.e. did you remove all empty subroutines?

mzhangw commented 2 years ago

@grantfirl I was told to leave those empty subroutines at some point since it is not the primary purpose of this activity, and I cleaned up some of them, but not all of them. So do you want me to double-check if there is anything left?

grantfirl commented 2 years ago

My preference would be to either leave all of the empty subroutines intact (to be removed in a separate PR) or to remove all of them in this one. Since you've already done a bunch of them, it doesn't make sense to undo them, so I would say that it would be good to double-check the rest of the code. @ligiabernardet any opinion?

ligiabernardet commented 2 years ago

@mzhangw Pls remove the empty subroutines and make sure RTs pass. Tks

mzhangw commented 2 years ago

@mzhangw Pls remove the empty subroutines and make sure RTs pass. Tks

@ligiabernardet @grantfirl All empty subroutines are removed as requested. This PR passed all RTs on Hera.

grantfirl commented 2 years ago

@mzhangw Please address the comment from @joeolson42. I ended up putting upstream PRs for this PR by itself due to the delay in approval for the ccpp-framework one. I'm going to ask for this to be put into the UFS merge queue next week.