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

Feature/mpas dycore #2451

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

dustinswales
Copy link
Collaborator

@dustinswales dustinswales commented Oct 7, 2024

Commit Queue Requirements:

  • Fill out all sections of this template.
  • All sub component pull requests have been reviewed by their code managers.
  • Run the full Intel+GNU RT suite (compared to current baselines) on either Hera/Derecho/Hercules
  • Commit 'test_changes.list' from previous step

Description:

This pull request contains changes to generalize atmospheric component of the UFS Weather Model:

  1. The reorganization of the repository is to achieve the following goals:
  • Move dynamical core specific code into subdirectories.
  • Move common code (e.g. physics) into shareable, cross-dycore, higher-level directories. This is outlined in Figure 1 of the "Tiger Team" report.
  1. Refactor the CMakeLists to build an atmospheric component with either an FV3
    or MPAS dynamical core.

  2. Add CMakeLists.txt to build MPAS core_atmosphere component. The CMake recipes from the "MPAS atmospheric core" were adapted to work in the UFS build system.

  3. Modify CCPP physics CmakeLists.txt to build independent MPAS/FV3 couplings to the physics.

  4. Create "stub" mpas infrastructure. This includes stubs for the MPAS atmospheric component driver and overlaying NUOPC cap.

With these changes, the MPAS dynamical core builds within UFS.
Next steps, build atmospheric driver for MPAS, which includes creating new typedefs for coupling between the CCPP and MPAS.

Commit Message:

* UFSWM - Modify build scripts to select which dynamical core to build for the UFS.
  * FV3 - Reorganize into dynamical core specific subdirectories. Modify build systems for multiple dynamical cores. Bring in 

Priority:

  • Normal.

Git Tracking

  • None

Sub component Pull Requests:

UFSWM Blocking Dependencies:

  • None

Changes

No changes to baselines. All RTs passed on Hera using Intel.
New test to tests/rt.conf to build UFS with MPAS dynamical core.

Input data Changes:

  • None.

Library Changes/Upgrades:

  • No Updates

Testing Log:

  • RDHPCS
    • Hera
    • Orion
    • Hercules
    • Jet
    • Gaea
    • Derecho
  • WCOSS2
    • Dogwood/Cactus
    • Acorn
  • CI
  • opnReqTest (complete task if unnecessary)

@BrianCurtis-NOAA
Copy link
Collaborator

@dustinswales It looks like you had a bunch of changes but a force push removed almost all of them. If you intend to work on this PR for a bit, please move it to a draft form until it's ready to be reviewed by the code managers.

@dustinswales
Copy link
Collaborator Author

@BrianCurtis-NOAA Sorry, I was doing some last minute cleanup. All is back now.

Copy link
Collaborator

@BrianCurtis-NOAA BrianCurtis-NOAA left a comment

Choose a reason for hiding this comment

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

I think that we will need to adjust rt.sh and the other .sh scripts in the tests directory to use MPAS.

I don't see any new tests files either, so you'll want to bring those in.

Comment on lines +373 to +380
#ifdef FRONT_MPAS
! if (trim(model) == "mpas") then
! call NUOPC_DriverAddComp(driver, trim(prefix), MPAS_SS, &
! info=info, petList=petList, comp=comp, rc=rc)
! if (ChkErr(rc,__LINE__,u_FILE_u)) return
! found_comp = .true.
! end if
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason this is commented out currently?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's just a placeholder for the timebeing.
The NUOPC pieces for MPAS will be added later, when the underlying atmospheric component work on MPAS is further along.

@dustinswales
Copy link
Collaborator Author

dustinswales commented Oct 7, 2024

I think that we will need to adjust rt.sh and the other .sh scripts in the tests directory to use MPAS.

I modified default_vars.sh and added a new "MPAS build only" test to rt.conf. If there are other places that need modification, please let me know.

I don't see any new tests files either, so you'll want to bring those in.

There are no new test files at the moment.

FV3
Copy link
Collaborator

Choose a reason for hiding this comment

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

This submodule pointer update seems to be wrong. If I click on the changes, I get taken to a update of 22 files all within the NCAR fv3atm fork - should this PR not update the code in the noaa-emc fv3atm? Maybe I am missing something (it's late).

@@ -499,6 +499,7 @@ export POSTXCONFIG=postxconfig-NT-gfs.txt
export POSTXCONFIG_FH00=postxconfig-NT-gfs_FH00.txt

export FV3=true
export MPAS=true
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest export MPAS=false here in export_fv3 so both aren't true. Making a separate export_mpas () is one option. If you want many of the same settings while turning on MPAS-A, it may be more concise to set:

export_fv3
export FV3=false
export MPAS=true

in a regression test until there are defaults to set.

@@ -433,3 +433,6 @@ RUN | datm_cdeps_control_cfsr | + hera hercules
### ATM-FBH test ###
COMPILE | atm_fbh | intel | -DAPP=ATMF -DCCPP_SUITES=FV3_HRRR -D32BIT=ON | - wcoss2 noaacloud acorn | fv3 |
RUN | cpld_regional_atm_fbh | - wcoss2 noaacloud acorn | baseline |

### UFS with MPAS dynamical core ###
COMPILE | atm_mpas_dyn32 | intel | -DAPP=ATMMPAS -DCCPP_SUITES=FV3_GFS_v16,FV3_GFS_v17_p8_ugwpv1 -D32BIT=ON | | mpas |
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not see RUN step that executes the ufs-model executable built with this COMPILE step. How do we even know the mpas model is compiled correctly. Do you plan to add at least one regression test for ufs with mpas core?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, I see this question has been already asked in one of the previous comments, and the answer was no. I'm now confused, but I do not see what we get by making these changes in the ufs-weather-model at this time. We can discuss the corresponding PR in the fv3atm and if it's necessary merge it, but why updating ufs-wm without any tests is necessary

Copy link
Collaborator

Choose a reason for hiding this comment

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

-D32BIT=ON is build flag used to build fv3 dycore using single-precision reals as a default. Is it used in MPAS core? I do not see it used in any mpas CMakeLists.txt. If it's not used, it should be removed from this COMPILE step.

@@ -157,7 +159,7 @@ if(FMS)
elseif (APP MATCHES "^(S2S|S2SA|S2SW|S2SWA|S2SWAL|ATM_DS2S|ATM_DS2S-PCICE|NG-GODAS|HAFS-MOM6|HAFS-MOM6W)$")
add_library(fms ALIAS FMS::fms_r8)
endif()
if(APP MATCHES "^(ATM|ATMAERO|ATMAQ|ATMWM|ATMW|ATML|ATMF|HAFS|HAFS-ALL)$")
if(APP MATCHES "^(ATM|ATMAERO|ATMAQ|ATMWM|ATMW|ATML|ATMF|ATMMPAS|HAFS|HAFS-ALL)$")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove if MPAS does not need FMS

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.

5 participants