-
Notifications
You must be signed in to change notification settings - Fork 249
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
base: develop
Are you sure you want to change the base?
Feature/mpas dycore #2451
Conversation
…ture for CCPP. More CMake modifications
…r-model into feature/mpas-dycore-reorg
e1538f9
to
b411c6c
Compare
@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. |
@BrianCurtis-NOAA Sorry, I was doing some last minute cleanup. All is back now. |
There was a problem hiding this 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.
#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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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.
There are no new test files at the moment. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 | |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)$") |
There was a problem hiding this comment.
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
Commit Queue Requirements:
Description:
This pull request contains changes to generalize atmospheric component of the UFS Weather Model:
Refactor the CMakeLists to build an atmospheric component with either an FV3
or MPAS dynamical core.
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.
Modify CCPP physics CmakeLists.txt to build independent MPAS/FV3 couplings to the physics.
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:
Priority:
Git Tracking
Sub component Pull Requests:
UFSWM Blocking Dependencies:
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:
Library Changes/Upgrades:
Testing Log: