Skip to content

Complete CCPPization of UW shallow convection scheme (uwshcu) - #408

Open
jimmielin wants to merge 4 commits into
ESCOMP:mainfrom
jimmielin:hplin/uwshcu
Open

Complete CCPPization of UW shallow convection scheme (uwshcu)#408
jimmielin wants to merge 4 commits into
ESCOMP:mainfrom
jimmielin:hplin/uwshcu

Conversation

@jimmielin

@jimmielin jimmielin commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

Tag name (The PR title should also include the tag name):
Originator(s): @jimmielin
Assisted-by: claude-opus:4.6[1m]

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

  • Closes Convert UW shallow convection (uwshcu) #365 - CCPPizes UW shallow convection scheme (uwshcu)
  • Adds clamp_number_concentrations utility scheme to replicate CAM physics_types.F90 behavior that clamps num* constituents to [1e-12, 1e10] range.
  • Refactors convect_shallow_diagnostics_after_convective_evaporation into its own scheme file (previously inlined in convect_shallow_diagnostics) to accommodate the fact that UW does not use ZM convective evaporation.
  • Removes unused arguments from compute_cloud_fraction and rk_stratiform (cleanup)

List all namelist files that were added or changed:

A       schemes/uw_shallow/uw_convect_shallow_namelist.xml
  - namelist parameters for UW shallow convection

List all files eliminated and why: N/A

List all files added and what they do:

A       schemes/uw_shallow/uw_convect_shallow.F90
A       schemes/uw_shallow/uw_convect_shallow.meta
A       schemes/uw_shallow/uw_convect_shallow_namelist.xml
  - CCPPized UW shallow convection scheme.

A       schemes/sima_diagnostics/uw_convect_shallow_diagnostics.F90
A       schemes/sima_diagnostics/uw_convect_shallow_diagnostics.meta
  - diagnostics for UW shallow convection.

A       schemes/sima_diagnostics/convect_shallow_diagnostics_after_convective_evaporation.F90
A       schemes/sima_diagnostics/convect_shallow_diagnostics_after_convective_evaporation.meta
  - refactored out of convect_shallow_diagnostics into its own scheme file
    (Hack shallow-specific diagnostics after zm_conv_evap)

A       schemes/utilities/clamp_number_concentrations.F90
A       schemes/utilities/clamp_number_concentrations.meta
  - utility scheme to clamp number concentration constituents to [1e-12, 1e10]
    after apply_constituent_tendencies, replicating CAM physics_types.F90 behavior

A       test/test_suites/suite_convect_shallow_uw.xml
  - test SDF for UW shallow convection

List all existing files that have been modified, and describe the changes:

M       schemes/cloud_fraction/compute_cloud_fraction.F90
M       schemes/cloud_fraction/compute_cloud_fraction.meta
  - cleanup unused arguments, unused local variables

M       schemes/hack_shallow/hack_convect_shallow.F90
  - cleanup

M       schemes/rasch_kristjansson/rk_stratiform.F90
M       schemes/rasch_kristjansson/rk_stratiform.meta
  - cleanup

M       schemes/sima_diagnostics/convect_shallow_diagnostics.F90
M       schemes/sima_diagnostics/convect_shallow_diagnostics.meta
  - move convect_shallow_diagnostics_after_convective_evaporation to standalone file.

M       suites/suite_cam5.xml
  - add UW shallow convection scheme to CAM5 suite.

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:

This is a combination of 13 commits and dropped 1 commits.

Initial commit. Move to atmos_phys: uw_shallow

Build fixes for CAM

Continue work on meta

Tentative draft for meta

draft diags

Add convect_shallow_uw draft scheme

(wrong diag scheme name)

add nl file

Disable check_energy for now since snapshot runs have issues (same as ZM)

Add clamp_number_concentrations utility scheme.

Give up on using tendencies for the number concentration clamp

Various cleanup (req CAM changes)

Address review comments from code review agent (claude-opus-4.6)

Update CAM5 and test SDF for UW shallow

Update diagnostics
@jimmielin jimmielin self-assigned this Jun 4, 2026
@jimmielin jimmielin added the enhancement New feature or request label Jun 4, 2026
@jimmielin
jimmielin requested a review from nusbaume June 5, 2026 16:29
@nusbaume

Copy link
Copy Markdown
Collaborator

Automated review against project code-review rules

Reviewed the diff against our 12 code-review rules. Most of the diff is well-behaved; the two large findings both live in schemes/uw_shallow/uw_convect_shallow.F90 (ported legacy CAM code) and look like deliberate choices to confirm rather than oversights. One clean, easy fix exists in the new utility module.

Static review of the diff only — nothing was built or run.

