cam6_3_020: MPAS bug fixes, performance enhancements, allow initial files writes, refactor/cleanup #303

jtruesdal · 2020-12-18T22:38:55Z

PR mods:
. Update CIME external to include NUOPC compliance for MPAS
. Update MPAS external: NUOPC compliance, timer output, new sponge layer diffusion
. Remove MPAS Streams generation (unused)
. dyn_in and dyn_out refactored to point to the same structure.
. Fixes to allow NAG compilation
. Fix to allow running on 1 pe.
. Refactor for weak scaling.
. bug fixed in MPAS sub stepping.
. fixed a small discrepancy in the diagnosed pressure of one of the simple model cases.
. Update default MPASA grids, initial files, and boundary data for newly supported resolutions
. New defaults for MPAS dycore timestep. Changed the 3rd-order scaler transport upwind coefficient
from 600s to 900s.
. Add functionality to output initial condition files.
. Add new sponge layer diffusion options for model stability 1) Rayleigh 2) diffusion similar to SE

Fixes #385

Cam development

sync CAM-1 cam_development

… upwind coef to Hundsdorfer .25 -> 1.0

…hy was screwing up the kessler test.

bld/namelist_files/namelist_defaults_cam.xml

src/dynamics/mpas/dyn_comp.F90

This commit enable the MPAS dycore to build successfully with the physics, dynamics introduced in e3630ff. This commit updates the CAM configure script to use the infrastructure changes when using the MPAS dycore, creates a stub function of get_dyn_grid_info in dyn_grid, and comments out the logic of both d_p_coupling and p_d_coupling. Because get_dyn_grid_info, d_p_coupling, and p_d_coupling do not contain any logic, any CAM-MPAS simulations ran on this commit will not run correctly.

This commit fills out the physics_column_t array, where one indice of the array relates to one MPAS cell. This commit also removes the allocations of global arrays in dyn_grid_init, which were used in the previous physics, dynamics implementation. Because d_p_coupling and p_d_coupling are still commented out, CAM-MPAS simulations will not run succsfully.

This commit updates the d_p_coupling function of the MPAS dycore to use the new weak scaling changes implemented in a previous PR. As of this commit, p_d_coupling has not been updated and is currently commented out, so any CAM-MPAS simulation will not be correct.

This commit updates the p_d_coupling function of the MPAS dycore to use the new changes that resolve the weak scaling problems. Now that p_d_coupling is complete, it is now possible to run CAM-MPAS simulations successfully.

… from MPAS height coordinate.

cacraigucar

@jtruesdal Are these all the expected changes from the development side of things for this PR? If so, I believe this is ready to be sent out for others to review.

src/dynamics/mpas/dp_coupling.F90

…ponge controlled by nu_top

Cam development

…s, 32 lev default for simple models, add r8 to naked reals

jtruesdal · 2021-04-23T20:07:08Z

@cacraigucar This PR is ready for review again. I've updated the cime external to a newer version to fix a bug we had with MPAS restart writes. The MPAS dycore external has been updated to allow it to use the CESM version of ESMF. The MPAS update was requested to allow NUOPC compatibility. @gold2718 We still have the outstanding issue of having an MPAS hash from a branch that could be removed in the future. I can open an issue on that if we don't have a fix right now but I don't think it should hold up the PR. It's not an immediate problem and we have assurances from the MPAS team that they won't delete anything until this issue is resolved.

cacraigucar · 2021-04-23T21:38:35Z

@jtruesdal - The problem with the hash and its possible removal occurs if a user checks out the CAM tag and uses it as a base for their branch or long-term work. Even if they are not using MPAS, a future run of manage_externals/checkout_externals could break their entire branch if the hash were removed. The likelihood of this happening is small, but it is not zero. As a result, I believe we (at least I) would be much more comfortable if the hash issue were resolved before this PR is put onto the CAM trunk. If its not fixed now, it will continue to be a problem for all CAM tags until it is fixed. If the CAM tag without the "forever hash" gets into a CESM tag, then the odds of it creating a problem goes up.

…lopment, remove unused code for weak scaling, Makefile - toggle use of External ESMF library CPP define when using NUOPC driver

jtruesdal · 2021-05-18T18:34:47Z

@cacraigucar This PR is ready for review again. I pushed the change to make MPAS an optional component.

cacraigucar · 2021-05-18T20:05:11Z

Externals.cfg


 [cime]
-tag = cime5.8.37
+tag = cime5.8.42


If regression tests for non-MPAS tests have answer changes, you may need to update this cime tag in a separate CAM tag all by itself.

I will test that now. Would the following failure against baseline be acceptable?

FAIL SMS_D_Ln9.ne0CONUSne30x8_ne0CONUSne30x8_mt12.FCHIST.cheyenne_intel.cam-outfrq9s_refined_camchem BASELINE /glade/p/cesm/amwg/cesm_baselines/cam6_3_017: FIELDLIST field lists differ (otherwise bit-for-bit)

Yes, fieldlist differences are fine. Answer changes due to cime would not be good mixed in with this tag, if there are any.

cacraigucar · 2021-05-18T20:07:54Z

doc/ChangeLog

+
+List all files added and what they do:
+
+List all existing files that have been modified, and describe the changes:


Double check that all the modified files are accurately reflected here. (Externals.cfg is not listed though it now has change)

I added documentation for Externals.cfg, diagnostics.F90 and listed subroutines that were removed when adopting weak scaling fixes. I'll have one more update when I run the official regression tests and will push this file before tagging.

nusbaume

Looks good! I do have one recommendation about wrapping an endrun call in an if-statement, but the rest are all just minor things like typos or error message text additions.

doc/ChangeLog

bld/namelist_files/namelist_defaults_cam.xml

src/dynamics/mpas/dp_coupling.F90

src/dynamics/mpas/driver/cam_mpas_subdriver.F90

src/dynamics/mpas/dyn_comp.F90

src/dynamics/mpas/stepon.F90

src/dynamics/mpas/dyn_grid.F90

gold2718 · 2021-05-31T03:55:52Z

I'm not sure where this should be documented, however, there is a bug in manage_externals. If the hash for MPAS is updated, you have to rerun checkout_externals with the --optional flag or the MPAS external will not be updated.
See ESMCI/manage_externals#155

gold2718

A few change requests

gold2718 · 2021-05-29T05:18:46Z

doc/ChangeLog

+One-line Summary:
+Github PR URL:


Please fill these in

gold2718 · 2021-05-30T03:28:27Z

doc/ChangeLog

+. Remove unused code in build-namelist for generating MPAS streams files.
+
+. Fix to allow MPAS to run with 1 task.
+
+. Fix to allow MPAS to run with NAG/debug.
+
+. Replace bad grid file for running 480-km res with no topo.
+
+. Fix for bug when MPAS dycore is substepping during physics timestep.
+
+. Update calculation of hydrostatic pressure from MPAS state.
+
+. Add ability to write initial file when using MPAS dycore.
+
+. Correct interface error when calculation CAM pressures from MPAS height.
+
+. update the MPAS-A dycore tag and CAM files associated with namelists in order
+	to support building with the ESMF/NUOPC driver and to introduce two new
+        absorbing layer options
+
+	1. A diffusion scheme similar to CAM-SE. Using a mpas_cam_coef =  1.0 will give
+	   a damping coefficient of 0.2216E7, 0.6482E6, 0.1927E6 in the top-most three layers on
+	   the dynamics variables u, w, and theta. The top 3 damping coefficients scale
+	   linearly with mpas_cam_coef.
+
+	   This option is controlled by the namelist variable mpas_cam_coef.
+
+	2. A Rayleigh damping on the horizontal momentum with a user-specified damping
+	   timescale and a linear ramp over a user-specified number of levels from
+	   the model top.
+
+	   This option is controlled by the namelist variables mpas_rayleigh_damp_u,
+	   mpas_rayleigh_damp_u_timescale_days, and mpas_number_rayleigh_damp_u_levels.
+
+. Add calls to MPAS framework routines to write out detailed timers
+	for individual components of the MPAS-A dycore at the end of a simulation. This
+	can be helpful in comparing the runtime performance of CAM-MPAS with stand-
+	alone MPAS-Atmosphere, which already had detailed timers.
+
+. Port weak scaling mods to MPAS


Please point to issues that describe the need for each of these changes (as noted in the instructions). I do not currently see any issues addressed by this PR, please link to them or open them as needed.

New issue: #385
MPAS is currently under development so in this case, I've opened a general issue for all of these items. I will be better about opening issues in the future as we anticipate more updates as the dycore port matures. While MPAS is under heavy development and in the interest of making efficient use of the reviewers' time, it is very likely that each of the next PR's will include several changes. I will make sure to update the issue, in the case of last-minute additions to a PR that weren't documented in the original.

While this looks like efficiency on the surface, it costs us a lot of time on the back end. The issues are our record and database for discussions of what needed fixing and why. Please trim #385 to match what is fixed in this PR and add a line to that effect in the PR title (see item 10 of the CAM SE PR procedures or the GitHub issue-closing syntax.

doc/ChangeLog

gold2718 · 2021-05-30T23:33:01Z

doc/ChangeLog

+      to translate between physics and dynamics columns.
+
+src/physics/cam/geopotential.F90
+. fix calculation of CAM pressure from MPAS height coordinate.


The code change looks more like it is for FV3, I do not see any change specific to MPAS.

This particular if statement is applicable to all the dycores. MPAS is listed as a comment in the else part of the statement. This is one of Peter's changes, I think he just wanted to make explicit the differences between the dycores in the setting of the hydrostatic elements. If you would like more comments or something please 'at' PeterHjortLauritzen and I'll let him put the wording together.

My point is that while the additional comments mention MPAS, there is a code change that affects FV3. @PeterHjortLauritzen, is the FV3 change appropriate? If so, there should be an issue discussing the change as I expect it would change answers for FV3 runs.

The changelog comment for this mod is incorrect and I will replace that. The change is a no-op for FV3 and the rest of the dycores. Peter just removed a logical that obscured which dycores were involved and replaced it with the complete expression and comment explicitly listing them.

Ah, thanks for the explanation. I see now that the new version still has the fvdyn variable but is no longer using it. Please remove that variable from geopotential.F90.

Removed variable and associated code.

doc/ChangeLog

src/dynamics/mpas/stepon.F90

gold2718 · 2021-05-31T04:24:16Z

src/dynamics/mpas/stepon.F90

-end subroutine shift_time_levels
+   call cam_mpas_write_restart(initial_stream, endrun)
+
+   call pio_closefile(fh)


Please use cam_pio_closefile (which will do more work in the future):

Suggested change

call pio_closefile(fh)

call cam_pio_closefile(fh)

Thanks for the suggestion, I changed it.

src/dynamics/mpas/dyn_grid.F90

gold2718 · 2021-05-31T05:35:12Z

src/dynamics/mpas/dp_coupling.F90

-   real(r8), allocatable, dimension(:) :: bbuffer, cbuffer ! transpose buffers
+   real(r8), allocatable:: pmid(:,:)   !mid-level pressure consisten with MPAS discrete state

+   real(r8), allocatable, dimension(:) :: bbuffer, cbuffer ! transpose buffers


These are not used anymore, delete (same for p_d_coupling).

Good catch. Removed from both subroutines.

src/dynamics/mpas/dp_coupling.F90

gold2718

Looks good now (pending input from @PeterHjortLauritzen).

gold2718 · 2021-06-03T15:29:51Z

doc/ChangeLog

