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

CCPPize Hack shallow convection #195

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

Conversation

jimmielin
Copy link
Member

@jimmielin jimmielin commented Jan 27, 2025

Originator(s): @jimmielin

Summary (include the keyword ['closes', 'fixes', 'resolves'] and issue number):

Describe any changes made to the namelist:

  • new hack shallow convection namelist options cmfmca_namelist.xml
  • move ZM namelist options to zm_conv_options_namelist.xml

List all files eliminated and why: N/A

List all files added and what they do:

CCPPize Hack shallow convection
A       schemes/hack_shallow/cmfmca.F90
A       schemes/hack_shallow/cmfmca.meta
A       schemes/hack_shallow/cmfmca_namelist.xml
A       test/test_suites/suite_convect_shallow_hack.xml
A       schemes/sima_diagnostics/convect_shallow_diagnostics.F90
A       schemes/sima_diagnostics/convect_shallow_diagnostics.meta

Interstitial schemes for zm_conv_evap
A       schemes/hack_shallow/set_general_conv_fluxes_to_shallow.F90
A       schemes/hack_shallow/set_general_conv_fluxes_to_shallow.meta
A       schemes/hack_shallow/set_shallow_conv_fluxes_to_general.F90
A       schemes/hack_shallow/set_shallow_conv_fluxes_to_general.meta

Interstitial schemes to combine shallow and deep convective fluxes
A       schemes/hack_shallow/convect_shallow_sum_to_deep.F90
A       schemes/hack_shallow/convect_shallow_sum_to_deep.meta

Move ZM namelist options to zm_conv_options scheme
A       schemes/zhang_mcfarlane/zm_conv_options.F90
A       schemes/zhang_mcfarlane/zm_conv_options.meta

Update cloud_fraction_fice to accept top_lev registry quantity (vertical_layer_index_of_cloud_fraction_top)
Add new init-phase only scheme to assign top_lev to trop_cloud_top_lev for CAM5+ macrophysics
M       schemes/cloud_fraction/cloud_fraction_fice.F90
M       schemes/cloud_fraction/cloud_fraction_fice.meta
A       schemes/cloud_fraction/set_cloud_fraction_top.F90
A       schemes/cloud_fraction/set_cloud_fraction_top.meta

Change CMFMC_DP units for consistency with snapshot; update rliq metadata unit
M       schemes/zhang_mcfarlane/zm_convr.F90
M       schemes/zhang_mcfarlane/zm_convr.meta
M       schemes/sima_diagnostics/zm_diagnostics.F90
M       schemes/sima_diagnostics/zm_diagnostics.meta

List all existing files that have been modified, and describe the changes:
(Helpful git command: git diff --name-status development...<your_branch_name>)

Freeze ChangeLog
M       doc/ChangeLog

Move ZM namelist options to zm_conv_options scheme
R100    schemes/zhang_mcfarlane/zm_convr_namelist.xml   schemes/zhang_mcfarlane/zm_conv_options_namelist.xml
M       suites/suite_cam7.xml
M       test/test_suites/suite_zhang_mcfarlane.xml

Update cloud_fraction_fice to accept top_lev registry quantity (vertical_layer_index_of_cloud_fraction_top)
M       schemes/cloud_fraction/cloud_fraction_fice.F90
M       schemes/cloud_fraction/cloud_fraction_fice.meta
M       suites/suite_cam7.xml
M       test/test_suites/suite_zhang_mcfarlane.xml

List any test failures:

Is this a science-changing update? New physics package, algorithm change, tuning changes, etc?
New Hack shallow convection scheme

@jimmielin jimmielin added the enhancement New feature or request label Jan 27, 2025
@jimmielin jimmielin self-assigned this Jan 27, 2025
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.

nothing massive - just a few requests!

thanks @jimmielin woo! hack shallow!

Comment on lines +4 to +7
! Hack shallow convective scheme.
!
! Original Author: J. Hack
! CCPPized: Haipeng Lin, October 2024
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can I request porting the full comment over from CAM, to preserve that history?

Suggested change
! Hack shallow convective scheme.
!
! Original Author: J. Hack
! CCPPized: Haipeng Lin, October 2024
!-----------------------------------------------------------------------
!
! Purpose:
! Moist convective mass flux procedure:
!
! Method:
! If stratification is unstable to nonentraining parcel ascent,
! complete an adjustment making successive use of a simple cloud model
! consisting of three layers (sometimes referred to as a triplet)
!
! Code generalized to allow specification of parcel ("updraft")
! properties, as well as convective transport of an arbitrary
! number of passive constituents (see q array). The code
! is written so the water vapor field is passed independently
! in the calling list from the block of other transported
! constituents, even though as currently designed, it is the
! first component in the constituents field.
!
! Author: J. Hack
!
! BAB: changed code to report tendencies in cmfdt and dq, instead of
! updating profiles. Cmfdq contains water only, made it a local variable
! made dq (all constituents) the argument.
!
! CCPPized: Haipeng Lin, October 2024
!-----------------------------------------------------------------------

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks! Added to above cmfmca_run (lightly edited to collapse in comment by BAB)

Comment on lines 111 to 116
! in-module parameters (hardcoded)
dzmin = 0.0_kind_phys
betamn = 0.10_kind_phys

! specify that relaxation timescale should be applied to column as opposed to triplets individually
rlxclm = .true.
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you just set these above at the module level, as you do ssfac, etc?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, done!

integer, intent(in) :: pcnst ! number of ccpp constituents
type(ccpp_constituent_prop_ptr_t), &
intent(in) :: const_props(:) ! ccpp constituent properties pointer
integer, intent(in) :: nstep ! current time step index
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is used - can remove from argument list and metadata.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks!

Comment on lines 156 to 157
! temporary for debug
use cam_logfile, only: iulog
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be an input argument to cmfmca_run ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, thanks! Updated

@@ -0,0 +1,281 @@
[ccpp-table-properties]
name = cmfmca
type = scheme
Copy link
Collaborator

Choose a reason for hiding this comment

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

i believe this should include the wv_saturation dependency

dependencies = ../../to_be_ccppized/wv_saturation.F90

Copy link
Member Author

Choose a reason for hiding this comment

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

Added, thank you!

Comment on lines 323 to 329
const_check_loop: do m = 1, pcnst
call const_props(m)%standard_name(const_standard_name)
if (const_standard_name == 'water_vapor_mixing_ratio_wrt_moist_air_and_condensed_water') then
const_wv_idx = m
exit const_check_loop
endif
enddo const_check_loop
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd recommend using the ccpp_const_get_idx in ccpp_const_utils that you added. I hope that'll make it easier to find and replace with the framework version eventually...

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, updated!

Comment on lines 29 to 30
<!-- FIXME: This is a resolution-dependent value -->
<value>1.0e-4</value>
Copy link
Collaborator

@peverwhee peverwhee Jan 28, 2025

Choose a reason for hiding this comment

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

is this a "FIXME before merging this PR" or a "FIXME eventually when we have some capability"? If it's the latter, I'd suggest a comment describing what we need to implement

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question... it was more of a reminder that eventually the resolution-dependent options need to be ported, but from the CAM-SIMA docs on namelist XML files it looks like this work is implied: so I got rid of the comment.

When populating the value fields, you don't need to port all of them at this time. You need to put in the value that is used by default by CAM, but the rest will be ported once all schemes have been ported.

call history_out_field('CMFDQR', cmfdqr)

! Calculate fractional occurrence of shallow convection
! FIXME (hplin): this definition looks counter-intuitive but is replicated verbatim from convect_shallow.F90. To check.
Copy link
Collaborator

Choose a reason for hiding this comment

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

did you check? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the reminder, I did not! @adamrher, could you please help check if this is correct? This is from convect_shallow.F90 - The definition of this just feels backwards to me, since it marks a column as having shallow convection when the shallow convection cloud mass flux is zero.

    freqsh(:) = 0._kind_phys
    do i = 1, ncol
      if(maxval(cmfmc_sh(i,:)) <= 0._kind_phys) then
        freqsh(i) = 1._kind_phys
      endif
    enddo
    call history_out_field('FREQSH', freqsh)

Choose a reason for hiding this comment

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

