Add extended diagnostic output from Thompson MP, includes "Add optional scaling to RRTMGP flux adjustment" (#668), fix index bug in m_micro.F90 #679
Conversation
…pson_ext_diag_dom
physics/module_mp_thompson.F90
Outdated
| deallocate (nrten1) | ||
| deallocate (ncten1) | ||
| deallocate (qcten1) | ||
| else |
There was a problem hiding this comment.
This looks fine. It is unquestionably "cleaner" from a code (and metadata) point of view to put these diagnostics in an array. However, this sorta hides what the various diagnostics are. For example, are some of these diagnostics already represented by preexisting variables? Will this lead to hard-to-find duplication at some point down the road? There is a similar array for radiation. The opaqueness of these types of arrays is somewhat against the spirit of the CCPP is all I'm saying, despite being albeit unquestionably cleaner. Another way of looking at this is, what if every CCPP scheme had arrays of opaque/latent/hidden (in terms of metadata) variables?
|
I think it is important to understand that these diagnostics are highly specific to Thompson MP and that only developers who know what they are doing will be using them (e.g. @ericaligo-NOAA who contributed these code changes using the |
|
These diagnostics are not represented by preexisting variables and
duplication would not be an issue. As Dom noted, they are
Thompson-scheme specific.
…On 6/25/2021 2:58 PM, grantfirl wrote:
***@***.**** approved this pull request.
This looks fine. It is unquestionably "cleaner" from a code (and
metadata) point of view to put these diagnostics in an array. However,
this sorta hides what the various diagnostics are. For example, are
some of these diagnostics already represented by preexisting
variables? Will this lead to hard-to-find duplication at some point
down the road? There is a similar array for radiation. The opaqueness
of these types of arrays is somewhat against the spirit of the CCPP is
all I'm saying, despite being albeit unquestionably cleaner. Another
way of looking at this is, what if every CCPP scheme had arrays of
opaque/latent/hidden (in terms of metadata) variables?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#679 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALQ75MOPQTC6ZNTAF6ETEQ3TUTGT3ANCNFSM47EOCKTA>.
|
|
I get it, it's the easiest implementation for sure, no question. If physics developers really want this functionality (to be able to define an array of diagnostics for which no metadata exists, regardless of whether its contents are represented by pre-existing variables, and "protected" from being molested by other schemes), we probably need to say this in our documentation and carve out an exception in rules or otherwise provide guidance. Some non-trivial fraction of existing diagnostic variables could probably be wrapped up in arrays like this if we were to be consistent across existing CCPP schemes too. If somewhere down the line some scheme wants access to one of the diagnostics (to use as input into their scheme to do something useful with it), then it will just need to be extracted and made an interface variable at that time, I guess. It might be hard for them to discover them without good documentation of what is in the diagnostics array however. I'm guessing that folks might want to explore this in the CCPP-framework meeting if we haven't done so at some previous point in the past already. |
|
I don't think anyone will ever want to "use" those arrays. They are purely for diagnosing Thompson MP, for example to understand why it becomes unstable at larger timesteps.
… On Jun 25, 2021, at 2:34 PM, grantfirl ***@***.***> wrote:
I get it, it's the easiest implementation for sure, no question. If physics developers really want this functionality (to be able to define an array of diagnostics for which no metadata exists, regardless of whether its contents are represented by pre-existing variables, and "protected" from being molested by other schemes), we probably need to say this in our documentation and carve out an exception in rules or otherwise provide guidance. Some non-trivial fraction of existing diagnostic variables could probably be wrapped up in arrays like this if we were to be consistent across existing CCPP schemes too. If somewhere down the line some scheme wants access to one of the diagnostics (to use as input into their scheme to do something useful with it), then it will just need to be extracted and made an interface variable at that time, I guess. It might be hard for them to discover them without good documentation of what is in the diagnostics array however.
I'm guessing that folks might want to explore this in the CCPP-framework meeting if we haven't done so at some previous point in the past already.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#679 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5C2RIVMAUJBTQAZ3N3TNLTUTR6ZANCNFSM47EOCKTA>.
|
|
Other examples falling within the same mold would be all of the scheme-specific diagnostics coming from unified UGWP, MYNN PBL, GF convection, etc. I'm not talking about this specific PR -- more about the precedent it might create. I'm not asking to change what was done in any way, just thinking how to handle the general case. How can you be sure that what you consider scheme-specific diagnostic will not be wanted by another developer in the future? This seems subjective. |
|
We just need to make clear that these are scheme-internal diagnostics that are not supposed to be used by anyone else. Not having any documentation on what these arrays are certainly helps with not being useable by other schemes/developers. I would consider these diagnostics as the equivalent to scheme-internal debug flags that developers put in and that can be turned on if needed to diagnose a model crash. MYNN has those, RUC LSM has those, and others too.
… On Jun 25, 2021, at 2:41 PM, grantfirl ***@***.***> wrote:
Other examples falling within the same mold would be all of the scheme-specific diagnostics coming from unified UGWP, MYNN PBL, GF convection, etc.
I'm not talking about this specific PR -- more about the precedent it might create. I'm not asking to change what was done in any way, just thinking how to handle the general case.
How can you be sure that what you consider scheme-specific diagnostic will not be wanted by another developer in the future? This seems subjective.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#679 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5C2RJSWAJFHUEWQH73EW3TUTSYRANCNFSM47EOCKTA>.
|
|
Dom, right. That's precisely my point. Other schemes need this kind of functionality, yet up until now, we've been asking for metadata and for the variables to be passed in/out individually, which I think is the case if you look at both of your examples, MYNN PBL and RUC LSM. Perhaps they have these types of variables (that are only allocated and given values when a special flag is set), yet they are passed in/out individually and have metadata, because that's what the CCPP rules say to do. There are exceptions like what is in this PR and the radiation "fluxr" array. This PR (and other arrays like this) represents a kind of "subversion" of the CCPP rules, but I'm now arguing that it should probably be an officially-sanctioned one. After looking at the list of standard names of variables in the Diag and Interstitial DDTs, there are many that are precisely the type of variable that have been lumped into an array in this PR. If we change the CCPP rules to allow this kind of thing, it saves developers the trouble (and CCPP maintainers longterm) from having to mess with metadata for which no one will use except for those developing/debugging a particular scheme. I think that it could help reduce complexity and increase scheme portability in the longterm. All of this hinges on all of the "hidden" variables truly being scheme-specific and for diagnostic/debugging purposes, though. Although I do worry about "abuse" of this technique (like I mentioned upthread), the fact that the fluxr array hasn't caused too many problems means that this is probably viable to do (and we should therefore include an exception to the rule in the CCPP docs for this purpose). |
I couldn't agree more. |
|
I'll chime in that this seems OK too.
The problems may occur for the developer if they change this array in
future releases where they pay
the price for an undocumented list of arrays. Ideally there would be a way
to document this array within
it or with some kind of information file that connects indices to arrays,
but I am not sure of the best way.
Jimy
…On Fri, Jun 25, 2021 at 3:42 PM Dom Heinzeller ***@***.***> wrote:
Dom, right. That's precisely my point. Other schemes need this kind of
functionality, yet up until now, we've been asking for metadata and for the
variables to be passed in/out individually, which I think is the case if
you look at both of your examples, MYNN PBL and RUC LSM. Perhaps they have
these types of variables (that are only allocated and given values when a
special flag is set), yet they are passed in/out individually and have
metadata, because that's what the CCPP rules say to do. There are
exceptions like what is in this PR and the radiation "fluxr" array.
This PR (and other arrays like this) represents a kind of "subversion" of
the CCPP rules, but I'm now arguing that it should probably be an
officially-sanctioned one. After looking at the list of standard names of
variables in the Diag and Interstitial DDTs, there are many that are
precisely the type of variable that have been lumped into an array in this
PR. If we change the CCPP rules to allow this kind of thing, it saves
developers the trouble (and CCPP maintainers longterm) from having to mess
with metadata for which no one will use except for those
developing/debugging a particular scheme. I think that it could help reduce
complexity and increase scheme portability in the longterm. All of this
hinges on all of the "hidden" variables truly being scheme-specific and for
diagnostic/debugging purposes, though.
Although I do worry about "abuse" of this technique (like I mentioned
upthread), the fact that the fluxr array hasn't caused too many problems
means that this is probably viable to do (and we should therefore include
an exception to the rule in the CCPP docs for this purpose).
I couldn't agree more.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#679 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEIZ77FUTXP2KRR47GWQAFTTUTZ33ANCNFSM47EOCKTA>
.
|
There was a problem hiding this comment.
Just a few thoughts added. I think we should just clean up the extra things that were added that really do only care about extended diagnostics but are otherwise added regardless. I don't believe this should be done in the interest of memory footprint or requirement to have just these 3 items in all people running the scheme.
physics/module_mp_thompson.F90
Outdated
| allocate (nrten1(kts:kte)) | ||
| allocate (ncten1(kts:kte)) | ||
| allocate (qcten1(kts:kte)) | ||
| else |
There was a problem hiding this comment.
If there are only 3 arrays that overlap out of the total ~35, is it much worry? Also, @ericaligo-NOAA is there a reason we need those 3 (vtsk1, txri1, txrc1) at all in the case without extended diagnostics? I think it's because you started with just 3 arrays and have since expanded. So my bigger concern is taking out those 3 completely in the circumstance of running without that namelist option - making it cleaner overall since some users won't want the overheads of 3 more 3D variables - which is a memory issue when running really big domains.
| @@ -3669,6 +4001,8 @@ subroutine mp_thompson (qv1d, qc1d, qi1d, qr1d, qs1d, qg1d, ni1d, & | |||
| qiten(k) = qiten(k) - xri*odt | |||
| niten(k) = -ni1d(k)*odt | |||
| tten(k) = tten(k) - lfus*ocp(k)*xri*odt*(1-IFDRY) | |||
| !diag | |||
There was a problem hiding this comment.
If you follow my suggestion to take these for ext diag only, then please remember to place these in a IF-statement for the logical namelist check.
There was a problem hiding this comment.
@gthompsnWRF @ericaligo-NOAA I would like to finish the discussion of this PR today, if possible, because we need to merge it next week and I will be on leave most of next week, too.
Should we remove those three diagnostic fields vtsk1, txri1, txrc1 and not worry about wrapping them in if statements inside do loops?
physics/module_mp_thompson.F90
Outdated
| @@ -3449,6 +3777,7 @@ subroutine mp_thompson (qv1d, qc1d, qi1d, qr1d, qs1d, qg1d, ni1d, & | |||
| nstep = 0 | |||
| do k = kte, kts, -1 | |||
| vts = 0. | |||
| vtsk1(k)=0. | |||
There was a problem hiding this comment.
@ericaligo-NOAA I know why you created a separate array to get a diagnostic output to show for sure where snow terminal velocity is zero when snow is zero, but is it really needed any longer? You know the reason I have vtsk(k) = vtsk(k+1) is to avoid the "accumulation" problem during sedimentation. So the assignment in zero snow is possible here, but you can also just use a post-process mask of something like: WHERE (snow.lt.1.E-9) vtsk(k)=0.0 in output files to achieve the same thing without adding pretty much unneeded variables.
| @@ -3758,6 +4094,89 @@ subroutine mp_thompson (qv1d, qc1d, qi1d, qr1d, qs1d, qg1d, ni1d, & | |||
| if (qg1d(k) .le. R1) qg1d(k) = 0.0 | |||
| enddo | |||
|
|
|||
| ! Diagnostics | |||
There was a problem hiding this comment.
For this instant, I am going to assume all the terms individually are done right, but I will attempt to come back to this and double-check, thinking it may take me 20-30 minutes to be really thorough.
|
I can explain why three of these arrays are always allocated. For all other extended diagnostics, computation and assignment are isolated to one block of code at the end of the subroutine. The three arrays, however, are computed in various places in the code beforehand and are inside loops over the vertical dimension. I did not want to "pollute" the code and negatively affect performance by adding |
|
You're right that for understanding the temperature warnings we probably
don't care about vtsk. I kept it in because I plan on adding vtrk,vtik
and vtgk for potentially future development work along with other
diagnostics that Brad and I looked at when Thompson was running in
NMMB. I don't expect the repository to include those additional
diagnostics I plan on adding, and I will likely need to include them in
a branch. Agree that the condition of snow.lt.1E-9 would work. I
created the txri and txrc diagnostics for the two terms listed below in
red since they do contribute to the heating/cooling unless you feel we
don't need to look at these. I'm not sure what is meant by whitespace
and I hope I'm not contributing to that.
tten(k) = tten(k) - *lfus*ocp(k)*xri*odt*(1-IFDRY)*
!diag
txri1(k) = lfus*ocp(k)*xri*odt*(1-IFDRY)
tten(k) = tten(k) + lfus2*ocp(k)*xrc*odt*(1-IFDRY)
!diag
txrc1(k) = *lfus2*ocp(k)*xrc*odt*(1-IFDRY)*DT*
…On 6/28/2021 1:01 PM, gthompsnWRF wrote:
***@***.***WRF* requested changes on this pull request.
Just a few thoughts added. I think we should just clean up the extra
things that were added that really do only care about extended
diagnostics but are otherwise added regardless. I don't believe this
should be done in the interest of memory footprint or requirement to
have just these 3 items in all people running the scheme.
------------------------------------------------------------------------
In physics/module_mp_thompson.F90
<#679 (comment)>:
>
!..Local variables
REAL, DIMENSION(kts:kte):: &
- qv1d, qc1d, qi1d, qr1d, qs1d, qg1d, ni1d, &
- nr1d, nc1d, nwfa1d, nifa1d, &
I admit it's nit-picky, and I can also ignore whitespace in git, but I
really keep struggling why people keep changing whitespace anywhere
like this when it serves no purpose and just adds to the viewer load.
I know it means nothing but everyone ever touching my scheme keeps
bumping whitespace around and it's distracting.
------------------------------------------------------------------------
In physics/module_mp_thompson.F90
<#679 (comment)>:
> @@ -3669,6 +4001,8 @@ subroutine mp_thompson (qv1d, qc1d, qi1d, qr1d, qs1d, qg1d, ni1d, &
qiten(k) = qiten(k) - xri*odt
niten(k) = -ni1d(k)*odt
tten(k) = tten(k) - lfus*ocp(k)*xri*odt*(1-IFDRY)
+!diag
If you follow my suggestion to take these for ext diag only, then
please remember to place these in a IF-statement for the logical
namelist check.
------------------------------------------------------------------------
In physics/module_mp_thompson.F90
<#679 (comment)>:
> @@ -3449,6 +3777,7 @@ subroutine mp_thompson (qv1d, qc1d, qi1d, qr1d, qs1d, qg1d, ni1d, &
nstep = 0
do k = kte, kts, -1
vts = 0.
+ vtsk1(k)=0.
@ericaligo-NOAA <https://github.com/ericaligo-NOAA> I know why you
created a separate array to get a diagnostic output to show for sure
where snow terminal velocity is zero when snow is zero, but is it
really needed any longer? You know the reason I have vtsk(k) =
vtsk(k+1) is to avoid the "accumulation" problem during sedimentation.
So the assignment in zero snow is possible here, but you can also just
use a post-process mask of something like: WHERE (snow.lt.1.E-9)
vtsk(k)=0.0 in output files to achieve the same thing without adding
pretty much unneeded variables.
------------------------------------------------------------------------
In physics/module_mp_thompson.F90
<#679 (comment)>:
> @@ -3758,6 +4094,89 @@ subroutine mp_thompson (qv1d, qc1d, qi1d, qr1d, qs1d, qg1d, ni1d, &
if (qg1d(k) .le. R1) qg1d(k) = 0.0
enddo
+! Diagnostics
For this instant, I am going to assume all the terms individually are
done right, but I will attempt to come back to this and double-check,
thinking it may take me 20-30 minutes to be really thorough.
------------------------------------------------------------------------
In physics/module_mp_thompson.F90
<#679 (comment)>:
> + allocate (tprr_rcg1(kts:kte))
+ allocate (tprr_rcs1(kts:kte))
+ allocate (tprv_rev1(kts:kte))
+ allocate (txri1(kts:kte))
+ allocate (txrc1(kts:kte))
+ allocate (tten1(kts:kte))
+ allocate (qvten1(kts:kte))
+ allocate (qrten1(kts:kte))
+ allocate (qsten1(kts:kte))
+ allocate (qgten1(kts:kte))
+ allocate (qiten1(kts:kte))
+ allocate (niten1(kts:kte))
+ allocate (nrten1(kts:kte))
+ allocate (ncten1(kts:kte))
+ allocate (qcten1(kts:kte))
+ else
If there are only 3 arrays that overlap out of the total ~35, is it
much worry? Also, @ericaligo-NOAA <https://github.com/ericaligo-NOAA>
is there a reason we need those 3 (vtsk1, txri1, txrc1) at all in the
case without extended diagnostics? I think it's because you started
with just 3 arrays and have since expanded. So my bigger concern is
taking out those 3 completely in the circumstance of running without
that namelist option - making it cleaner overall since some users
won't want the overheads of 3 more 3D variables - which is a memory
issue when running really big domains.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#679 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALQ75MORTIHGFATJCH7CZDTTVCTIPANCNFSM47EOCKTA>.
|
…pson_ext_diag_dom
…s in physics/module_mp_thompson.F90
climbfuji
force-pushed
the
thompson_ext_diag_dom
branch
from
18476ae to
28650fb
Compare
|
As far as I could tell,
- lfus*ocp(k)*xri*odt*(1-IFDRY), which I assigned to txri, does
contribute to a temperature change, and is not duplicate,
therefore I would include that in the if statement. There is another
diagnostic, txrc, on a different line, related to xrc, which also
contributes to a temperature change.
Down the road, I do plan on adding all of the fall speeds and
approximately 6 other diagnostics for development purposes.
Also, I believe what's in the PR now is likely not the final product,
unfortunately. Sorry about that. I made some changes that I think
were necessary, but I believe cross sections will tell us if my changes
are correct.
Eric
…On 7/2/2021 9:05 AM, Dom Heinzeller wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In physics/module_mp_thompson.F90
<#679 (comment)>:
> @@ -3669,6 +4001,8 @@ subroutine mp_thompson (qv1d, qc1d, qi1d, qr1d, qs1d, qg1d, ni1d, &
qiten(k) = qiten(k) - xri*odt
niten(k) = -ni1d(k)*odt
tten(k) = tten(k) - lfus*ocp(k)*xri*odt*(1-IFDRY)
+!diag
@gthompsnWRF <https://github.com/gthompsnWRF> @ericaligo-NOAA
<https://github.com/ericaligo-NOAA> I would like to finish the
discussion of this PR today, if possible, because we need to merge it
next week and I will be on leave most of next week, too.
Should we remove those three diagnostic fields vtsk1, txri1, txrc1 and
not worry about wrapping them in |if| statements inside |do| loops?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#679 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALQ75MPTAVFSP6IDRBR2KVTTVW2P5ANCNFSM47EOCKTA>.
|
Ok. Here is my suggestion. For the purpose of getting this PR merged, I will temporarily comment out the three variables vtsk1, txri1, txrc1 to avoid adding more if statements inside the do loops. We can update the diagnostics in a follow-up PR once you have completed your investigation. |
|
@ericaligo-NOAA @gthompsnWRF I commented out the three diagnostics vts1, txri, txrc in my latest commit. We can decide what to do with them in a follow-up PR. Please take a look and approve if satisfied. Thanks! |
…nswales/ccpp-physics into thompson_ext_diag_dom
climbfuji
changed the title
climbfuji
changed the title
Description
This PR builds on the work of @ericaligo-NOAA to output a number of diagnostic quantities from Thompson MP using generic
aux3darrays. A dedicated set of arrays and the corresponding logic to allocate them is added in the associated fv3atm PR listed below, and the Thompson microphysics code is updated to populate these based on a runtime configuration flag ininput.nml.Fixes #680.
This PR includes the changes in #668, "Add optional scaling to RRTMGP flux adjustment".
Also: fixes index bug in
m_micro.F90.Testing
See ufs-community/ufs-weather-model#658
Dependencies
#679
NOAA-EMC/fv3atm#331
ufs-community/ufs-weather-model#658