+      to translate between physics and dynamics columns.
+
+src/physics/cam/geopotential.F90
+. fix calculation of CAM pressure from MPAS height coordinate.


My point is that while the additional comments mention MPAS, there is a code change that affects FV3. @PeterHjortLauritzen, is the FV3 change appropriate? If so, there should be an issue discussing the change as I expect it would change answers for FV3 runs.

src/dynamics/mpas/dp_coupling.F90

src/dynamics/mpas/dyn_comp.F90

nusbaume

Looks like a couple of my requested edits were missed (?). Once they are fixed (or you have a reason to not make those changes) then this PR should be good to go on my end.

src/dynamics/mpas/dyn_comp.F90

src/dynamics/mpas/dyn_grid.F90

…umbers added to similar error messages to aid debug, ChangeLog updates, remove unused variable and code in geopotential.F90, Make MPAS external non-optional again

nusbaume

Looks great now, thanks!

Merge pull request ESCOMP#303 from jtruesdal/mpas_fixes - MPAS updates and bugfixes

jtruesdal and others added 6 commits May 7, 2020 10:35

Merge pull request #1 from ESCOMP/cam_development

ce969e5

Cam development

Merge pull request #5 from ESCOMP/cam_development

f95085f

Cam development

Merge pull request #7 from ESCOMP/cam_development

17430cd

sync CAM-1 cam_development

create copy of Brians mpas_fixes branch on my fork for PR

55cc749

updates to mpas defaults, longer timestep 600->900s, scalar transport…

bf44227

… upwind coef to Hundsdorfer .25 -> 1.0

replace mpasa120 ncdata grid file with a notopo default, the topograp…

8725316

…hy was screwing up the kessler test.

cacraigucar approved these changes Dec 19, 2020

View reviewed changes

bld/namelist_files/namelist_defaults_cam.xml Outdated Show resolved Hide resolved

src/dynamics/mpas/dyn_comp.F90 Outdated Show resolved Hide resolved

src/dynamics/mpas/dyn_comp.F90 Show resolved Hide resolved

cacraigucar assigned jtruesdal Dec 19, 2020

cacraigucar added this to the CESM2.3 milestone Dec 19, 2020

MiCurry and others added 5 commits January 7, 2021 10:35

Update MPAS p_d_coupling with new weak scaling changes

4273a26

This commit updates the p_d_coupling function of the MPAS dycore to use the new changes that resolve the weak scaling problems. Now that p_d_coupling is complete, it is now possible to run CAM-MPAS simulations successfully.

Peters interface fix to correct a error when calculating CAM pressure…

e94dd8d

… from MPAS height coordinate.

cacraigucar reviewed Jan 11, 2021

View reviewed changes

src/dynamics/mpas/dp_coupling.F90 Outdated Show resolved Hide resolved

jtruesdal and others added 9 commits January 13, 2021 15:54

updates to add sponge diffusion to top 3 layers - similar to CAM-SE s…

19cd4c4

…ponge controlled by nu_top

Merge pull request #9 from jtruesdal/cam_development

6f82fbf

Cam development

Merge pull request #10 from ESCOMP/cam_development

face455

Cam development

bring up to date with ESCOMP cam_development head

dff8425

MPAS timer updates

6249eb8

Miles port of weak scaling mods to MPAS

8d9066e

mpas 120/480 defaults for AMIP runs

9368dbc

bug fix for mpas amip commit

3a83b5f

update externals for 120/480 defaults

12b2b24

cacraigucar marked this pull request as draft February 22, 2021 22:28

cacraigucar changed the title ~~MPAS bug fixes, performance enhancements, allow initial files writes, refactor/cleanup~~ cam6_3_011: MPAS bug fixes, performance enhancements, allow initial files writes, refactor/cleanup Feb 26, 2021

cacraigucar changed the title ~~cam6_3_011: MPAS bug fixes, performance enhancements, allow initial files writes, refactor/cleanup~~ MPAS bug fixes, performance enhancements, allow initial files writes, refactor/cleanup Mar 1, 2021

