Add internal wave test #249

cbegeman · 2021-09-28T20:59:15Z

This PR ports the internal wave test group from legacy compass.

from default to case directory

cbegeman · 2021-09-28T21:19:45Z

@xylar I broke something recently. I'll let you know when it's ready again.

xylar · 2021-09-28T21:20:49Z

No worries, I saw that this was in draft mode, so wasn't going to test it out until you gave the okay.

cbegeman · 2021-09-29T14:34:59Z

@xylar Actually, I think it's ready for review as-is. It failed earlier because I linked to wrong MPAS-O build. Feel free to give suggestions on the viz too. Right now it plots the initial and final states.

xylar · 2021-09-30T01:52:25Z

.gitignore

-# Ignore all PNGs
-*.png
-


Just noting that we discussed this. Ignoring PNG files causes trouble for the documentation so we don't want to include these in the `.gitignore.

xylar

Truly superb work as always. All 3 test cases together ran perfectly on Ubuntu with gfortran in under 3 minutes.

My only major concern is that the viscosities for the RPE test case aren't the same as in the original test case or the paper. Maybe that's just a question of having copied the viscosities from the baroclinic channel and not having updated them?

xylar · 2021-09-30T01:53:07Z

compass/ocean/__init__.py

@@ -23,6 +24,7 @@ def __init__(self):
        self.add_test_group(GlobalConvergence(mpas_core=self))
        self.add_test_group(GlobalOcean(mpas_core=self))
        self.add_test_group(Gotm(mpas_core=self))
+        self.add_test_group(InternalWave(mpas_core=self))


Super pedantic but could we move this down one so test groups are alphabetical?

xylar · 2021-09-30T02:12:45Z

compass/ocean/__init__.py

@@ -3,6 +3,7 @@
 from compass.ocean.tests.global_convergence import GlobalConvergence
 from compass.ocean.tests.global_ocean import GlobalOcean
 from compass.ocean.tests.gotm import Gotm
+from compass.ocean.tests.internal_wave import InternalWave


I'm much less concerned about the imports but might as well keep them alphabetical, too.

docs/users_guide/ocean/test_groups/internal_wave.rst

compass/ocean/tests/internal_wave/ten_day_test/namelist.forward

compass/ocean/tests/internal_wave/ten_day_test/streams.forward

xylar · 2021-09-30T02:51:09Z

compass/ocean/tests/internal_wave/rpe_test/__init__.py

+        name = 'rpe_test'
+        super().__init__(test_group=test_group, name=name)
+
+        nus = [1, 5, 10, 20, 200]


These are different from the viscosities in the legacy test case and in the original paper. I think these might be copied from the baroclinic channel?

Following, https://github.com/MPAS-Dev/compass/blob/legacy/ocean/internal_waves/5km/rpe_test/plot.py#L10

Suggested change

nus = [1, 5, 10, 20, 200]

nus = [0.01, 1, 15, 150]

The formatting of the step names might get a little screwed up if 0.01 turns into something with 16 digits, so you might want to do some formatting magic there.

@xylar I did notice that the viscosities were different in the legacy internal wave case and the latest baroclinic channel case. I went with the baroclinic channel choices because nu=1 seemed low enough to me. But since nu=0.01 is in the paper we should include that.

I feel like I have a slight preference for sticking with the same set of nu values as in the paper, so the 4 values in my suggestion above. That would make for an easier comparison with the model results from the paper if anyone wanted to do that. Maybe no one will actually want to do that and it's a moot point.

@mark-petersen, do you have an opinion?

xylar · 2021-09-30T02:55:39Z

compass/ocean/tests/internal_wave/rpe_test/namelist.forward

@@ -0,0 +1 @@
+config_run_duration = '20_00:00:00'


Just a note to say that I agree with reducing the run duration from 100 days to 20 days. The analysis is performed at 20 days so no added advantage in running 80 more days by default. Also, the experiments in the paper were run for 200 days, so 100 days isn't consistent with the paper either.

xylar · 2021-09-30T02:59:54Z

@mark-petersen, since you wrote the legacy test case, could you give this a look as soon as possible? We'd ideally like to merge this one soon.

Co-authored-by: Xylar Asay-Davis <[email protected]>

pep8speaks · 2021-09-30T16:06:41Z

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-10-04 17:57:37 UTC

Co-authored-by: Xylar Asay-Davis <[email protected]>

xylar · 2021-09-30T16:14:15Z

@cbegeman, when you get a chance, could you fix the PEP8 issues that the linter pointed out unless there's a good reason to keep the current formatting (e.g. long module names or URLs)?

xylar · 2021-09-30T22:13:20Z

@cbegeman, let me know when you'd like me to take another look. It seems form your commits like you've addressed all my concerns.

cbegeman · 2021-09-30T22:15:37Z

@xylar You're welcome to take another look. I did make changes in response to all your suggestions. I figured I'd just wait for @mark-petersen's review.

xylar · 2021-09-30T22:23:06Z

Sounds good!

mark-petersen · 2021-10-04T17:37:05Z

Thank you @cbegeman for this helpful conversion from compass legacy. Tested with gnu optimized on grizzly. Created all plots after run. For example, here is a plot, required little fix I just pushed.

/lustre/scratch3/turquoise/mpeterse/runs/n/211004_internal_wave_1/ocean/internal_wave/rpe_test/analysis
sections_internal_wave.png

xylar · 2021-10-04T17:44:29Z

compass/ocean/tests/internal_wave/namelist.forward

+config_use_mom_del2 = .true.
+config_mom_del2 = 10.0
+config_implicit_bottom_drag_coeff = 1.0e-2
+config_use_cvmix_convection = .false.


@mark-petersen, you pushed this change as part of your commit. I want to make sure it was intentional, since it wasn't mentioned in the commit message.

In general, I think it's a better practice to suggest changes as part of a review rather than pushing them yourself.

You are right, that was unintentional. See comment below, I was testing that at the same time. I just corrected it, but I'll do the suggest changes method next time.

cbegeman · 2021-10-04T17:45:53Z

@mark-petersen Thanks for reviewing and catching the change needed to the viz script!

mark-petersen · 2021-10-04T18:01:06Z

That last plot is not terribly insightful, but shows that everything works as expected.

Unfortunately, this PR revealed an unrelated problem (the point of these tests!). In debug, all these tests fail in the CVMix convection routine:

Program received signal SIGFPE: Floating-point exception - erroneous arithmetic operation.

Backtrace for this error:
#2  0xe411bb in __cvmix_convection_MOD_cvmix_coeffs_conv_wrap
	at /usr/projects/climate/mpeterse/repos/compass/pr/E3SM-Project/components/mpas-ocean/src/cvmix/src/shared/cvmix_convection.F90:227

222     call cvmix_coeffs_conv(new_Mdiff, new_Tdiff,                              &
223                            CVmix_vars%SqrBuoyancyFreq_iface,                  &
224                            CVmix_vars%WaterDensity_cntr,                      &
225                            CVmix_vars%AdiabWaterDensity_cntr,                 &
226                            nlev, max_nlev, nint(CVMix_vars%kOBL_depth)+1,     &
227                            CVmix_conv_params_user)

I think that last line should be

227                            CVmix_conv_params_in)

because the original was an optional argument, and in our case it is not passed in, so we get a debug error when it tries to use it. I don't know why this idealized case triggers this error (in debug only) but the global cases do not.

If that is indeed the fix, it is a correction for the cvmix repo, not even MPAS-Ocean. So I think I'll communicate with Luke and make a cvmix issue if needed, but not hold up this PR. For this test case, I think convection shouldn't even be needed - it ran fine without it.

mark-petersen

Also run successfully with intel 19 on grizzly, fails with debug as noted above. I think the cvmix convection failure is out of scope for this PR, so I'm approving it. Thanks!

mark-petersen · 2021-10-04T18:54:21Z

I confirmed that my suggested fix in cvmix above,

components/mpas-ocean/src/cvmix/src/shared/cvmix_convection.F90:227
227                            CVmix_conv_params_in)

does fix this problem, i.e. I can run the test cases in this PR with gnu debug without errors.

mark-petersen · 2021-10-04T19:30:52Z

Issue reported: CVMix/CVMix-src#92

xylar

I reran with the latest changes using gfortran on Ubuntu and everything looks good to me.

xylar · 2021-10-05T04:51:30Z

Thanks for the hard work on this one, @cbegeman!

cbegeman and others added 12 commits September 28, 2021 14:39

Add internal wave test case with init step only

265d513

Add forward step to internal wave

f987489

Set up domain tracer distribution

7f29ac3

Add vertVelocityTop to streams file

6a2bdc7

Add viz for internal wave

f7b6931

Move initial_state script and forward script ...

4652462

from default to case directory

move config file from default

9acdc5d

Add rpe and ten-day tests

48b31a3

Add documentation for internal wave tests

9c9a89f

Change analysis for rpe test to reflect compass legacy analysis

f610de1

Remove pngs from gitignore

6577f97

Add figure to docs

4fc8d68

cbegeman marked this pull request as draft September 28, 2021 20:59

xylar added enhancement New feature or request ocean python package DEPRECATED: PRs and Issues involving the python package (master branch) labels Sep 28, 2021

xylar self-requested a review September 28, 2021 21:00

xylar self-assigned this Sep 28, 2021

xylar added the in progress This PR is not ready for review or merging label Sep 28, 2021

cbegeman marked this pull request as ready for review September 29, 2021 14:35

xylar removed the in progress This PR is not ready for review or merging label Sep 29, 2021

xylar reviewed Sep 30, 2021

View reviewed changes

xylar requested a review from mark-petersen September 30, 2021 02:59

Add global stats to output

995273f

Co-authored-by: Xylar Asay-Davis <[email protected]>

cbegeman and others added 2 commits September 30, 2021 10:08

Fixup namelist dt syntax

cf42c4e

Co-authored-by: Xylar Asay-Davis <[email protected]>

Fixup docs for 80 char lines

36dbf74

Co-authored-by: Xylar Asay-Davis <[email protected]>

cbegeman force-pushed the add-internal-wave-test branch from 139d54f to 9c2b4d6 Compare September 30, 2021 17:23

Resolve PEP8 style issues

a58f43d

cbegeman force-pushed the add-internal-wave-test branch from 9c2b4d6 to a58f43d Compare September 30, 2021 17:24

Change nu values for consistency with Petersen et al. 2015

010e841

xylar reviewed Oct 4, 2021

View reviewed changes

correct nRows in analysis plot

cb36336

mark-petersen force-pushed the add-internal-wave-test branch from 3f1ddf9 to cb36336 Compare October 4, 2021 17:57

mark-petersen approved these changes Oct 4, 2021

View reviewed changes

xylar approved these changes Oct 5, 2021

View reviewed changes

xylar merged commit c60b52f into MPAS-Dev:master Oct 5, 2021

mark-petersen mentioned this pull request Oct 29, 2021

Optional argument CVmix_conv_params_user used in subroutine. CVMix/CVMix-src#92

Open

cbegeman deleted the add-internal-wave-test branch August 23, 2023 20:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add internal wave test #249

Add internal wave test #249

cbegeman commented Sep 28, 2021

cbegeman commented Sep 28, 2021

xylar commented Sep 28, 2021

cbegeman commented Sep 29, 2021

xylar Sep 30, 2021

xylar left a comment

xylar Sep 30, 2021

xylar Sep 30, 2021

xylar Sep 30, 2021

cbegeman Sep 30, 2021

xylar Sep 30, 2021

xylar Sep 30, 2021

xylar commented Sep 30, 2021

pep8speaks commented Sep 30, 2021 •

edited

Loading

xylar commented Sep 30, 2021

xylar commented Sep 30, 2021

cbegeman commented Sep 30, 2021

xylar commented Sep 30, 2021

mark-petersen commented Oct 4, 2021

xylar Oct 4, 2021

mark-petersen Oct 4, 2021

cbegeman commented Oct 4, 2021

mark-petersen commented Oct 4, 2021

mark-petersen left a comment

mark-petersen commented Oct 4, 2021

mark-petersen commented Oct 4, 2021

xylar left a comment

xylar commented Oct 5, 2021

Add internal wave test #249

Add internal wave test #249

Conversation

cbegeman commented Sep 28, 2021

cbegeman commented Sep 28, 2021

xylar commented Sep 28, 2021

cbegeman commented Sep 29, 2021

Choose a reason for hiding this comment

xylar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xylar commented Sep 30, 2021

pep8speaks commented Sep 30, 2021 • edited Loading

Comment last updated at 2021-10-04 17:57:37 UTC

xylar commented Sep 30, 2021

xylar commented Sep 30, 2021

cbegeman commented Sep 30, 2021

xylar commented Sep 30, 2021

mark-petersen commented Oct 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbegeman commented Oct 4, 2021

mark-petersen commented Oct 4, 2021

mark-petersen left a comment

Choose a reason for hiding this comment

mark-petersen commented Oct 4, 2021

mark-petersen commented Oct 4, 2021

xylar left a comment

Choose a reason for hiding this comment

xylar commented Oct 5, 2021

pep8speaks commented Sep 30, 2021 •

edited

Loading