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

Complete Zhang McFarlane conversion to CCPP #186

Open
wants to merge 49 commits into
base: development
Choose a base branch
from

Conversation

cacraigucar
Copy link
Collaborator

closes #66

cacraigucar and others added 30 commits May 17, 2024 11:52
Pull ZM 0.5 timestep removal into main
Remove 0.5*timestop logic from all to zm

Register species from MICM
Replace original, locally-developed license with Apache 2.0 license.
Bring in development ESCOMP#108 (TUV-x) ESCOMP#112 (tropopause_find) to main
Bring in new directory structure from development
Tag name (The PR title should also include the tag name):
`atmos_phys0_07_001`
Originator(s): @jimmielin

List all `development` PR URLs included in this PR and a short
description of each:
* Update extraterrestrial flux in TUV-x prior to calculating rate constants ESCOMP#152 by @boulderdaze
* Simplify deallocation of multiple objects associated with the TUV-x ESCOMP#156 by @boulderdaze
* Fill in errmsg, errflg in check_energy schemes ESCOMP#160 by @jimmielin
* Validates the MUSICA meta data against the CCPP standard names ESCOMP#162 by @boulderdaze
* Add constituent tendency updater ESCOMP#111 by @peverwhee
* Add cloud optical calculations for use in TUV-x ESCOMP#167 by @mattldawson
* Add initialize_constituents scheme ESCOMP#149 by @peverwhee
* Add diagnostics to TJ2016 test schemes ESCOMP#170 by @peverwhee
* update "radians" to "rad" ESCOMP#173 by @peverwhee
* Solar zenith angle and Earth-Sun distance ESCOMP#171 by @mattldawson
* Update standard names for tropopause_find ESCOMP#140 by @jimmielin
* Update surface albedo units ESCOMP#181 by @mattldawson
* don't set water species property for species that air_composition handles ESCOMP#185 by @peverwhee

List all test failures: N/A
@cacraigucar cacraigucar changed the base branch from main to development January 2, 2025 23:46
Copy link
Member

@jimmielin jimmielin left a comment

Choose a reason for hiding this comment

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

Thanks @cacraigucar, and congratulations on completing ZM 🎉!!!

I have minor, hopefully just formatting related changes. As mentioned in the chat I will avoid proposing large changes (e.g., moving the ZM namelist scheme) in this PR so it can be merged and I can propose the changes in another future PR.

I have tested building and running of Hack shallow convection with cloud_fraction_fice and zm_conv_evap and verified they are within roundoff (and bit-for-bit for history output), as well as (modified for shallow) the interstitial schemes for conv fluxes to/from general/deep.

schemes/zhang_mcfarlane/zm_convr_namelist.xml Outdated Show resolved Hide resolved
schemes/sima_diagnostics/zm_diagnostics.F90 Show resolved Hide resolved
schemes/sima_diagnostics/zm_evap_tendency_diagnostics.F90 Outdated Show resolved Hide resolved
schemes/sima_diagnostics/zm_tendency_diagnostics.F90 Outdated Show resolved Hide resolved
@@ -0,0 +1,29 @@
module to_be_ccppized_temporary
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps a call to this has to be added to the CAM7 SDF as well which will use ZM?

schemes/zhang_mcfarlane/zm_conv_convtran.F90 Outdated Show resolved Hide resolved
schemes/zhang_mcfarlane/zm_conv_convtran.F90 Outdated Show resolved Hide resolved
<group name="physics_before_coupler">
<scheme>initialize_constituents</scheme>
<scheme>to_be_ccppized_temporary</scheme>
<scheme>zm_convr</scheme>
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this is within the scope of the PR, but maybe the calls to ZM need to be added to the corresponding locations in the CAM7 SDF as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nusbaume and/or @peverwhee - Should ZM be added to the CAM7 SDF?

Copy link
Collaborator Author

@cacraigucar cacraigucar left a comment

Choose a reason for hiding this comment

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

I believe that I have addressed all of your comment, though I might have missed some.

<group name="physics_before_coupler">
<scheme>initialize_constituents</scheme>
<scheme>to_be_ccppized_temporary</scheme>
<scheme>zm_convr</scheme>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nusbaume and/or @peverwhee - Should ZM be added to the CAM7 SDF?

schemes/zhang_mcfarlane/zm_conv_convtran.F90 Outdated Show resolved Hide resolved
schemes/zhang_mcfarlane/zm_conv_convtran.F90 Outdated Show resolved Hide resolved
schemes/sima_diagnostics/zm_tendency_diagnostics.F90 Outdated Show resolved Hide resolved
schemes/sima_diagnostics/zm_evap_tendency_diagnostics.F90 Outdated Show resolved Hide resolved
schemes/sima_diagnostics/zm_diagnostics.F90 Show resolved Hide resolved
@cacraigucar cacraigucar requested a review from jimmielin January 13, 2025 17:49
Copy link
Member

@jimmielin jimmielin left a comment

Choose a reason for hiding this comment

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

Thanks @cacraigucar for addressing my comments!

There are a few comments I left open to get @nusbaume and @peverwhee's input, about the history variables and inclusion of ZM in the CAM7 SDF. I've marked the other comments as resolved.

Copy link
Collaborator

@peverwhee peverwhee left a comment

Choose a reason for hiding this comment

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

hooray! zm! a few (hopefully) small things!

note: i didn't review the code in to_be_ccppized since I assumed it was copied over from CAM...

also, reminder that you'll want to regenerate NamesNotInDictionary.txt after all the reviews are done.

schemes/sima_diagnostics/zm_diagnostics.F90 Outdated Show resolved Hide resolved
schemes/sima_diagnostics/zm_diagnostics.F90 Outdated Show resolved Hide resolved
schemes/sima_diagnostics/zm_tendency_diagnostics.F90 Outdated Show resolved Hide resolved
schemes/zhang_mcfarlane/zm_conv_convtran.F90 Show resolved Hide resolved
schemes/zhang_mcfarlane/zm_conv_evap.meta Show resolved Hide resolved
Comment on lines 213 to 219

work2 = max(fsnow_conv(i,k), work1)
if (snowmlt(i).gt.0._kind_phys) work2 = 0._kind_phys
ntsnprd(i,k) = prdprec(i,k)*work2 - evpsnow(i) - snowmlt(i)
tend_s_snwprd (i,k) = prdprec(i,k)*work2*latice
ntsnprd(i,k) = prdprec_gen(i,k)*work2 - evpsnow(i) - snowmlt(i)
tend_s_snwprd (i,k) = prdprec_gen(i,k)*work2*latice
tend_s_snwevmlt(i,k) = - ( evpsnow(i) + snowmlt(i) )*latice
else
ntsnprd(i,k) = prdsnow(i,k) - min(flxsnow(i,k)*gravit/pdel(i,k), evpsnow(i)+snowmlt(i))
tend_s_snwprd (i,k) = prdsnow(i,k)*latice
tend_s_snwevmlt(i,k) = -min(flxsnow(i,k)*gravit/pdel(i,k), evpsnow(i)+snowmlt(i) )*latice
end if
Copy link
Collaborator

Choose a reason for hiding this comment

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

same with this block - if we're always using old_snow, can replace lines 207 - 219 with:

if (flxprec(i,k).gt.0._kind_phys) then
   work1 = min(max(0._kind_phys,flxsnow(i,k)/flxprec(i,k)),1._kind_phys)
else
   work1 = 0._kind_phys
endif

work2 = max(fsnow_conv(i,k), work1)
if (snowmlt(i).gt.0._kind_phys) work2 = 0._kind_phys
ntsnprd(i,k) = prdprec_gen(i,k)*work2 - evpsnow(i) - snowmlt(i)
tend_s_snwprd  (i,k) = prdprec_gen(i,k)*work2*latice
tend_s_snwevmlt(i,k) = - ( evpsnow(i) + snowmlt(i) )*latice

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I asked Rich Neale about this, but he has not responded. Since this could fit under "code cleanup", I am inclined to leave it. Also, ZM may be having some science changes coming in, so we could bring that up when that PR comes along.

Copy link
Collaborator

@peverwhee peverwhee Jan 16, 2025

Choose a reason for hiding this comment

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

ok! I guess I'd like to be sure we haven't lost functionality in CAM with this PR? since there used to be a way to make old_snow=.false. (by passing in prdsnow)

schemes/zhang_mcfarlane/zm_conv_momtran.F90 Outdated Show resolved Hide resolved
schemes/zhang_mcfarlane/zm_convr.F90 Outdated Show resolved Hide resolved
schemes/zhang_mcfarlane/zm_conv_evap.meta Show resolved Hide resolved
Copy link
Collaborator Author

@cacraigucar cacraigucar left a comment

Choose a reason for hiding this comment

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

I believe I have addressed all of @peverwhee's comments, but I may have missed one

schemes/zhang_mcfarlane/zm_convr.F90 Outdated Show resolved Hide resolved
schemes/zhang_mcfarlane/zm_conv_momtran.F90 Outdated Show resolved Hide resolved
schemes/zhang_mcfarlane/zm_conv_evap.F90 Outdated Show resolved Hide resolved
schemes/zhang_mcfarlane/zm_conv_evap.F90 Outdated Show resolved Hide resolved
schemes/sima_diagnostics/zm_diagnostics.F90 Outdated Show resolved Hide resolved
schemes/sima_diagnostics/zm_diagnostics.F90 Outdated Show resolved Hide resolved
schemes/zhang_mcfarlane/zm_conv_evap.meta Show resolved Hide resolved
Comment on lines 213 to 219

work2 = max(fsnow_conv(i,k), work1)
if (snowmlt(i).gt.0._kind_phys) work2 = 0._kind_phys
ntsnprd(i,k) = prdprec(i,k)*work2 - evpsnow(i) - snowmlt(i)
tend_s_snwprd (i,k) = prdprec(i,k)*work2*latice
ntsnprd(i,k) = prdprec_gen(i,k)*work2 - evpsnow(i) - snowmlt(i)
tend_s_snwprd (i,k) = prdprec_gen(i,k)*work2*latice
tend_s_snwevmlt(i,k) = - ( evpsnow(i) + snowmlt(i) )*latice
else
ntsnprd(i,k) = prdsnow(i,k) - min(flxsnow(i,k)*gravit/pdel(i,k), evpsnow(i)+snowmlt(i))
tend_s_snwprd (i,k) = prdsnow(i,k)*latice
tend_s_snwevmlt(i,k) = -min(flxsnow(i,k)*gravit/pdel(i,k), evpsnow(i)+snowmlt(i) )*latice
end if
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I asked Rich Neale about this, but he has not responded. Since this could fit under "code cleanup", I am inclined to leave it. Also, ZM may be having some science changes coming in, so we could bring that up when that PR comes along.

@@ -192,7 +176,7 @@ subroutine zm_conv_evap_run(ncol, pver, pverp, &
evplimit = min(evplimit, flxprec(i,k) * gravit / pdel(i,k))

! Total evaporation cannot exceed input precipitation
evplimit = min(evplimit, (prec(i) - evpvint(i)) * gravit / pdel(i,k))
evplimit = min(evplimit, (prec_gen(i) - evpvint(i)) * gravit / pdel(i,k))

evpprec(i) = min(evplimit, evpprec(i))
if( .not.old_snow ) then
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See response on other old_snow comment

@cacraigucar cacraigucar requested a review from peverwhee January 14, 2025 23:26
intent = in
advected = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops, should've clarified. advected = true is only for individual constituents (not the whole array). I imagine this "works" since you've tested it, but I'd remove this to avoid future confusion

Copy link
Collaborator

Choose a reason for hiding this comment

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

agh i'm annoyed that this comment ended up here rather than in my review.

Comment on lines 213 to 219

work2 = max(fsnow_conv(i,k), work1)
if (snowmlt(i).gt.0._kind_phys) work2 = 0._kind_phys
ntsnprd(i,k) = prdprec(i,k)*work2 - evpsnow(i) - snowmlt(i)
tend_s_snwprd (i,k) = prdprec(i,k)*work2*latice
ntsnprd(i,k) = prdprec_gen(i,k)*work2 - evpsnow(i) - snowmlt(i)
tend_s_snwprd (i,k) = prdprec_gen(i,k)*work2*latice
tend_s_snwevmlt(i,k) = - ( evpsnow(i) + snowmlt(i) )*latice
else
ntsnprd(i,k) = prdsnow(i,k) - min(flxsnow(i,k)*gravit/pdel(i,k), evpsnow(i)+snowmlt(i))
tend_s_snwprd (i,k) = prdsnow(i,k)*latice
tend_s_snwevmlt(i,k) = -min(flxsnow(i,k)*gravit/pdel(i,k), evpsnow(i)+snowmlt(i) )*latice
end if
Copy link
Collaborator

@peverwhee peverwhee Jan 16, 2025

Choose a reason for hiding this comment

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

ok! I guess I'd like to be sure we haven't lost functionality in CAM with this PR? since there used to be a way to make old_snow=.false. (by passing in prdsnow)

Copy link
Collaborator

@peverwhee peverwhee left a comment

Choose a reason for hiding this comment

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

a couple lingering things

intent = in
advected = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like this one has "advected = true" twice

Comment on lines +86 to +91
#[ dif ]
# standard_name = detrainment_of_cloud_ice_due_to_deep_convection
# units = kg kg-1 s-1
# type = real | kind = kind_phys
# dimensions = (horizontal_loop_extent,vertical_layer_dimension)
# intent = in
Copy link
Collaborator

Choose a reason for hiding this comment

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

this can be removed

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.

Add ccpp'ized ZM
5 participants