Skip to content

Complete CCPPization of CAM5 UW PBL scheme (diag_TKE)#370

Open
jimmielin wants to merge 25 commits into
ESCOMP:mainfrom
jimmielin:hplin/diag_tke_rebase_main
Open

Complete CCPPization of CAM5 UW PBL scheme (diag_TKE)#370
jimmielin wants to merge 25 commits into
ESCOMP:mainfrom
jimmielin:hplin/diag_tke_rebase_main

Conversation

@jimmielin
Copy link
Copy Markdown
Member

@jimmielin jimmielin commented Mar 9, 2026

Tag name (The PR title should also include the tag name):
Originator(s): @jimmielin

Companion PR in CAM - ESCOMP/CAM#1445

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

  • Completes Convert UW PBL (diag_TKE) scheme #330 - CCPPizes UW PBL scheme (diag_TKE)
  • CCPPize UW interstitial to adjust diffusivity above PBL
  • Rearranging of interstitials for vertical diffusion, to accommodate for using either HB or UW PBL: new interstitial for handling Coords1D and potential temperature; new interstitial for computing kinematic water vapor flux and Obklen.
  • New stub to only zero out Beljaars, not TMS - used for CAM5 snapshot tests (TMS on, Beljaars off)

List all namelist files that were added or changed:

A       schemes/bretherton_park/bretherton_park_diff_namelist.xml
A       schemes/bretherton_park/eddy_diffusivity_adjustment_above_pbl_namelist.xml
  - for UW PBL

List all files eliminated and why: N/A

List all files added and what they do:

A       schemes/bretherton_park/bretherton_park_diff.F90
A       schemes/bretherton_park/bretherton_park_diff_namelist.xml
  - initial CCPPized subroutines for UW PBL

A       schemes/bretherton_park/eddy_diff.F90
  - moved from CAM (main driver module for UW PBL)

A       schemes/bretherton_park/eddy_diffusivity_adjustment_above_pbl.F90
A       schemes/bretherton_park/eddy_diffusivity_adjustment_above_pbl.meta
A       schemes/bretherton_park/eddy_diffusivity_adjustment_above_pbl_namelist.xml
  - mini-scheme used by the end of UW PBL

A       schemes/vertical_diffusion/vertical_diffusion_interstitials.F90
A       schemes/vertical_diffusion/vertical_diffusion_interstitials.meta
  - new interstitial for common vertical diffusion schemes
  - new interstitial for computing kinematic fluxes and Obukhov length

A       test/test_suites/suite_vdiff_bretherton_park.xml
  - SDF file

A       schemes/sima_diagnostics/bretherton_park_diff_diagnostics.F90
A       schemes/sima_diagnostics/bretherton_park_diff_diagnostics.meta
  - diagnostics for UW PBL

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

M       schemes/holtslag_boville/holtslag_boville_diff.F90
M       schemes/holtslag_boville/holtslag_boville_diff.meta
M       schemes/holtslag_boville/holtslag_boville_diff_interstitials.F90
M       schemes/holtslag_boville/holtslag_boville_diff_interstitials.meta
  - rearrange interstitials

M       schemes/sima_diagnostics/diffusion_solver_diagnostics.F90
  - comment update to clarify which surface stress is used here

M       schemes/vertical_diffusion/diffusion_solver.F90
M       schemes/vertical_diffusion/diffusion_solver.meta
  - init phase: changes for dropmixnuc (ndrop vertical mixing)-handled species to skip diffusion solver for tracers

M       schemes/vertical_diffusion/diffusion_stubs.F90
M       schemes/vertical_diffusion/diffusion_stubs.meta
  - new stub to only zero out Beljaars, not TMS
  - new stub for handling surface fluxes for dropmixnuc (ndrop vertical mixing)-handled species

M       suites/suite_cam4.xml
M       test/test_suites/suite_vdiff_holtslag_boville.xml
  - rearrange interstitials

M       schemes/sima_diagnostics/diffusion_solver_diagnostics.F90
M       schemes/sima_diagnostics/diffusion_solver_diagnostics.meta
  - standard name update for clarity

List all automated tests that failed, as well as an explanation for why they weren't fixed:

Is this an answer-changing PR? If so, is it a new physics package, algorithm change, tuning change, etc?

If yes to the above question, describe how this code was validated with the new/modified features:

Copy link
Copy Markdown
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

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

Thank you so much for helping bring in our first (!) CAM5 physics scheme @jimmielin! I realize I have what look like a ton of comments, but hopefully the vast majority of them are easy to resolve. Of course if you have any questions, comments, or concerns with any of my suggestions then just let me know. Thanks again!

Comment thread schemes/bretherton_park/bretherton_park_diff.F90
Comment thread schemes/bretherton_park/bretherton_park_diff.F90 Outdated
use ccpp_constituent_prop_mod, only: ccpp_constituent_prop_ptr_t

! dependency to get constituent index
use ccpp_const_utils, only: ccpp_const_get_idx
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we use the CCPP-framework equivalent for this?

Suggested change
use ccpp_const_utils, only: ccpp_const_get_idx
use ccpp_scheme_utils, only: ccpp_const_get_idx=>ccpp_constituent_index

This way we can eventually get rid of the helper scheme we currently have in atmospheric_physics.

Copy link
Copy Markdown
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 suggestion! I think this doesn't have an equivalent stub in CAM yet - https://github.com/ESCOMP/CAM/blob/cam_development/src/utils/cam_ccpp/ccpp_scheme_utils.F90 is a no-op stub right now. I am happy to write the stub but that adds yet another tangential fix to the stub which will be a nightmare to my house of cards.

I added a CAM issue to write the stub: ESCOMP/CAM#1541

The atmos_phys issue is existing: #215 - I can volunteer at the end of CAM5 to sweep through all of these once the stub is in, if that is OK?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the response @jimmielin! I'm leaving this comment open so that we remember, but otherwise will consider it resolved for the purposes of this PR.

Comment thread schemes/bretherton_park/bretherton_park_diff.F90 Outdated
Comment thread schemes/bretherton_park/bretherton_park_diff.F90 Outdated
Comment thread schemes/sima_diagnostics/bretherton_park_diff_diagnostics.F90
Comment thread suites/suite_cam4.xml
Comment thread test/test_schemes/initialize_constituents.F90 Outdated
Comment thread test/test_suites/suite_vdiff_bretherton_park.xml Outdated
Comment thread test/test_suites/suite_vdiff_bretherton_park.xml Outdated
nusbaume

This comment was marked as duplicate.

jimmielin and others added 2 commits April 21, 2026 17:11
@jimmielin jimmielin requested a review from nusbaume April 21, 2026 21:12
Copy link
Copy Markdown
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

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

Thanks for responding to all my comments @jimmielin! I just had a few final questions/suggestions.

Comment thread schemes/bretherton_park/bretherton_park_diff.F90 Outdated
use ccpp_constituent_prop_mod, only: ccpp_constituent_prop_ptr_t

! dependency to get constituent index
use ccpp_const_utils, only: ccpp_const_get_idx
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the response @jimmielin! I'm leaving this comment open so that we remember, but otherwise will consider it resolved for the purposes of this PR.

Comment thread schemes/bretherton_park/bretherton_park_diff.meta
# these are for the diffusion solver to compute implicit surface stress.
# for UW PBL, this is taux = wsx; tauy = wsy
# currently handled by hb_prepare_vertical_diffusion_inputs run phase.
#[ taux ]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the explanation! Maybe we can get a scientist's input on this whenever we open the "future work" PR. For now I'll leave it open so we can find this again, but will consider it "resolved" for this particular PR.

Comment thread schemes/bretherton_park/bretherton_park_diff_namelist.xml
Comment on lines +105 to +106
type(ccpp_constituent_prop_ptr_t), &
intent(in) :: const_props(:) ! CCPP constituent properties pointer
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm going to leave this comment open so that we can remember to remove the const_props input once we do the constituent subroutine update, but will consider this "resolved" for this particular PR. Thanks!

Comment thread schemes/sima_diagnostics/bretherton_park_diff_diagnostics.F90 Outdated
Comment thread schemes/sima_diagnostics/bretherton_park_diff_diagnostics.F90
Comment thread suites/suite_cam5.xml
Comment thread suites/suite_cam5.xml Outdated
@jimmielin jimmielin requested a review from nusbaume April 24, 2026 18:36
Copy link
Copy Markdown
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

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

Thanks again for all of this work @jimmielin! I had one last comment-fix that was missed from earlier, but otherwise everything looks great to me. Thanks again!

Comment thread schemes/bretherton_park/eddy_diffusivity_adjustment_above_pbl_namelist.xml Outdated
@nusbaume nusbaume requested a review from peverwhee April 24, 2026 21:32
Copy link
Copy Markdown
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.

Apologies for the delay on this! A few (mostly aesthetic) requests.

Comment thread schemes/bretherton_park/bretherton_park_diff.F90
Comment thread schemes/bretherton_park/bretherton_park_diff.F90 Outdated
Comment thread schemes/bretherton_park/bretherton_park_diff.F90 Outdated
Comment thread schemes/bretherton_park/bretherton_park_diff.F90 Outdated
Comment thread schemes/bretherton_park/bretherton_park_diff.F90
Comment thread schemes/bretherton_park/bretherton_park_diff.F90
Comment thread schemes/bretherton_park/bretherton_park_diff.F90
Comment thread schemes/bretherton_park/bretherton_park_diff.F90 Outdated
Comment thread schemes/bretherton_park/bretherton_park_diff.F90 Outdated
Comment thread schemes/holtslag_boville/holtslag_boville_diff_interstitials.F90 Outdated
@jimmielin jimmielin requested a review from peverwhee May 12, 2026 03:31
@jimmielin
Copy link
Copy Markdown
Member Author

Thanks @nusbaume and @peverwhee for your reviews and comments!

This should be ready for merging once a CAM tag is assigned. I did a test run of Nag tests:

  • GNU all passed
  • Nag all passed except the known failure subcolumn test

When Derecho is back I can do a test run there as well to ensure atmos_phys is all good then it'll just have to wait for its turn in the CAM tagging queue.

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.

3 participants