# Rule Result
1 No module-level save ❌ 1 violation (clamp_number_concentrations.F90)
2 use at lowest scope
3 pure where applicable ✅ helpers correctly marked pure
4 end do/end if not enddo/endif ✅ only commented-out code retains old form
5 Modern Fortran syntax (> vs .gt.) ❌ 153 old-style operators in the ported scheme
6 Namelist XML standard header
7 allocate returns stat/errmsg ✅ N/A (no allocate added)
8 Citations have DOI ✅ DOI in file header
9 Grammatical full-sentence comments ✅ spot-checked, not exhaustive
10 Single-output subroutine → function ⚠️ 1 candidate (fluxbelowinv)
11 Character args len=* ⚠️ errmsg is len=512 everywhere (CCPP convention)
12 No goto/go to ❌ 34 live go to 333 (kept deliberately, documented)

Findings

1. save at module level — Rule 1 ❌ (easy fix)
schemes/utilities/clamp_number_concentrations.F90:12 has a bare save after private. Module variables already carry implicit SAVE semantics, so the keyword is both non-compliant and redundant — recommend removing it. No other new module uses save.

2. Old-style relational operators — Rule 5 ❌ (widespread)
schemes/uw_shallow/uw_convect_shallow.F90 has 153 occurrences of .eq. .ne. .lt. .le. .gt. .ge. in live code (e.g. if (id_check .eq. 1), if (plcl .lt. 30000._kind_phys)). Understandable if the goal is bit-for-bit fidelity with the original CAM source, but it does not satisfy Rule 5. Please either convert to ==//=/</<=/>/>= or explicitly waive Rule 5 for this ported file and note why.

3. go to statements — Rule 12 ❌ (deliberate, documented)
schemes/uw_shallow/uw_convect_shallow.F90 has 34 live go to 333 targeting a single early-exit label. The authors documented the choice (≈ line 4455): "we later agreed this is one of the legitimate uses of the go to construct without introducing an unnecessary main loop and flags to exit, so this was kept here." Still a literal Rule 12 deviation — flagging for a human sign-off, not blocking.

4. Single-output subroutine — Rule 10 ⚠️ (minor)
uw_convect_shallow.F90:4917 pure subroutine fluxbelowinv(...)xflx is the sole output; per Rule 10 it could be a function. Low priority (returns an array, ported code).

5. errmsg character length — Rule 11 ⚠️ (likely intentional convention)
Every new scheme uses character(len=512), intent(out) :: errmsg. Rule 11 asks for len=*, but this fixed length is the established CCPP ccpp_error_message convention repo-wide, so probably not actionable unless we revisit the convention.

What's clean

  • Rule 4: hack_convect_shallow.F90 actively converts endifend if; remaining hits are commented-out code.
  • Rule 3: the pure helpers (exnf, getbuoy, single_cin, roots, slope, compute_alpha, compute_ppen, fluxbelowinv) are correctly labeled pure.
  • Rule 6: uw_convect_shallow_namelist.xml includes the standard documentation comment block.
  • Rule 8: the scheme header carries a full citation with DOI (https://doi.org/10.1175/2008JCLI2557.1).
  • Rule 2: use statements are scoped appropriately.
  • The compute_cloud_fraction / rk_stratiform changes are pure argument cleanup — no rule concerns.

Extra (outside the 12 rules): clamp_number_concentrations.F90:108 declares q_projected, which is only referenced in the commented-out alternative block below it — currently unused.


Bottom line: one quick fix (remove save), plus three items in the ported uw_convect_shallow.F90 (.gt.-style operators, go to, errmsg length) that are deliberate/convention-driven and need a maintainer decision rather than a blind edit. Nothing here looks like an accidental defect.

🤖 Generated with Claude Code

bogbot_arr_out(i, mkx:1:-1) = bogbot_arr(:mkx)
bogtop_arr_out(i, mkx:1:-1) = bogtop_arr(:mkx)

! NOTE (hplin): when bringing this code to CAM-SIMA, it was discussed whether

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 believe the goto 333 can be replaced with a subroutine/function call to do all of the setting of the code followed by a cycle column_loop. I think this should be synonymous with what is being done now with the goto's

Copy link
Copy Markdown
Collaborator 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. There is one goto 333 that cannot be eliminated this way.
At line 2072 it sets id_exit = .false. first directly after the block that assigns all the real outputs from the *_s arrays. It jumps to mark the column as success, not error, skipping the tail reset block, so it cannot be untangled without changing answers or making a bigger contraption.

I am happy to revisit this later. Just didn't want to take the chance to mess with answers after it is bit-for-bit already.

@jimmielin

Copy link
Copy Markdown
Collaborator Author

Thanks @nusbaume. This is ready for human review.

A couple of notes from the AI review that I did not resolve:

  • The go to 333 has one special case that differs from the others and it's not easily untangled so I kept it as-is.
  • I did not make fluxbelowinv a function. It works fine as-is, but I made it pure.

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.

Convert UW shallow convection (uwshcu)

4 participants