Hi @jimmielin sorry I missed this git prompt. That is indeed strange -- a value of 1 should be assigned to the column when there is shallow convection. The comment above it "! Modification : I should check whether below computation of freqsh is correct." suggests we aren't the only one who thinks so (can you git blame this comment?).

I looked through hk_conv.F90 and verified that the array (cmfmc2, which I presume is cmfmc_sh in your code) cannot be negative. There are explicit limiters on the components that are summed and multiplied to give cmfmc2: beta and eta:

https://github.com/ESCOMP/CAM/blob/6eb3046284ef57385ca7612165465668c97ec1f5/src/physics/cam/hk_conv.F90#L697.

And beta is limited using beta(i) = max(0.0_r8,beta(i)) statements anytime it's modified. So I believe this is a bug in this particular diagnostic, and that the logic should be:

if (maxval(cmfmc_sh(i,:)) > 0._kind_phys) then

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @adamrher! git blame shows that comment is before the Git era, unfortunately.

I've opened an issue in CAM: ESCOMP/CAM#1254. I can work on a PR to fix this (probably a separate one from the CCPPization PR, so that the bit-for-bit property of the CCPPization PR can be preserved.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I went back to svn and a blame there yields:
mvr 4/29/2010 11:49:39 AM updated the physics with code from the cam5 development branch; some changes to the dynamics, including the enabling of fourth-order divergence damping
I don't remember who mvr is, but that person was the person who did the very first svn commit in CAM, so an experienced programmer. Maybe Matt Rothstein (or maybe I'm making up names!)

Choose a reason for hiding this comment

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

That was Matt's username. Oh well. Diagnostics can be tough to trouble shoot as the simulation may still be bfb.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @cacraigucar and @swrneale. It looks like FREQSH is not in the default field list for CAM4 so indeed if I change the definition of this diagnostic it will still show up as b4b in the regression tests. Anyway, I have fixed it on the CAM-SIMA end first (which has separate diagnostics code from current CAM) and can address it in ESCOMP/CAM separately.

Choose a reason for hiding this comment

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

On second thought, it might not be worth the extra time fixing this diagnostic in ESCOMP/CAM, before you merge the PR to ESCOMP/CAM to call Hack from the atmospheric_physics version of convect_shallow.F90.

But I'm confused on one aspect. If we were running non-CCPP' cam5 physics in ESCOMP/CAM, which is also impacted by this bug, would it then use the convect_shallow in atmospheric_physics? Or are you separating just the cam4 parts out of convect_shallow for this Hack shallow task?

Copy link
Member Author

@jimmielin jimmielin Feb 11, 2025

Choose a reason for hiding this comment

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

My apologies for the confusion! For this freqsh definition in particular, the code used for calculating it in CAM-SIMA is separate because it is in the "diagnostic" scheme, where the non-portable calls to the CAM history code reside. (schemes/sima_diagnostics/convect_shallow_diagnostics.F90, line ~191). The freqsh definition in ESCOMP/CAM will remain in convect_shallow.F90. So I would have to fix it in both atmospheric_physics and ESCOMP/CAM.

There is no direct equivalent to convect_shallow.F90 in atmos_phys -- hk_conv.F90 (subroutine cmfmca) for Hack shallow is CCPPized (into cmfmca.F90), so the only part that is shared by CAM and CAM-SIMA is the cmfmca_run and cmfmca_init subroutines.

There's a variety of other schemes in this PR, e.g., schemes/hack_shallow/convect_shallow_sum_to_deep.F90 (combine shallow and deep convective fluxes to create "all" convective fluxes) and the diagnostics scheme (calls history_add_field and outfld) that do similar things as what convect_shallow.F90 and convect_diagnostics.F90 do, but in the SIMA way, so they're not used in ESCOMP/CAM (because history calls are different).

I'm happy to elaborate further if you have any questions. I realize this may be easier to explain once I open the ESCOMP/CAM PR and you will be able to see what's going on.

Choose a reason for hiding this comment

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

OK I understand now. Thanks for straightening me out and I agree we'll need fix it in ESCOMP/CAM.

I actually really like this idea of dissolving the whole concept convect_shallow into portable interfaces and interstitials. It really helps clarify what parts of the code are used.

@peverwhee peverwhee requested a review from nusbaume January 29, 2025 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants