Refactor C3P and introduce CCPP interface with MPAS compatibility; migrate to submodule#373
Conversation
… shared MPAS code. Move to stand alone submodule
…ke the ascii format
…ke the ascii format 2
…ke the ascii format 3
dustinswales
left a comment
There was a problem hiding this comment.
@lisa-bengtsson This all looks good to me. I think we can delete mynnedmf_wrapper_post ant use GFS_DCNV_generic_post everywhere?
I had some suggestions in the C3 submodule PR that will require changes here.
There was a problem hiding this comment.
@grantfirl I think this file can be removed entirely and replaced with GFS_PBL_generic_post. Right?
There was a problem hiding this comment.
There is a subtlety that the scheme wants pbl_tke, and I am a bit uncertain how to make it generic.
I see in the wrapper that there is:
[qke]
standard_name = nonadvected_turbulent_kinetic_energy_multiplied_by_2
long_name = 2 x tke at mass points
units = m2 s-2
dimensions = (horizontal_loop_extent,vertical_layer_dimension)
type = real
kind = kind_phys
intent = inout
[qke_adv]
standard_name = turbulent_kinetic_energy
long_name = turbulent kinetic energy
units = J kg-1
dimensions = (horizontal_loop_extent,vertical_layer_dimension)
type = real
kind = kind_phys
intent = inout
GFS_PBL_generic_post has "turbulent_kintetic_energy" and it is a tracer that is advected. After talking to @joeolson42 it seems that this is not always used by default and then set to 0.
From Joe: "qke_adv is the fully advected TKE, when tke_advect = T, otherwise = 0.
Basically, it's a scalar that is advected when the TKE advection flag is activated, otherwise that array is neglected.
QKE is always allocated and set equal to QKE_ADV when TKE advection is activated, so we only have to output QKE and then you'll get a prognosed variable that is either advected or not."
The problem is that I then, depending on scheme, have to divide by 2 inside the C3 driver to arrive at "pbl_tke".
There was a problem hiding this comment.
@dustinswales Most of GFS_pbl_generic_post.F90 isn't needed when MYNN PBL is being used. For example all other PBL schemes use the vertically-diffused tracer subset array, whereas MYNN does not. The only thing the MYNN PBL needs is to handled tendencies afterwards, hence the simple scheme. Alternatively, one could use GFS_pbl_generic_post.F90 and just check for MYNN to bypass most of that code.
There was a problem hiding this comment.
@grantfirl @lisa-bengtsson @joeolson42 Everything in mynn_wrapper_post.F90 is repeated in GFS_PBL_generic_post.F90. So why have both?
We could configure GFS_PBL_generic_post.F90 to only do what we want it to for MYNN (i.e., accumulate tends), and skip the rest.
One way to do this is to move the move the tendency application piece to the top of GFS_PBL_generic_post and return afterwards when using mynnedmf?
Also, organizationally it makes sense for the state/tendency update to occur in a Host interstitial, not a Scheme interstitial.
@lisa-bengtsson Sorry for cluttering your C3 PR with other housekeeping issues. If we agree, I will open an issue and address this later in a follow up PR.
There was a problem hiding this comment.
So why have both?
Ya, not technically necessary, but I think in the past, Joe has kinda preferred to keep MYNN using different interstitials (e.g. sgscloud) that are simpler/streamlined than the generic versions.
It's also not possible to move the tendency application first in that file due to the translation between the vertically-diffused tracer array logic. But, it doesn't really matter. It's easy to put a check for MYNN and skip everything except the tendency application.
There was a problem hiding this comment.
@dustinswales @grantfirl yes, opening a separate issue/PR for this would be better I think. From my point of view creating a GFS_generic_physics_post (or similar) to accompany the physics_pre that you created would be great for extracting the dynamics tendencies that are now used in multiple scheme and re-computed depending on suite. For instance, for saSAS I added that in GFS_micro_pre/post a while ago, that could be moved. This "overarching" pre/post could also include the general thermodynamic states that multiple schemes use as we discussed (such as moist air density...). Then having one "turbulent_kinetic_energy" regardless of scheme, and one place for the PBL tendencies regardless of scheme would also be fantastic. We could remove the pre-fix "GFS" if it is unclear that is is more generic than that.
… to have suffix _ccpp and _mpas respectively
…ure moisture conservation
| [submodule "physics/CONV/C3"] | ||
| path = physics/CONV/C3 | ||
| url = https://github.com/lisa-bengtsson/c3 | ||
| branch = c3 |
There was a problem hiding this comment.
@lisa-bengtsson Will the c3 branch always be the branch that the CCPP repo points to? I noticed that there were both main and c3 branches in the C3 repo. Some submodule schemes use main. Others use a separate branch like is done here. Just wanting to confirm your intent. Otherwise, if you do intend to use main, there should probably be a PR in the C3 submodule from the c3 branch to main to go along with this PR set.
There was a problem hiding this comment.
@grantfirl yes - that is this one: ufs-community/c3#2
|
@lisa-bengtsson If I wanted to review changes to the C3 code represented by this PR (since it contains both changes to the scheme and changing it to a submodule simultaneously), do you know if the start of the submodule C3 repo was identical to the version of C3 in the CCPP? I.e. if I were to look at differences between the commit that you're using in this PR versus the initial C3 commit, would that show me what has changed within the actual scheme internals? |
|
No, unfortunately not - more or less everything is new. I would treat it as if this was a new scheme that entered CCPP and maybe review it in terms of CCPP compliance? |
grantfirl
left a comment
There was a problem hiding this comment.
The part that remains in the CCPP repo looks great to me. Just minor comment to improve robustness, but not necessary to change if you don't want to.
Thank you Grant, I will address these this afternoon. |
|
@lisa-bengtsson Could you mention ufs-community/c3#2 in the PR description please? |
rhaesung
left a comment
There was a problem hiding this comment.
LGTM. I left a few minor comments on ufs-community/c3#2.
|
@dustinswales Has your requested change been addressed? It sounds like perhaps a separate issue was going to be created? |
The requested changes will be in a followup PR.
|
The C3 sub-PR was merged, and Lisa updated the hash and .gitmodules, so this PR should be set to merge. |
This PR introduces a major refactoring of the community convective cloud parameterization (C3P), including a new CCPP-facing interface that is compatible with shared MPAS code. As part of this effort, the codebase is migrated from a subdirectory into a standalone repository located here: ufs-community/c3#2