Add the option for inactive top cells #825

cbegeman · 2021-03-10T17:04:32Z

Inactive top cells are introduced to add flexibility to the vertical coordinate below ice shelves.

This PR introduces phase 1 in the development documented in #811, which touches only the code needed to run a basic ice shelf test case.

The variable minLevelCell and related minLevelEdge* and minLevelVertex* variables are introduced by analogy with maxLevel* variables. By default, there are no inactive top cells and minLevel* = 1. The main code changes are to k-loop limits and designating surface quantities as those at minLevel*.

xylar · 2021-03-10T19:18:11Z

Testing

COMPASS (3/22)

As a baseline in COMPASS, I'm using the legacy branch + xylar/compass@ca592c7, which adds a test suite (z_level_dev) that includes both all tests in the nightly suite and several tests from land_ice_fluxes.

The COMPASS branch used for testing is the one in MPAS-Dev/compass#55 plus the same commit from above for the z_level_dev test suite. It contains only the required changes for this work.

The results from the z_level_dev test suite are bit-for-bit identical to the baseline for:

Anvil with intel (@xylar)
Ubuntu with gnu (@xylar)

E3SM

The following E3SM tests are bit-for-bit when compared with E3SM/master:

PEM_Ln9_P1024.ne30_oECv3.A_WCYCL1850S.anvil_intel (@xylar)
PET_Ln9_P1024.ne30_oECv3.A_WCYCL1850S.anvil_gnu (@xylar)
SMS.T62_oQU120_ais20.MPAS_LISIO_TEST.anvil_gnu (@xylar)
SMS.T62_oQU120_ais20.MPAS_LISIO_TEST.anvil_intel (@xylar)
SMS_D.T62_oQU120_ais20.MPAS_LISIO_TEST.compy_pgi (@xylar)
SMS.T62_oQU120_ais20.MPAS_LISIO_TEST.compy_intel (@xylar)
PET_Ln9_P1024.ne30_oECv3.A_WCYCL1850S.compy_intel (@xylar)

These tests are failing with diffs (the first is the same as a test above but not in DEBUG mode):

SMS.T62_oQU120_ais20.MPAS_LISIO_TEST.compy_pgi (@xylar)
PEM_Ln9_P1024.ne30_oECv3.A_WCYCL1850S.compy_pgi (@xylar)

The following E3SM tests have passed (but where not compared with E3SM/master):

PET_Ln9.T62_oQU240.GMPAS-IAF.anvil_gnu (@xylar)
PEM_Ln9.T62_oQU240.GMPAS-IAF.anvil_gnu (@xylar)
PET_Ln3.T62_oEC60to30v3wLI.GMPAS-DIB-IAF-ISMF.anvil_intel (@xylar)
PET_Ln9.T62_oQU240.GMPAS-IAF.compy_pgi (@xylar)
PEM_Ln9.T62_oQU240.GMPAS-IAF.compy_pgi (@xylar)
PET_Ln3.T62_oEC60to30v3wLI.GMPAS-DIB-IAF-ISMF.compy_pgi (@xylar)

cbegeman · 2021-03-10T19:23:55Z

The results from the z_level_dev test suite are bit-for-bit identical to the baseline for:

Grizzly and Intel

xylar · 2021-03-10T19:36:30Z

@cbegeman, as I noted above, a corresponding PR into legacy in the compass repo is apparently going to be required. I thought legacy might just work but apparently some of my changes are needed.

xylar · 2021-03-10T20:11:11Z

@cbegeman, could you re-test on Grizzly with the new COMPASS branch I created? I'll re-test on Anvil.

cbegeman · 2021-03-11T00:59:09Z

@cbegeman, could you re-test on Grizzly with the new COMPASS branch I created? I'll re-test on Anvil.

Yes, we get bfb with the new COMPASS branch on Grizzly.

xylar · 2021-03-11T11:07:56Z

@cbegeman, E3SM tests vs. baseline are failing. Here are some details.

When I do a test merge at the following commits, I am indicating which pass and which fail SMS.T62_oQU120_ais20.MPAS_LISIO_TEST.anvil_intel

Commits failing:

f881969 Set vertical velocity top to 0 from k=1:minLevelCell-1
c85f36c Change k-limits on kineticEnergyCell computation

Commits passing:

73826a3 add minLevel* to ocn_vel_pressure_grad_tend
7f6922a MinLevel* changes to ocn_vert_transport_velocity_top
5f4c60e MinLevel* changes to ocn_tracer_vmix_tend_implicit:
da57e3b Do not use dry cells to determine minLevelEdge*
897c039 Do not use dry cells to determine minLevelVertex*
897c039 + cherry-pick of f881969

So the problem is c85f36c. We need to take that commit out.

cbegeman · 2021-03-11T14:54:28Z

@xylar Thanks for tracking this down. c85f36c was the commit I introduced so that QU240-Analysis was bfb on Grizzly. I'll remove it and test again there.

xylar · 2021-03-11T15:10:59Z

If the QU240 analysis is failing without this, I wonder if it's also failing with ocean/develop. It's hard to see how this could be caused by our changes.

cbegeman · 2021-03-11T15:37:51Z

So QU240-Analysis is now passing with MPAS-Dev/compass#55, which suggests that the difference is due to the xylar/compass/add_z_level_sub_ice_shelf_2D commits. I'll retest this branch without commit c85f36c with xylar/compass/add_z_level_sub_ice_shelf_2D.

Tests also pass with xylar/compass/add_z_level_sub_ice_shelf_2D so we're good to go without that commit.

xylar · 2021-03-11T16:47:46Z

If it's still failing, let me know. Something to fix for phase 2.

xylar · 2021-03-12T14:28:40Z

@cbegeman, I haven't had a chance yet but I'll retest everything on Anvil soon, then try again on Compy.

src/core_ocean/mode_forward/mpas_ocn_time_integration_split.F

src/core_ocean/shared/mpas_ocn_diagnostics.F

mattdturner · 2021-03-16T14:38:59Z

src/core_ocean/mode_forward/mpas_ocn_time_integration_split.F

