Skip to content

Add advection operator convergence test #8

Merged
xylar merged 16 commits intoMPAS-Dev:masterfrom
xylar:add_advection_convergence_test
Jan 20, 2021
Merged

Add advection operator convergence test #8
xylar merged 16 commits intoMPAS-Dev:masterfrom
xylar:add_advection_convergence_test

Conversation

@xylar
Copy link
Collaborator

@xylar xylar commented Nov 6, 2020

This merge contains the COMPASS changes from: MPAS-Dev/MPAS-Model#583

Adds the Skamarock and Gassmann cosine bell test case and a regression
suite across resolutions to test operator convergence

This PR requires the addition of the cosine bell test case in MPAS-Model init mode, which is in MPAS-Dev/MPAS-Model#583, or changing the initial_state step to a python script.

Reference: Section 2a in Skamarock, W.C. and A. Gassmann, 2011: Conservative Transport Schemes for Spherical Geodesic Grids: High-Order Flux Operators for ODE-Based Time Integration. Mon. Wea. Rev., 139, 2962–2975, https://doi.org/10.1175/MWR-D-10-05056.1

@xylar xylar self-assigned this Nov 6, 2020
@xylar xylar added enhancement New feature or request ocean labels Nov 6, 2020
@xylar xylar force-pushed the add_advection_convergence_test branch from bc0caf5 to 7553b9e Compare November 9, 2020 20:38
@xylar xylar marked this pull request as ready for review November 9, 2020 20:39
@xylar
Copy link
Collaborator Author

xylar commented Nov 9, 2020

We will want to take out the COMPASS changes from MPAS-Dev/MPAS-Model#583. Then, that PR and this one will be our first test of making changes to both MPAS-Ocean init mode and COMPASS at the same time. Let's see how it goes...

Copy link
Collaborator

@vanroekel vanroekel left a comment

Choose a reason for hiding this comment

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

approved by visual inspection and verifying test set up correctly on grizzly

@mark-petersen
Copy link
Collaborator

Well, I tricked myself the very first time. I tried to run this with the head of ocean/develop, forgetting that this PR depends on changes from MPAS-Model init mode. Unfortunately, there is no MPAS-Ocean init mode error if you pick a test case that doesn't exist. Instead, you get an indecipherable error during simulation start-up:

App launch reported: 1 (out of 1) daemons - 2 (out of 2) procs
[gr1281:28737] *** An error occurred in MPI_Isend
[gr1281:28737] *** reported by process [2296512513,1]
[gr1281:28737] *** on communicator MPI COMMUNICATOR 3 DUP FROM 0
[gr1281:28737] *** MPI_ERR_COUNT: invalid count argument
[gr1281:28737] *** MPI_ERRORS_ARE_FATAL (processes in this communicator will now abort,
[gr1281:28737] ***    and potentially your MPI job)

Once I pointed the compass config to the executable and namelists from MPAS-Dev/MPAS-Model#583, it works fine.

This is a good chance to switch a test case from init mode to a python script. I have a working example with xarray here:
https://github.com/mark-petersen/MPAS-Model/blob/internal_tide_test_case/testing_and_setup/compass/ocean/internal_tide/3km/RK4/add_initial_state.py
that could be altered for this test case.

@xylar
Copy link
Collaborator Author

xylar commented Nov 14, 2020

@mark-petersen, in your example, you use things like:

    vars2D = ['ssh', 'bottomDepth', 'bottomDepthObserved',
        'surfaceStress', 'atmosphericPressure', 'boundaryLayerDepth']
    for var in vars2D:
        globals()[var] = np.nan * np.ones(nCells)

This is fine for a one-off but I don't think this is acceptable as a general practice for python coding. anytime you find yourself using globals(), there's got to be a better, more "pythonic" way to do it. I'm only pointing this out because I don't want this example to become a template for future test cases initialized in python.

That being said, I'm working on initializing a test case in python right now as well, and I really appreciate this example for comparison, since I'm running into a bit of trouble.

Given the work @vanroekel has already put into MPAS-Dev/MPAS-Model#583, I think the right thing to do would be to rebase that branch, removing changes in testing_and_setup/compass and to merge that PR and this one at roughly the same time, rather than trying to remove the init-mode changes in this case. We can encourage that approach in the future, though.

@xylar
Copy link
Collaborator Author

xylar commented Nov 14, 2020

@mark-petersen, Regarding the confusing error, can you track down why this isn't giving a more useful error message and fix it somehow? Or at least create an issue on MPAS-Model?

@xylar xylar force-pushed the add_advection_convergence_test branch from 7553b9e to b01187c Compare November 14, 2020 21:49
@mark-petersen
Copy link
Collaborator

I reran with the rebased code and executable from MPAS-Dev/MPAS-Model#583. Everything works great. Here is that wonderful convergence plot:
convergence
see

/lustre/scratch3/turquoise/mpeterse/runs/nightly/ocean_model_201114_cosine_bell2/ocean/convergence_global

@mark-petersen
Copy link
Collaborator

I used these lines:

    vars2D = ['ssh', 'bottomDepth', 'bottomDepthObserved',
        'surfaceStress', 'atmosphericPressure', 'boundaryLayerDepth']
    for var in vars2D:
        globals()[var] = np.nan * np.ones(nCells)

because otherwise for xarray you need one line per variable at the top, and another one at the bottom, and you have to ensure that the dimensions match. That seemed like a lot of bookkeeping, so I switched it to a few lists instead.

@mark-petersen
Copy link
Collaborator

See MPAS-Dev/MPAS-Model#758 for issue.

@mark-petersen
Copy link
Collaborator

Ran convergence test with rebased PR here: MPAS-Dev/MPAS-Model#583 with gnu debug an optimized, produces same plot as above. Takes 12 minutes on grizzly with 4 nodes.

jonbob added a commit to E3SM-Project/E3SM that referenced this pull request Jan 18, 2021
Add cosine bell advection test case

This PR brings in a new mpas-source submodule with changes only to the
ocean core. It adds initial condition for cosine bell test case, used for
converge test of tracer advection.

See MPAS-Dev/MPAS-Model#583 and MPAS-Dev/compass#8.

[BFB]
jonbob added a commit to E3SM-Project/E3SM that referenced this pull request Jan 19, 2021
Add cosine bell advection test case

This PR brings in a new mpas-source submodule with changes only to the
ocean core. It adds initial condition for cosine bell test case, used
for converge test of tracer advection.

See MPAS-Dev/MPAS-Model#583 and MPAS-Dev/compass#8.

[BFB]
@pep8speaks
Copy link

pep8speaks commented Jan 20, 2021

Hello @xylar! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 7:1: E402 module level import not at top of file

Comment last updated at 2021-01-20 12:11:47 UTC

@xylar xylar added MPAS-Model PR finished DEPRECATED and removed MPAS-Model PR required DEPRECATED labels Jan 20, 2021
Adds the Skamarock and Gassmann cosine bell test case and a regression
suite across resolutions to test operator convergence
@xylar xylar force-pushed the add_advection_convergence_test branch from e301b71 to 51368dc Compare January 20, 2021 12:11
@xylar
Copy link
Collaborator Author

xylar commented Jan 20, 2021

I rebased and update the submodule for MPAS-Model/ocean/develop. I reran the test suite on Anvil. The results look similar to before:

convergence

But I wanted to note that the order of convergence is noticeably lower and the error in each test is noticeably higher than in September when @mark-petersen last posted a plot: MPAS-Dev/MPAS-Model#583 (review)

image

Did parameters change that would account for this? Longer integration time?

@xylar
Copy link
Collaborator Author

xylar commented Jan 20, 2021

@vanroekel and @mark-petersen, thank you for your reviews earlier. I'm going to go ahead and merge because the order of convergence isn't related to this PR. But it would have been good if this had been noticed and discussed before MPAS-Dev/MPAS-Model#583 went in. It also would have been good if we had had this regression suite since September as a tool to find out when things changed. Part of our slow ocean/develop to E3SM process that I think is hindering us more than it's helping.

@xylar xylar merged commit d1ba753 into MPAS-Dev:master Jan 20, 2021
@xylar xylar deleted the add_advection_convergence_test branch January 20, 2021 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request MPAS-Model PR finished DEPRECATED ocean

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants