Skip to content

Merge stochastic physics (SPPT only) into GSL's develop branch#239

Open
gsketefian wants to merge 198 commits into
ufs-community:noaa/developfrom
dtcenter:gsl/MPAS_stoch_physics_try_merge_stoch_master
Open

Merge stochastic physics (SPPT only) into GSL's develop branch#239
gsketefian wants to merge 198 commits into
ufs-community:noaa/developfrom
dtcenter:gsl/MPAS_stoch_physics_try_merge_stoch_master

Conversation

@gsketefian
Copy link
Copy Markdown

@gsketefian gsketefian commented Mar 31, 2026

This PR enables use of the SPPT scheme of stochastic physics into MPAS-Model. This is addressed in Issue #204.

Main changes:

  1. Bring in the stochastic_physics code as a submodule.
  2. Make changes to module files, Makefiles, registry xml files, and fortran files, and GitHub workflow files to allow the SPPT stochastic scheme to be used with MPAS.
  3. Adds the stochastic_physics code as a submodule. There is a companion PR into the authoritative stochastic_physics repo here.

This PR adds a new GitHub Actions CI workflow (run_mpas_stoch.yml) that runs two tests of MPAS-Model on a winter case using GFS ICs/LBCs and with SPPT enabled. The first test is the "convection_permitting" physics suite, and the second is with the "hrrrv5" suite. These two tests use the same grid, static file, ICs, LBCs, etc as the existing CI test(s) for the winter case with GFS ICs/LBCs, etc. These tests require the setup (namelist, streams, etc) files located in the directories

MPAS-Model/testing_and_setup/ufs-community/cases/ufscommunity.convection_permitting.gfs.winter.stoch
MPAS-Model/testing_and_setup/ufs-community/cases/ufscommunity.hrrrv5.gfs.winter.stoch

Mandatory Questions

  • Does this PR include any additions or changes to external inputs (e.g., microphysics lookup tables, static data for gravity-wave drag -- things like that)?
    • No. External data files are not added, but the new CI tests do require the namelist, stream, etc files at MPAS-Model/testing_and_setup/ufs-community/cases/ufscommunity.[convection_permitting|hrrrv5].gfs.winter.stoch, which are under version control.
  • Does this PR require updating one or more baselines for the CI tests? If so, what?
    • No.

Priority Reviewers

gsketefian and others added 30 commits September 12, 2025 12:36
…he stochastic_physics submodule itself will be added in a separate commit.
…'t updated during latest merge of gsl/develop.
…he gsl/MPAS_stoch_physics in the stochastic_physics repo.
… smoke) from "output" to "output_smoke" (and file from "history..." to "history_smoke...". This is necessary because the stream name "output" is already taken by the main output stream, and having the same name for two different output streams apparently causes SMIOL I/O errors during the forecast (and incorrect/corrupted history*.nc files).
…) no stale stream_list.atmosphere files exist in the two default_inputs directories (one under core_atmosphere and the other immediately under the MPAS-Model top-level directory), and (2) for the atmosphere core, all files (stream-related as well as namelist) in the top-level directory are backed up before new such files are copied from the default_inputs directory back up a level into the top-level directory. Previously, existing (and thus possibly outdated) stream_list.atmosphere.* files in the top-level directory were not being replaced by newer ones in default_inputs, and that was causing unexpected (and wrong) behavior. Probably a similar fix is needed for the init_atmsophere core and maybe even other ones.
…ll as its output file) so it doesn't conflict with the default output stream.
…tochastic_physics's master branh will gradually happen.
…tochastic_physics code know that the dycore being used is MPAS.
…ill instead be defined in the Makefile for stochastic_physics only (in a separate commit into the stochastic_physics repo) since it is only needed by the stochastic_physics submodule.
…e that is now added in the Makefile in stochastic_physics).
gsketefian and others added 13 commits May 24, 2026 10:50
…lity with the use of mpi_f08 in stochastic_physics (where we don't want to have an option like MPAS_USE_MPI_F08 to choose between "use mpi_f08" and "use mpi" because in the UFS, which the stochastic_physics repo is a part of, Fortran 2008 is a required standard).
…h_physics_try_merge_stoch_master

gsketefian accidentally duplicated some changes in dustinswales's PR#4 before merging that PR.  This resolves those conflicts so the PR can be merged.
Move tendency names to stochastic physics init
…ysics_try_merge_stoch_master' into gsl/MPAS_stoch_physics_try_merge_stoch_master
…chastic_physics submodule (from mpas_stochastic_physics.F to mpas_stochastic_physics.F90) to reflect change in directory tree.
@gsketefian gsketefian marked this pull request as ready for review May 26, 2026 17:18
Change the fetch URL from dustinswales/stochastic_physics to
dtcenter/stochastic_physics, which is the authoritative upstream
that contains the required commit (063b1897).
@clark-evans clark-evans moved this from DRAFT to Needs review in MPAS-A PRs to process May 27, 2026
COMMAND mpas_streams_gen ${CMAKE_CURRENT_BINARY_DIR}/Registry_processed.xml streams.${ARG_CORE} stream_list.${ARG_CORE}. listed
COMMENT "CORE ${ARG_CORE}: Generate Streams"
DEPENDS mpas_streams_gen Registry_processed.xml)
DEPENDS mpas_streams_gen ${CMAKE_CURRENT_BINARY_DIR}/Registry_processed.xml)
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.

why we need to add ${CMAKE_CURRENT_BINARY_DIR}?

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.

@gsketefian or @NingWang325, is this one you can answer?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'm working on remembering the details of why I needed this.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR integrates the NOAA-PSL stochastic_physics package (SPPT scheme only) into MPAS-Model as a git submodule, plumbing the perturbation pattern generation and application into the atmosphere core's init/timestep/RK3 paths and the registry. It also adds a dedicated CI workflow that runs convection_permitting and hrrrv5 winter GFS cases with SPPT enabled, plus build-system support (MKL/LAPACK, NetCDF-Fortran) on several HPC modulefiles and CI images.

Changes:

  • Add stochastic_physics as a submodule with corresponding Make/CMake/Registry wiring and SPPT call sites in atm_core_init, atm_do_timestep, and atm_srk3.
  • New CI workflow run_mpas_stoch.yml and new test case directories ufscommunity.{convection_permitting,hrrrv5}.gfs.winter.stoch (namelists, streams, stream_lists).
  • Build environment updates: LAPACK/BLAS/MKL on modulefiles and CI, NetCDF-Fortran setup, MPAS_USE_MPI_F08 always defined, registry-processed-file paths made absolute in CMake.

Reviewed changes

Copilot reviewed 33 out of 33 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
testing_and_setup/ufs-community/cases/ufscommunity.hrrrv5.gfs.winter.stoch/* New SPPT test case files (namelist, streams, stream lists) for the hrrrv5 suite.
testing_and_setup/ufs-community/cases/ufscommunity.convection_permitting.gfs.winter.stoch/* New SPPT test case files for the convection_permitting suite.
src/Makefile Adds $(LAPACK_LIBS) to the link line; introduces an esmf_time_f91 typo in the -I path.
src/core_atmosphere/Registry.xml Adds Gaussian-grid dims (hard-coded 504/248) and new stoch_pattern_* variables; includes stochastic_physics registry.
src/core_atmosphere/mpas_atm_core.F Wires stochastic_physics_pattern_init and _adv into init and timestep paths.
src/core_atmosphere/Makefile Builds and links the stochastic_physics library; reworks post_build to back up existing default_inputs.
src/core_atmosphere/dynamics/mpas_atm_time_integration.F Calls stochastic_physics_pattern_apply for 'phys' and 'prog' tendencies in atm_srk3.
src/core_atmosphere/dynamics/Makefile Adds -I../stochastic_physics include path.
src/core_atmosphere/CMakeLists.txt Defines a stochastic_physics CMake target with BLAS/NetCDF deps and links it into core_atmosphere.
modulefiles/mpas/{derecho,gaeac6,hera,orion,ursa}.intel.lua Loads Intel MKL and exports NetCDF C/Fortran root env vars.
Makefile Adds LAPACK_LIBS to relevant build targets; adds new intel-mpi-gaeac6 build target.
CMakeLists.txt Always defines MPAS_USE_MPI_F08; adds explicit dependency to serialize init_atmosphere on core_atmosphere builds.
cmake/Functions/MPAS_Functions.cmake Uses absolute ${CMAKE_CURRENT_BINARY_DIR} paths for Registry_processed.xml dependencies.
.gitmodules Adds src/core_atmosphere/stochastic_physics submodule pointing at dtcenter fork.
.github/workflows/run_mpas.yml, run_mpas_hrrr.yml, build_mpas.yml, build_mpas_intel.yml Add LAPACK/BLAS, NetCDF setup, and MKL support; minor commented-out trigger lines.
.github/workflows/run_mpas_stoch.yml New CI workflow that runs baseline vs feature SPPT-enabled MPAS and compares outputs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Makefile

mpas: $(AUTOCLEAN_DEPS) externals frame ops dycore drver
$(LINKER) $(LDFLAGS) -o $(EXE_NAME) driver/*.o -L. -ldycore -lops -lframework $(LIBS) -I./external/esmf_time_f90 -L./external/esmf_time_f90 -lesmf_time
$(LINKER) $(LDFLAGS) -o $(EXE_NAME) driver/*.o -L. -ldycore -lops -lframework $(LIBS) -I./external/esmf_time_f91 -L./external/esmf_time_f90 -lesmf_time $(LAPACK_LIBS)
Copy link
Copy Markdown
Author

@gsketefian gsketefian Jun 1, 2026

Choose a reason for hiding this comment

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

I noticed this as well but thought it might be intentional by @NingWang325. Kind of surprising that it works! @NingWang325 can you say if it's intentional or a typo?

Comment thread .github/workflows/run_mpas_stoch.yml Outdated
Comment on lines +315 to +316
# Links to tables in the testing_and_setup directory of MPAS-Model, e.g. Thompson MP tables.
ln -sf ${srcdir}/{ufscmnty_data}/tables/thompson/MP_THOMPSON_* .
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed.

Comment on lines +41 to +44
<dim name="lon_for" definition="504"
description="The number of longitude points for the Gaussian grid"/>
<dim name="lat_leg" definition="248"
description="The number of latitude points for the Gaussian grid"/>
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.

Without full comprehension of what these variables are used for, I think Copilot has some valid points here.

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.

My browser quit on me so I don't know if this will be a duplicate comments or not... Without fully understanding what these variables are for, it seems Copilot has some good points here.

Comment thread .github/workflows/run_mpas_stoch.yml Outdated
Comment on lines +3 to +4
on: [push, pull_request, workflow_dispatch]
#on: [workflow_dispatch]
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sure, that is fine. I used these to shut off testing every time I push a commit that affects only a single test (e.g. the new one, run_mpas_stoch.yml). I'll remove them manually.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed.

Comment on lines +305 to +309
!
! init stochastic pattern generation
if (dosppt(domain)) then
call stochastic_physics_pattern_init (domain, ierr)
endif
@clark-evans
Copy link
Copy Markdown
Collaborator

@gsketefian @NingWang325 @JeffBeck-NOAA I turned Copilot loose on a review of this PR. It generated five comments, the first few of which (at minimum) look like things that need to be addressed before merge.

&soundings
config_sounding_interval = 'none'
/
&nam_stochy
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.

just curious, why "nam_stochy"?

Copy link
Copy Markdown
Author

@gsketefian gsketefian Jun 1, 2026

Choose a reason for hiding this comment

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

@joeolson42 I think this is what the stochastic physics namelist in the FV3 is called, so we just kept it. My guess as to its meaning: "nam" = namelist, "stochy" = "stochastic". Not sure about the "y" in "stochy". I can't find any explanation in the inline comments in the stochastic_physics code. It might have French origins, since I think there were some French developers working on the code at some point. I'm curious too, so I'll ask @pjpegion in the companion PR #97 into the NOAA-PSL/stochastic_physics repo.

Here's the link to my question.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@joeolson42 Sorry, turns out @pjpegion doesn't know the answer regarding the "y" in "stochy".

I’d personally prefer a more straightforward name like stoch_phys or stoch_physics for the namelist, though it isn't a strong preference.

Comment thread Makefile
"LAPACK_LIBS = -lmkl_intel_lp64 -lmkl_core -lmkl_sequential" \
"CPPFLAGS = $(MODEL_FORMULATION) -D_MPI" )

intel-mpi-gaeac6: # BUILDTARGET for gaea C6
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 don't think we need a new section for gaeac6.
ifort_icx works at gaeac6, as used in rrfs-workflow:
https://github.com/NOAA-EMC/rrfs-workflow/blob/c5e831bdf9a59459beb7dfe07127afbf0c8fd7be/sorc/build.mpas#L28

adding a new section for gaeac6 is some kind of confusing.

If really needed, suggest changing intel-mpi-gaeac6 to be more verbose to fit a specific compiler environment

!
! init stochastic pattern generation
if (dosppt(domain)) then
call stochastic_physics_pattern_init (domain, ierr)
Copy link
Copy Markdown
Collaborator

@dustinswales dustinswales Jun 1, 2026

Choose a reason for hiding this comment

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

@gsketefian Should this die and report error if initialization fails?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@dustinswales I would think it should at least write out a warning in the log file(s) if not die altogether, but I think @NingWang325 is best suited to answer this one.

Comment thread CMakeLists.txt
if("atmosphere" IN_LIST MPAS_CORES AND "init_atmosphere" IN_LIST MPAS_CORES)
add_dependencies(mpas_init_atmosphere core_atmosphere)
endif()

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.

Lines 128-134: Is this a new requirement due to the stochastic physics?
I don't think we need this dependency before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Needs review

Development

Successfully merging this pull request may close these issues.

8 participants