jtruesdal and others added 3 commits March 8, 2021 12:04

add pressure level fields to default history output, rayleigh default…

38498c2

…s, 32 lev default for simple models, add r8 to naked reals

merge in changes for weak scaling

8a35da0

sync MPAS with cam development

97002e7

jtruesdal requested a review from cacraigucar April 27, 2021 15:05

MPAS moved to optional component until merged with mainline MPAS deve…

24121b5

…lopment, remove unused code for weak scaling, Makefile - toggle use of External ESMF library CPP define when using NUOPC driver

fvitt approved these changes May 18, 2021

View reviewed changes

cacraigucar approved these changes May 18, 2021

View reviewed changes

peverwhee approved these changes May 19, 2021

View reviewed changes

nusbaume approved these changes May 20, 2021

View reviewed changes

cacraigucar mentioned this pull request May 24, 2021

cam6_3_021: Update CAM codes for Casper and the execution of qsat* subroutines on GPU #366

Merged

gold2718 requested changes May 31, 2021

View reviewed changes

jtruesdal added 2 commits June 2, 2021 22:43

PR updates from goldy and jesse

cde52b2

change mixing to diffusion

fa79940

jtruesdal requested a review from gold2718 June 3, 2021 05:34

gold2718 approved these changes Jun 3, 2021

View reviewed changes

gold2718 requested a review from nusbaume June 3, 2021 15:38

cacraigucar changed the title ~~MPAS bug fixes, performance enhancements, allow initial files writes, refactor/cleanup~~ cam6_3_020: MPAS bug fixes, performance enhancements, allow initial files writes, refactor/cleanup Jun 3, 2021

nusbaume requested changes Jun 3, 2021

View reviewed changes

src/dynamics/mpas/dyn_comp.F90 Outdated Show resolved Hide resolved

src/dynamics/mpas/dyn_grid.F90 Outdated Show resolved Hide resolved

jtruesdal linked an issue Jun 3, 2021 that may be closed by this pull request

MPAS updates to add new functionality, bug fixes, refactor cleanup #385

Closed

better error coverage for user errors specifying decomposions, line n…

02e503f

…umbers added to similar error messages to aid debug, ChangeLog updates, remove unused variable and code in geopotential.F90, Make MPAS external non-optional again

jtruesdal requested a review from nusbaume June 3, 2021 19:39

nusbaume approved these changes Jun 3, 2021

View reviewed changes

jtruesdal added 4 commits June 3, 2021 14:42

Bringing mpas_fixes up to cam_development for PR

0e87e44

update ChangeLog and remove added space from mod

3852251

update date

610c8cf

update Changelog for merge/commit

462c073

jtruesdal merged commit fbc2289 into ESCOMP:cam_development Jun 5, 2021

gold2718 mentioned this pull request Apr 29, 2022

Update MPAS-A physics, dynamics coupling with weak scaling changes #301

Closed

cacraigucar added a commit to cacraigucar/CAM that referenced this pull request Aug 4, 2022

Merge tag 'cam6_3_020' into cam_ml_tau

cf44177

Merge pull request ESCOMP#303 from jtruesdal/mpas_fixes - MPAS updates and bugfixes


		List all files added and what they do:

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

		One-line Summary:
		Github PR URL:

cam6_3_020: MPAS bug fixes, performance enhancements, allow initial files writes, refactor/cleanup #303

cam6_3_020: MPAS bug fixes, performance enhancements, allow initial files writes, refactor/cleanup #303

Uh oh!

Conversation

jtruesdal commented Dec 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cacraigucar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jtruesdal commented Apr 23, 2021

Uh oh!

cacraigucar commented Apr 23, 2021

Uh oh!

jtruesdal commented May 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nusbaume left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gold2718 commented May 31, 2021

Uh oh!

gold2718 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jtruesdal commented Dec 18, 2020 •

edited

Loading