Skip to content

PUMAS round3#357

Open
cacraigucar wants to merge 65 commits into
ESCOMP:mainfrom
nusbaume:pumas_round3
Open

PUMAS round3#357
cacraigucar wants to merge 65 commits into
ESCOMP:mainfrom
nusbaume:pumas_round3

Conversation

@cacraigucar

@cacraigucar cacraigucar commented Jan 16, 2026

Copy link
Copy Markdown
Collaborator

Originator(s): cacraigucar, nusbaume, billsacks, peverwhee, I-Kuan Hu, jimmielin

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

  • PUMAS code now has metadata and goes through the CAM-SIMA compilation step. [Note PUMAS diagnostics will come in a later stage] - This step has been named "round3".
  • This commit contains contributions from participants at the PUMAS hackathon
  • Partially addresses CCPP-ize PUMAS #118

List all namelist files that were added or changed:
A schemes/pumas/pumas_pre_main_namelist.xml
- pumas CAM-SIMA namelist generator

List all files eliminated and why: N/A

List all files added and what they do:
A schemes/pumas/micro_pumas_ccpp_dimensions_post.F90
A schemes/pumas/micro_pumas_ccpp_dimensions_post.meta
A schemes/pumas/micro_pumas_ccpp_dimensions_pre.F90
A schemes/pumas/micro_pumas_ccpp_dimensions_pre.meta
- code which subsets and converts to pumas_r8 and back

A schemes/pumas/pumas_pre_main.F90
A schemes/pumas/pumas_pre_main.meta
- placeholder for interstitial schemes

A schemes/sima_diagnostics/pumas_diagnostics.F90
A schemes/sima_diagnostics/pumas_diagnostics_calling.F90
- These are being included even though they are only partially complete - NO REVIEW IS BEING DONE NOW

A test/test_suites/suite_pumas.xml
- test suite for PUMAS

List all existing files that have been modified, and describe the changes:
(Helpful git command: git diff --name-status development...<your_branch_name>)
M .gitmodules
M doc/NamesNotInDictionary.txt
M schemes/pumas/pumas

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?
- new capability in CAM-SIMA

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

@cacraigucar
cacraigucar marked this pull request as draft January 16, 2026 21:11
@cacraigucar
cacraigucar requested a review from nusbaume June 18, 2026 21:38
@cacraigucar
cacraigucar marked this pull request as ready for review June 18, 2026 21:38
@jimmielin

Copy link
Copy Markdown
Collaborator

Please note that since compute_cloud_fraction_two_moment has been merged, rhmini in micro_pumas_ccpp can now use its value via the namelist and the hardcoded value in registry.xml for CAM-SIMA can also be removed. Please see nusbaume/PUMAS#1

jimmielin and others added 20 commits July 14, 2026 16:39
… pumas_post_main

Mirrors CAM micro_pumas_cam post-call recomputation, which discards the
per-substep values PUMAS returns. Match dei standard name with RRTMGP and
the CAM-SIMA registry (singular crystal). Run pumas_post_main before
apply_heating_rate (air density uses the pre-heating temperature) and add
the low stratiform cloud fraction optics limiter to the suite.
Mirrors CAM micro_pumas_cam, including the separate meters-to-microns
conversion step. Match snow diameter standard name with RRTMGP and the
CAM-SIMA registry (snow_crystal).
Updates for bit-for-bit fields that interact with RRTMGP
atmos_phys0_26_000: Move ndrop_bam, nucleate_ice, and other microp_aero interstitials to atmos_phys (ESCOMP#411)
@nusbaume

Copy link
Copy Markdown
Collaborator

Automated code review (PR #357 — PUMAS round3)

This is an AI-assisted review (claude-opus:4.8) against the rules in doc/Code-review.md, doc/Code-style.md, and AGENTS.md. Per the PR description, pumas_diagnostics.F90 and pumas_diagnostics_calling.F90 are explicitly not being reviewed at this stage, so I've excluded them (I did note a couple of things about them at the bottom for later). Auto-generated doc/NamesNotInDictionary.txt and the submodule pointer/.gitmodules bumps were not reviewed.

High priority

  1. Missing implicit noneschemes/pumas/micro_pumas_ccpp_dimensions_post.F90 (Code-style MUST). The module preamble goes straight from the use statements to public :: with no implicit none. Every module must have implicit none in the preamble.

  2. errmsg must be character(len=*), not character(len=512) (Code-review rule 11). The rule explicitly covers CCPP-framework variables such as errmsg. This affects:

    • schemes/pumas/micro_pumas_ccpp_dimensions_pre.F90 (both subroutines)
    • schemes/pumas/micro_pumas_ccpp_dimensions_post.F90
    • schemes/pumas/micro_pumas_ccpp_optics_limiter.F90
    • schemes/pumas/pumas_pre_main.F90 (both subroutines)
    • schemes/pumas/pumas_post_main.F90
    • schemes/utilities/clamp_number_concentrations.F90 (both subroutines)
    • test/test_schemes/clubb_stub.F90

    For reference, schemes/microp_aero/ccpp/hetfrz_classnuc_stub.F90 (added in this same PR) already does this correctly with character(len=*).

  3. Module-level saveschemes/utilities/clamp_number_concentrations.F90:13 (Code-review rule 1). Module variables are implicitly saved, so the explicit module-level save is redundant and should be removed.

Medium priority

  1. endifend if (Code-review rule 4)schemes/pumas/pumas_pre_main.F90:46 (the if (do_clubb_sgs) block).

  2. Commented-out code (Code-review rule 13) — schemes/utilities/clamp_number_concentrations.F90. The _run subroutine ends with a large block of commented-out alternative implementation (the q_projected = ... formulation, ~30 lines). The explanatory rationale is valuable, but per rule 13 the dead code itself should be removed (keep the explanation as prose if you want the reasoning preserved). Note this also leaves the declared local q_projected unused in the active code.

  3. Namelist XML missing the standard comment block (Code-review rule 6). Neither new namelist file has the standard <!-- Each namelist variable is defined in an <entry> element... --> block that follows <entry_id_pg version="2.0"> in the other namelist XMLs (e.g. dadadj_namelist.xml):

    • schemes/microp_aero/ccpp/hetfrz_classnuc_stub_namelist.xml
    • schemes/pumas/pumas_pre_main_namelist.xml
  4. Missing default private + explicit public (Code-style: modules are default private with explicit public interfaces).

    • schemes/pumas/micro_pumas_ccpp_dimensions_pre.F90 (no private, no public)
    • schemes/pumas/micro_pumas_ccpp_dimensions_post.F90 (has public but no private)
    • schemes/pumas/pumas_pre_main.F90 (no private, no public)

Low priority / for human review

  1. Comment grammar/typo (Code-review rule 9) — schemes/pumas/pumas_pre_main.F90 header. "The hope that there will eventually be one set of portable interstitials..." is a sentence fragment (missing "is" — "The hope is that..."), and "intersititals" is misspelled (→ "interstitials").

  2. schemes/sima_diagnostics/pumas_diagnostics.F90-keep_for_run (2150 lines added). This file has a non-.F90 extension and appears to be a leftover working/backup copy committed by accident (GitHub can't even diff it). It falls under the diagnostics work you've flagged as out-of-review, but a -keep_for_run file almost certainly should not be committed to the repo — please confirm/remove.

  3. (Optional, Code-review rule 3 — pure.) The pure data-reshaping converters do no I/O and have no side effects, so they are candidates for the pure keyword: micro_pumas_ccpp_dimensions_pre_*, micro_pumas_ccpp_dimensions_post_run, micro_pumas_ccpp_optics_limiter_run, and clamp_number_concentrations_run. (pumas_post_main_run calls external size_dist_param_* routines, so it likely can't be pure.) Flagging as optional since it isn't universal practice for CCPP entry points here.

  4. (Minor, Code-style modern syntax.) schemes/utilities/clamp_number_concentrations.F90:~112 uses the old array-constructor (/ ... /); prefer [ ... ].

Notes (not blocking)

  • The effective_radius_of_stratiform_cloud_{liquid_water,ice}_particle..._droplet/..._crystal and effective_diameter_..._particle_for_radiation..._crystal_for_radiation renames are applied consistently across rk_stratiform.{F90,meta}, rrtmgp_{lw,sw}_cloud_optics.meta, and rk_stratiform_diagnostics.{F90,meta}, with units unchanged (um). No rule-14 (same standard_name, differing units) conflicts were found in the modified .meta files.
  • The new mass_number_concentration_*_{droplets,drops,crystals,particles}_in_moist_air_and_condensed_water names in initialize_constituents.F90 are consistent with their use in clamp_number_concentrations.F90. As a reminder (AGENTS.md), any newly coined standard names should be explicitly called out in the PR description for SE review.
  • For later: pumas_diagnostics.F90 and pumas_diagnostics_calling.F90 also carry module-level save statements (rule 1) — worth addressing when those files come up for review.

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.

7 participants