Add drying slope case #352

cbegeman · 2022-04-05T20:13:44Z

This PR ports the MPAS-Ocean drying_slope test case from legacy, which is based on

Warner, J. C., Defne, Z., Haas, K., & Arango, H. G. (2013). A wetting and
drying scheme for ROMS. Computers & geosciences, 58, 54-61.

Currently, we support one vertical coordinate (sigma) and two horizontal resolutions (250m, 1km). The test case was verified to produce the same results as the legacy case.

pep8speaks · 2022-04-05T20:13:48Z

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 2022-04-19 15:28:19 UTC

xylar · 2022-04-05T20:49:01Z

This is exciting! I'll give this a look tomorrow.

I took off the "legacy" tag because we use that to indicate PRs into the legacy branch. I'll update the description of that tag to make that clear.

xylar

This is really great! I'm able to run the test cases successfully through both forward runs but the viz step is giving me some trouble.

viz is failing because the csv files are not getting downloaded automatically. It will be necessary to add all of the csv files as input files to the step, using database=drying_slope as one of the arguments.

Then, I think it would be easier if you just moved the functions from MoviePlotter and TimeSeriesPlotter into Viz. This is because there need to be some shared attributes (times, damping_coeffs and datatypes seem like some obvious ones. These could be passed from Viz over to each of the other classes but I think it just doesn't seem clearly advantageous to have 3 classes in this case.

I do realize that this approach was inherited either from IsomipPlus or from the legacy version of drying_slope (or perhaps both) and predates the current compass.

One more thing is that documentation needs to be added before this gets merged.

compass/ocean/tests/drying_slope/default/__init__.py

compass/ocean/tests/drying_slope/viz/plot.py

compass/ocean/tests/drying_slope/viz/__init__.py

cbegeman · 2022-04-08T19:56:38Z

@xylar Thanks for the detailed review and suggestions! I implemented all those changes and will get the documentation together by Monday.

xylar

This is really excellent work! The documentation is very clear. The new organization of the viz step seems to work really well, too. There are a few new PEP8 details to deal with (see #352 (comment)) and I have some additional very minor ones.

I ran both test cases and the resulting movie is really fun to watch!

xylar · 2022-04-13T13:00:33Z

compass/ocean/tests/drying_slope/viz/__init__.py

+        """
+        """


This should probably have a one-sentence description.

xylar · 2022-04-13T13:03:28Z

compass/ocean/tests/drying_slope/viz/__init__.py

+        super().__init__(test_case=test_case, name='viz')
+
+        damping_coeffs = [0.0025, 0.01]
+        times = ['0.50', '0.05', '0.40', '0.15', '0.30', '0.25']


Would it make sense for these to be in ascending order?

compass/ocean/tests/drying_slope/viz/__init__.py

cbegeman · 2022-04-13T16:24:29Z

@xylar I believe I've now made all the changes you mentioned and resolved PEP issues.

@sbrus89 Please give this a whirl when you have the chance!

Thanks to you both!

sbrus89 · 2022-04-14T14:13:25Z

@cbegeman, super. I'll test this today.

sbrus89

@cbegeman, this is a great setup! I have a few minor comments, hopefully they are useful.

sbrus89 · 2022-04-15T16:41:16Z

compass/ocean/tests/drying_slope/namelist.forward

@@ -0,0 +1,28 @@
+config_run_duration='0000_12:00:01'
+config_dt='0000_00:00:30'


The legacy case uses config_dt='0000_00:00:20' for the 250m resolution. With the current 30s dt, there are some large errors on the right boundary for the 250m case:

For a 20 second dt:

I also ran it with a 10 second dt:

The smaller dt progressively improves the boundary mismatch and also improves the drying front (especially for r=0.0025). Maybe we should with with 10s (or even 5 since there's still a slight mismatch with 10?)

@sbrus89 Good idea. How about 8s since that is roughly 1/4 the dt of the lower resolution case?

Sounds good to me.

compass/ocean/tests/drying_slope/namelist.forward

sbrus89 · 2022-04-15T16:47:51Z

compass/ocean/tests/drying_slope/namelist.forward

+config_zero_drying_velocity=.true.
+config_verify_not_dry=.true.
+config_thickness_flux_type='upwind'
+config_cvmix_background_viscosity=1e4


I think we should set config_use_cvmix = .false..

compass/ocean/tests/drying_slope/namelist.init

compass/ocean/tests/drying_slope/drying_slope.cfg

sbrus89 · 2022-04-15T16:52:08Z

compass/ocean/tests/drying_slope/drying_slope.cfg

+ny = 28
+
+# the size of grid cells (m)
+dc = 1000.0


Similarly, this doesn't seem to be needed. Can resolution be passed as an argument to self.add_step(InitialState(test_case=self)) so this can get set in InitialState.run?

The same, if this is set in the code (in configure()), it shouldn't be set here, too.

I'll defer to @xylar on whether it makes more sense to take the original approach of having these be config options that are modified in Default.configure or remove these as config options and set them in InitialState.run as you suggest. My thinking behind the use of config options was making these dimensions more externally visible, but I can see how it might be misleading since dc and ny are not independent.

I think it's better to include it in configure() and not in the config file, to make a bit clearer that the configure() version always takes priority. You can use the brand new "comment" argument to set() to have the comment included there instead.

Thanks @xylar! I didn't refresh in time so my comment was a bit lagged but it all worked out.

compass/ocean/tests/drying_slope/drying_slope.cfg

Co-authored-by: Xylar Asay-Davis <xylarstorm@gmail.com>

cbegeman · 2022-04-17T19:07:32Z

I've made the changes @sbrus89 suggested. I also rebased to make use of the new comment feature.

Note: af3b8bb makes use of additional namelist files to set config_dt for each resolution. I'm happy to change this to the add_namelist_options method in Forward step if that's preferred.

xylar · 2022-04-18T07:03:17Z

Note: af3b8bb makes use of additional namelist files to set config_dt for each resolution. I'm happy to change this to the add_namelist_options method in Forward step if that's preferred.

If it were me, I would use add_namelist_options but I don't think it's necessary to change it.

xylar

I'm good with this. Hopefully, one of you can run one last set of tests?

xylar · 2022-04-18T12:22:37Z

@dengwirda, we're ready for your review on this so we can get it in.

cbegeman · 2022-04-18T14:47:16Z

I did run these test cases with the latest commits on anvil.

docs/users_guide/ocean/test_groups/drying_slope.rst

xylar · 2022-05-03T16:36:48Z

Thanks, @cbegeman! Such a good feature to have!

cbegeman requested review from xylar, dengwirda and sbrus89 April 5, 2022 20:13

cbegeman added ocean legacy DEPRECATED: PRs or Issues related to the legacy branch of COMPASS labels Apr 5, 2022

cbegeman force-pushed the add-drying-slope-case branch 3 times, most recently from 535acf8 to 0c2354d Compare April 5, 2022 20:43

xylar removed the legacy DEPRECATED: PRs or Issues related to the legacy branch of COMPASS label Apr 5, 2022

xylar self-assigned this Apr 8, 2022

xylar reviewed Apr 8, 2022

View reviewed changes

compass/ocean/tests/drying_slope/viz/__init__.py Outdated Show resolved Hide resolved

xylar reviewed Apr 8, 2022

View reviewed changes

compass/ocean/tests/drying_slope/viz/__init__.py Outdated Show resolved Hide resolved

cbegeman force-pushed the add-drying-slope-case branch 2 times, most recently from 0d8e1de to 4ced8a3 Compare April 11, 2022 19:09

xylar reviewed Apr 13, 2022

View reviewed changes

xylar added the enhancement New feature or request label Apr 13, 2022

sbrus89 reviewed Apr 15, 2022

View reviewed changes

compass/ocean/tests/drying_slope/namelist.forward Outdated Show resolved Hide resolved

sbrus89 reviewed Apr 15, 2022

View reviewed changes

compass/ocean/tests/drying_slope/namelist.init Outdated Show resolved Hide resolved

sbrus89 reviewed Apr 15, 2022

View reviewed changes

compass/ocean/tests/drying_slope/drying_slope.cfg Outdated Show resolved Hide resolved

sbrus89 reviewed Apr 15, 2022

View reviewed changes

compass/ocean/tests/drying_slope/drying_slope.cfg Show resolved Hide resolved

cbegeman and others added 10 commits April 15, 2022 12:40

Move viz and add movie

e05f193

Move comparison data to public_html

c8713d3

Fix PEP8 issues

7e173f2

Change strings to f-strings

2a7e9a3

Co-authored-by: Xylar Asay-Davis <xylarstorm@gmail.com>

remove landice, atm forcing

c729320

Use symlinks for comparison data

6093a20

Move plotting scripts to viz object

858f5e8

Additional cleanup in response to review

b3b0c56

Add documentation for drying_slope case

260b3c6

Cleanup viz

f76ff08

cbegeman force-pushed the add-drying-slope-case branch from 294d9d9 to 5d28325 Compare April 17, 2022 19:03

cbegeman added 2 commits April 17, 2022 14:08

Remove resolution and ny from config

b61748c

Change forward namelist options

0844f09

cbegeman force-pushed the add-drying-slope-case branch from 5d28325 to 5faedd8 Compare April 17, 2022 19:09

xylar approved these changes Apr 18, 2022

View reviewed changes

sbrus89 reviewed Apr 18, 2022

View reviewed changes

docs/users_guide/ocean/test_groups/drying_slope.rst Outdated Show resolved Hide resolved

cbegeman force-pushed the add-drying-slope-case branch from 5faedd8 to c175f97 Compare April 18, 2022 15:01

cbegeman added 3 commits April 19, 2022 10:28

Change timestep for high resolution case

62deaaf

Add config option to generate movie

9d79ab1

Remove variable drag from init and forcing

98ec328

cbegeman force-pushed the add-drying-slope-case branch from c175f97 to 98ec328 Compare April 19, 2022 15:28

cbegeman mentioned this pull request Apr 21, 2022

Add single layer drying slope case #370

Merged

xylar removed the request for review from dengwirda May 3, 2022 16:35

xylar merged commit e829ea1 into MPAS-Dev:master May 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add drying slope case #352

Add drying slope case #352

cbegeman commented Apr 5, 2022

pep8speaks commented Apr 5, 2022 •

edited

Loading

xylar commented Apr 5, 2022

xylar left a comment

cbegeman commented Apr 8, 2022

xylar left a comment

xylar Apr 13, 2022

xylar Apr 13, 2022

cbegeman commented Apr 13, 2022

sbrus89 commented Apr 14, 2022

sbrus89 left a comment

sbrus89 Apr 15, 2022

cbegeman Apr 15, 2022

sbrus89 Apr 15, 2022

sbrus89 Apr 15, 2022

sbrus89 Apr 15, 2022

xylar Apr 15, 2022

cbegeman Apr 15, 2022

xylar Apr 15, 2022

cbegeman Apr 15, 2022

cbegeman commented Apr 17, 2022 •

edited

Loading

xylar commented Apr 18, 2022

xylar left a comment

xylar commented Apr 18, 2022

cbegeman commented Apr 18, 2022

xylar commented May 3, 2022

		@@ -0,0 +1,28 @@
		config_run_duration='0000_12:00:01'
		config_dt='0000_00:00:30'

Add drying slope case #352

Add drying slope case #352

Conversation

cbegeman commented Apr 5, 2022

pep8speaks commented Apr 5, 2022 • edited Loading

Comment last updated at 2022-04-19 15:28:19 UTC

xylar commented Apr 5, 2022

xylar left a comment

Choose a reason for hiding this comment

cbegeman commented Apr 8, 2022

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

cbegeman commented Apr 13, 2022

sbrus89 commented Apr 14, 2022

sbrus89 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbegeman commented Apr 17, 2022 • edited Loading

xylar commented Apr 18, 2022

xylar left a comment

Choose a reason for hiding this comment

xylar commented Apr 18, 2022

cbegeman commented Apr 18, 2022

xylar commented May 3, 2022

pep8speaks commented Apr 5, 2022 •

edited

Loading

cbegeman commented Apr 17, 2022 •

edited

Loading