@@ -1759,11 +1759,11 @@ subroutine ocn_time_integrator_split(domain, dt)!{{{
                                .or.lat>- 2.5.and.lat<  2.5 &
                                .or.lat> 35.0.and.lat< 40.0 &
                                .or.lat> 55.0.and.lat< 60.0 ) then
-                              do k = 1, config_reset_debugTracers_top_nLayers
+                              do k = minLevelCell(iCell), minLevelCell(iCell)+config_reset_debugTracers_top_nLayers


The upper limit of this loop should also have the -1 appended (like line 1750)

src/core_ocean/mode_forward/mpas_ocn_time_integration_split.F

mattdturner · 2021-03-16T14:44:06Z

@cbegeman Thanks for the work in this PR. I added a few additional comments to the split explicit driver regarding loop limits.

Aside from those comments, I think this looks good by visual inspection. Once those loop limits are updated, I will approve this PR and rely on testing by you and @xylar

mattdturner

Approved via visual inspection and testing by @cbegeman and @xylar

xylar · 2021-03-22T17:47:10Z

I am still working on 3 (slow) tests on Compy. I will update my "Testing" comment and post the list of completed tests on E3SM-Project/E3SM#4171 once they are done.

mark-petersen · 2021-03-22T18:00:58Z

@dengwirda and @philipwjones please approve. We hope to merge today or tomorrow.

philipwjones · 2021-03-22T19:21:52Z

@mattdturner @cbegeman @xylar Has anyone looked at the performance/timing diffs for these changes in any of the tests with optimization on?

xylar · 2021-03-22T19:26:44Z

@mattdturner @cbegeman @xylar Has anyone looked at the performance/timing diffs for these changes in any of the tests with optimization on?

@philipwjones, I don't believe so. We were not given a clear idea of what we needed to be looking at in which tests. This is a bit part of what we were requesting your help with both in your review here and in the design doc.

mattdturner · 2021-03-22T20:28:11Z

@philipwjones I just ran a quick test on Chrysalis.

Test case: SMS_P2560x1.T62_oRRS18to6v3.GMPAS-IAF.chrysalis_intel

Before these changes:

    OCN Run Time:     698.355 seconds      139.671 seconds/mday         1.69 myears/wday

After these changes are applied:

    OCN Run Time:     702.525 seconds      140.505 seconds/mday         1.68 myears/wday

cbegeman · 2021-03-22T21:02:55Z

@philipwjones Timing differences between the runs appear to be in the noise:

Test case: PEM_Ln9_P1024.ne30_oECv3.A_WCYCL1850S.cori-knl_intel, 1024 proc.

Before these changes:
OCN Run Time: 5.651 seconds 30.138 seconds/mday 7.85 myears/wday
After these changes:
OCN Run Time: 5.618 seconds 29.964 seconds/mday 7.90 myears/wday

Test case: PET_Ln9.T62_oQU240.GMPAS-IAF.cori-knl_intel

Before these changes:
OCN Run Time: 4.954 seconds 6.606 seconds/mday 35.83 myears/wday
After these changes (2 runs):
OCN Run Time: 5.888 seconds 7.851 seconds/mday 30.15 myears/wday
OCN Run Time: 5.046 seconds 6.728 seconds/mday 35.19 myears/wday

Test case: PET_Ln3.T62_oEC60to30v3wLI.GMPAS-DIB-IAF-ISMF.cori-haswell_intel

Before these changes:
OCN Run Time: 11.764 seconds 188.232 seconds/mday 1.26 myears/wday
After these changes:
OCN Run Time: 11.686 seconds 186.984 seconds/mday 1.27 myears/wday

philipwjones · 2021-03-22T21:30:02Z

Thanks @cbegeman and @mattdturner - I also ended up doing a couple of quick tests. PGI on Summit may be slightly faster but in the noise. Intel/grizzly slightly slower at 1-2% level but Intel tends to do a better job of vectorizing so probably more impacted by this change and not surprising.

philipwjones

Approve based on testing by myself and others, prior discussions on design, and visual inspection of a few routines.

xylar · 2021-03-22T21:35:31Z

Thanks for the testing, @philipwjones! That's helpful. I'm also not surprised to see a little slowdown. I was guessing that PGI might show some (especially because it's the only compiler where we're seeing non-bit-for-bit results for the optimized code). I'll see what timings look like when my last run on Compy finishes.

dengwirda · 2021-03-22T23:18:11Z

src/core_ocean/shared/mpas_ocn_vmix.F

         N = maxLevelCell(iCell)

         ! A is lower diagonal term
-         A(1)=0.0_RKIND
-         do k = 2, N
+         A(Nsurf)=0.0_RKIND


Is there a slightly different treatment of zeroing-out unneeded entries in the tridiagonals between tracer_vmix_tend_implicit and vel_vmix_tend_implicit?

It looks like A(1:Nsurf) = 0.0, etc is done in vel_vmix, but only A(Nsurf) = 0.0 in tracer_vmix. I think there may only be a difference in unused values that are out of range, so I don't think this will impact results in any way.

You're right. I didn't catch that because it is unused but we should be consistent. I'm happy to commit this change, but I might include it in phase 2 if including it here would hold up the merge since our tests are pretty much complete. @xylar or @mark-petersen do you have opinions about whether to commit this now or later?

Suggested change

A(Nsurf)=0.0_RKIND

A(1:Nsurf)=0.0_RKIND

I'm personally not worried about this, and think it can go into phase II --- I think the only difference would be that some array entries (that are never used) could be allocated with undefined values rather than being zeroed out.

This shouldn't happen in the minLevelCell=1 case anyway though, as A(1:Nsurf) and A(Nsurf), etc should do the same zeroing.

dengwirda

Approved based on visual inspection + compiling/running locally.

cbegeman · 2021-03-22T23:26:33Z

@dengwirda Thanks for your review!

xylar · 2021-03-23T08:38:06Z

Timing on Compy

`PET_Ln9_P1024.ne30_oECv3.A_WCYCL1850S.compy_intel`

E3SM/master:

OCN Run Time:       1.643 seconds        8.760 seconds/mday        27.02 myears/wday

with this branch:

OCN Run Time:       1.149 seconds        6.126 seconds/mday        38.64 myears/wday

`SMS.T62_oQU120_ais20.MPAS_LISIO_TEST.compy_intel`

E3SM/master:

OCN Run Time:      40.649 seconds        8.130 seconds/mday        29.12 myears/wday

with this branch:

OCN Run Time:      41.177 seconds        8.235 seconds/mday        28.74 myears/wday

`SMS.T62_oQU120_ais20.MPAS_LISIO_TEST.compy_pgi`

E3SM/master:

OCN Run Time:      42.600 seconds        8.520 seconds/mday        27.78 myears/wday

with this branch:

OCN Run Time:      43.195 seconds        8.639 seconds/mday        27.40 myears/wday

…xt (PR #4171) Update mpas-source: Add the option for inactive top cells Brings in a new mpas-source submodule with changes only to the ocean core. Inactive top cells are introduced to add flexibility to the vertical coordinate below ice shelves. For all existing mesh configurations, this simply substitutes arrays with values of 1 rather than a fixed character 1 for the lower bound of vertical loops in the code. See MPAS-Dev/MPAS-Model#825. [non-BFB] only for compy with PGI optimized (BFB in debug mode)

Update mpas-source: Add the option for inactive top cells Brings in a new mpas-source submodule with changes only to the ocean core. Inactive top cells are introduced to add flexibility to the vertical coordinate below ice shelves. For all existing mesh configurations, this simply substitutes arrays with values of 1 rather than a fixed character 1 for the lower bound of vertical loops in the code. See MPAS-Dev/MPAS-Model#825. [non-BFB] only for compy with PGI optimized (BFB in debug mode)

xylar · 2021-03-25T16:18:03Z

Thanks so much for the hard work on this, @cbegeman! Feel free to delete you branch and any others lying around from your testing.

Change three init mode namelist options In conjunction with MPAS-Dev/MPAS-Model#825, The following namelist changes are needed: * config_alter_ICs_for_pbcs --> config_alter_ICs_for_pbs (to support both top and bottom cells) * config_pbc_alteration_type --> config_pc_alteration_type * config_use_rx1_constraint = .true. --> config_init_vertical_grid_type = 'haney-number' (to support a new z-level vertical grid)

Add the option for inactive top cells MPAS-Dev#825

…n/develop Enhance inactive top cells feature #840 Here, we extend the capability for Inactive top cells introduced by #825 to apply to additional model configurations besides those needed to run a basic ice shelf test case. More detail about the scope of this development is documented in #811. The variable minLevelCell and related minLevelEdge* and minLevelVertex* variables are added by analogy with maxLevel* variables. By default, there are no inactive top cells and minLevel* = 1. The main code changes are to k-loop limits and designating surface quantities as those at minLevel*.

…next (PR #4219) Update mpas-source: Enhance inactive top cells feature Brings in a new mpas-source submodule with changes only to the ocean core. It extends the capability for inactive top cells introduced by MPAS-Dev/MPAS-Model#825 to apply to additional model configurations besides those needed to run a basic ice shelf test case. More detail about the scope of this development is documented in MPAS-Dev/MPAS-Model#811. [BFB]

…4219) Update mpas-source: Enhance inactive top cells feature Brings in a new mpas-source submodule with changes only to the ocean core. It extends the capability for inactive top cells introduced by MPAS-Dev/MPAS-Model#825 to apply to additional model configurations besides those needed to run a basic ice shelf test case. More detail about the scope of this development is documented in MPAS-Dev/MPAS-Model#811. [BFB]

Add z-level versions of the sub-ice-shelf 2D test case These tests were used to test and debug the new minLevel* additions in MPAS-Dev/MPAS-Model#825 z_level_full_cells_const_S -- a test case with constant T and S, full top and bottom cells, and the JM equation of state. Should still remain stationary to nearly machine precision z_level -- the same test case but with stratified S (and rho) and partial top and bottom cells. The partial cells will induce further pressure gradient errors and a small flow. z_level has been added to the nightly regression suite.

cbegeman changed the title ~~Ocean/new min level cell feature~~ Add the option for inactive top cells Mar 10, 2021

cbegeman requested review from mark-petersen, dengwirda and xylar March 10, 2021 17:05

cbegeman added feature Ocean labels Mar 10, 2021

xylar assigned mark-petersen Mar 10, 2021

cbegeman requested review from philipwjones and mattdturner March 10, 2021 19:11

xylar mentioned this pull request Mar 10, 2021

Change three init mode namelist options MPAS-Dev/compass#55

Merged

cbegeman force-pushed the ocean/new-min-level-cell-feature branch from f881969 to e063a30 Compare March 11, 2021 15:28

mattdturner reviewed Mar 15, 2021

View reviewed changes

src/core_ocean/mode_forward/mpas_ocn_time_integration_split.F Outdated Show resolved Hide resolved

mattdturner reviewed Mar 15, 2021

View reviewed changes

src/core_ocean/shared/mpas_ocn_diagnostics.F Outdated Show resolved Hide resolved

mattdturner reviewed Mar 16, 2021

View reviewed changes

src/core_ocean/mode_forward/mpas_ocn_time_integration_split.F Outdated Show resolved Hide resolved

mattdturner reviewed Mar 16, 2021

View reviewed changes

src/core_ocean/mode_forward/mpas_ocn_time_integration_split.F Outdated Show resolved Hide resolved

mattdturner reviewed Mar 16, 2021

View reviewed changes

src/core_ocean/mode_forward/mpas_ocn_time_integration_split.F Outdated Show resolved Hide resolved

cbegeman force-pushed the ocean/new-min-level-cell-feature branch from 8c840f5 to 7cd11b2 Compare March 16, 2021 16:56

mattdturner approved these changes Mar 22, 2021

View reviewed changes

philipwjones approved these changes Mar 22, 2021

View reviewed changes

dengwirda reviewed Mar 22, 2021

View reviewed changes

dengwirda approved these changes Mar 22, 2021

View reviewed changes

cbegeman mentioned this pull request Mar 24, 2021

Enhance the option for inactive top cells cbegeman/MPAS-Model#3

Closed

xylar merged commit 29a819a into MPAS-Dev:ocean/develop Mar 25, 2021

mark-petersen mentioned this pull request Mar 29, 2021

GOTM in MPAS-Ocean #704

Merged

cbegeman mentioned this pull request Apr 4, 2021

Enhance inactive top cells feature #840

Merged

mark-petersen added a commit to mark-petersen/MPAS-Model that referenced this pull request Apr 12, 2021

Merge branch 'ocean/develop' into e3sm/develop

cdce2f5

Add the option for inactive top cells MPAS-Dev#825

mark-petersen mentioned this pull request Apr 14, 2021

Update mpas-source: Enhance inactive top cells feature E3SM-Project/E3SM#4219

Merged

xylar mentioned this pull request Apr 30, 2021

Add z-level versions of the sub-ice-shelf 2D test case MPAS-Dev/compass#97

Merged

cbegeman deleted the ocean/new-min-level-cell-feature branch July 5, 2021 18:07

Add the option for inactive top cells #825

Add the option for inactive top cells #825

Uh oh!

Conversation

cbegeman commented Mar 10, 2021

Uh oh!

xylar commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

COMPASS (3/22)

E3SM

Uh oh!

cbegeman commented Mar 10, 2021

Uh oh!

xylar commented Mar 10, 2021

Uh oh!

xylar commented Mar 10, 2021

Uh oh!

cbegeman commented Mar 11, 2021

Uh oh!

xylar commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbegeman commented Mar 11, 2021

Uh oh!

xylar commented Mar 11, 2021

Uh oh!

cbegeman commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xylar commented Mar 11, 2021

Uh oh!

xylar commented Mar 12, 2021

Uh oh!

Uh oh!

Uh oh!

mattdturner Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattdturner commented Mar 16, 2021

Uh oh!

mattdturner left a comment

Choose a reason for hiding this comment

Uh oh!

xylar commented Mar 22, 2021

Uh oh!

mark-petersen commented Mar 22, 2021

Uh oh!

philipwjones commented Mar 22, 2021

Uh oh!

xylar commented Mar 22, 2021

Uh oh!

mattdturner commented Mar 22, 2021

Uh oh!

cbegeman commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philipwjones commented Mar 22, 2021

Uh oh!

philipwjones left a comment

Choose a reason for hiding this comment

Uh oh!

xylar commented Mar 22, 2021

Uh oh!

dengwirda Mar 22, 2021

Choose a reason for hiding this comment

Uh oh!

cbegeman Mar 22, 2021

Choose a reason for hiding this comment

Uh oh!

dengwirda Mar 22, 2021

Choose a reason for hiding this comment

Uh oh!

dengwirda left a comment

Choose a reason for hiding this comment

Uh oh!

cbegeman commented Mar 22, 2021

xylar commented Mar 10, 2021 •

edited

Loading

xylar commented Mar 11, 2021 •

edited

Loading

cbegeman commented Mar 11, 2021 •

edited

Loading

cbegeman commented Mar 22, 2021 •

edited

Loading

`PET_Ln9_P1024.ne30_oECv3.A_WCYCL1850S.compy_intel`

`SMS.T62_oQU120_ais20.MPAS_LISIO_TEST.compy_intel`

`SMS.T62_oQU120_ais20.MPAS_LISIO_TEST.compy_pgi`