Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(+) porous topography implementation #3

Merged
merged 9 commits into from
Nov 30, 2021

Conversation

sditkovsky
Copy link

Initial implementation of porous topography modifying only cell face metrics (grid volumes are not modified). Cell faces are modified in the continuity scheme (MOM_continuity_PPM.F90), Coriolis acceleration (MOM_CoriolisAdv.F90), and the Rayleigh bottom channel drag (MOM_set_viscosity.F90).

Porous channels can be set in the CHANNEL_LIST_FILE using the following a similar template to existing barotropic channel restrictions:

barotropic: [U|V]_width, min_longitude, max_longitude, min_latitude, max_latitude, width
baroclinic: [U|V]_WIDTH_POR, min_lon, max_lon, min_lat, max_lat, width, min_bathy, max_bathy, avg__bathy

The porous topography implementation was tested for scaling and rotational consistency.

* reads in porous topography parameters from CHANNEL_LIST_FILE

*new module to compute curve fit for porous topography

*porous constraints used to modify continuity_PPM, CoriolisAdv, and Rayleigh bottom channel drag
@sditkovsky sditkovsky changed the title (+) porous topography implementation (#3) (+) porous topography implementation Nov 17, 2021
@codecov
Copy link

codecov bot commented Nov 17, 2021

Codecov Report

Merging #3 (8620742) into dev/gfdl (32d0a4e) will decrease coverage by 0.02%.
The diff coverage is 28.11%.

❗ Current head 8620742 differs from pull request most recent head 3f38239. Consider uploading reports for the commit 3f38239 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##           dev/gfdl       #3      +/-   ##
============================================
- Coverage     29.18%   29.15%   -0.03%     
============================================
  Files           239      240       +1     
  Lines         71294    71456     +162     
============================================
+ Hits          20810    20836      +26     
- Misses        50484    50620     +136     
Impacted Files Coverage Δ
src/core/MOM_grid.F90 61.68% <ø> (ø)
src/core/MOM_transcribe_grid.F90 31.32% <0.00%> (-1.18%) ⬇️
src/core/MOM_variables.F90 28.57% <ø> (ø)
src/framework/MOM_dyn_horgrid.F90 32.17% <0.00%> (-0.87%) ⬇️
src/initialization/MOM_shared_initialization.F90 27.70% <0.00%> (-1.79%) ⬇️
src/core/MOM_CoriolisAdv.F90 39.50% <14.28%> (ø)
src/core/MOM_porous_barriers.F90 19.17% <19.17%> (ø)
...c/parameterizations/vertical/MOM_set_viscosity.F90 47.41% <20.00%> (-0.09%) ⬇️
src/core/MOM_continuity_PPM.F90 39.42% <38.77%> (-0.62%) ⬇️
src/core/MOM_dynamics_split_RK2.F90 61.01% <41.17%> (-0.36%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32d0a4e...3f38239. Read the comment docs.

Copy link
Member

@adcroft adcroft left a comment

Choose a reason for hiding this comment

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

This is looking very good and mostly clean. There are a few specific adjustments/comments in line which are easy changes.

It passed regression tests at https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/14109.
Note to devs: there are new diagnostics so M6E needs updating.

src/core/MOM_dynamics_split_RK2.F90 Outdated Show resolved Hide resolved
src/core/MOM_dynamics_unsplit.F90 Outdated Show resolved Hide resolved
src/core/MOM_dynamics_unsplit_RK2.F90 Outdated Show resolved Hide resolved
src/core/MOM_porous_barriers.F90 Outdated Show resolved Hide resolved
src/core/MOM_porous_barriers.F90 Outdated Show resolved Hide resolved
src/core/MOM_variables.F90 Show resolved Hide resolved
src/core/MOM.F90 Show resolved Hide resolved
src/core/MOM_porous_barriers.F90 Outdated Show resolved Hide resolved
src/core/MOM_porous_barriers.F90 Show resolved Hide resolved
src/core/MOM_porous_barriers.F90 Outdated Show resolved Hide resolved
* Style Fixes

* Store depths for porous curve fitting as [Z ~> m] and account for Z_ref
@sditkovsky
Copy link
Author

Thank you @adcroft @Hallberg-NOAA @marshallward for the feedback. I've pushed some fixes that I think address all of the above concerns.

* changed porous topo code to preserve dimensions

* readded rotations that got lost in merge
@sditkovsky
Copy link
Author

Sorry, my last patch didn't merge in properly and the commits got a bit messy. I'll fix things now

@marshallward
Copy link
Member

marshallward commented Nov 25, 2021

@sditkovsky If you want to reshape your commit and prune the small edits, you can use git rebase -i dev/gfdl to combine commits together, and also to shift everything ahead of the most recent dev/gfdl.

If you want to experiment first, you can try it on a new branch (git branch -d test_branch)

Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

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

After a number of rounds of review, I think that this excellent contribution is ready to be merged in once it has successfully passed the regression tests.

@Hallberg-NOAA
Copy link
Member

Hallberg-NOAA commented Nov 30, 2021

This PR has passed pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/14228, although there will have to be an update to MOM6-examples due to the new diagnostics and entries in the MOM_parameter_doc files.

@Hallberg-NOAA Hallberg-NOAA merged commit 2087e05 into NOAA-GFDL:dev/gfdl Nov 30, 2021
adcroft added a commit to adcroft/MOM6 that referenced this pull request Dec 1, 2021
- Spacing within expressions was uneven and made multiplation look like
  POW functions. Leftover from merging NOAA-GFDL#3.
- No answer changes.
marshallward pushed a commit that referenced this pull request Dec 1, 2021
- Spacing within expressions was uneven and made multiplation look like
  POW functions. Leftover from merging #3.
- No answer changes.
marshallward pushed a commit that referenced this pull request Dec 29, 2021
  Use the por_face_area[UV] in the effective thickness calculations in
zonal_face_thickness and merid_face_thickness, so that they are more consistent
with their use elsewhere in the code for the relative weights in calculating the
barotropic accelerations.  Because these por_face_area arrays are still 1 in all
test cases, the answers are unchanged in any test cases from before a few weeks
ago, but there could be answer changes in cases that are using the very recently
added capability (in PR #3) to set fractional face areas.  This change was
discussed with Sam Ditkovsky, and agreed that there is no reason to keep the
ability to recover the previous answers in any cases that use the recently added
partial face width option.

  This commit also expanded the comments describing the h_u and h_v arguments to
btcalc(), zonal_face_thickness(), and merid_face_thickness() routines, the
diag_h[uv] elements of the accel_diag_ptrs type and the h_u and h_v elements of
the BT_cont_type.

  All answers and output are bitwise identical in the MOM6-examples test suite
and TC tests, but answer changes are possible in cases using a very recently
added code option.
marshallward pushed a commit that referenced this pull request Jan 26, 2022
* additions for stochastic physics and ePBL perts

* cleanup of code and enhancement of ePBL perts

* Update MOM_diabatic_driver.F90

remove conflict with dev/emc

* Update MOM_diabatic_driver.F90

further resolve conflict

* Update MOM_diabatic_driver.F90

put id_sppt_wts, etc back.

* add stochy_restart writing to mom_cap

* additions for stochy restarts

* clean up debug statements

* clean up code

* fix non stochastic ePBL calculation

* re-write of stochastic code to remove CPP directives

* remove blank link in MOM_diagnostics

* clean up MOM_domains

* make stochastics optional

* correct coupled_driver/ocean_model_MOM.F90 and other cleanup

* clean up of code for MOM6 coding standards

* remove stochastics container

* revert MOM_domains.F90

* clean up of mom_ocean_model_nuopc.F90

* remove PE_here from mom_ocean_model_nuopc.F90

* remove debug statements

* stochastic physics re-write

* move stochastics to external directory

* doxygen cleanup

* add write_stoch_restart_ocn to MOM_stochastics

* add logic to remove incrments from restart if outside IAU window

* revert logic wrt increments

* add comments

* update to gfdl 20210806 (#74)

* remove white space and fix comment

* Update MOM_oda_incupd.F90

remove unused index bounds, and fix sum_h2 loop.

Co-authored-by: pjpegion <Philip.Pegion@noaa.gov>
Co-authored-by: Marshall Ward <marshall.ward@noaa.gov>

* Fussing with zotero.bib.

Getting a warning about a repeated bibliography entry for adcroft2004.
Rob thinks this is a hash failure.

* Still fussing with zotero.bib

- it was complaining about the (unused) Kasahara reference.

* Several little things, one is making sponge less verbose.

- Pointing to OBC wiki file from the lateral parameterizations doc.

- Using the MOM6 verbosity to control the time_interp verbosity.

- Making the check for negative water depths more informative.

* return a more accurate error message in MOM_stochasics

* Working on boundary layer docs.

* Done with EPBL docs?

* Undoing some patches from others

* Cleaning up too-new commits

* Adding in that SAL commit again.

* correction on type in directory name

* Added some to vertical viscisity doc.

* Cleaned up whitespace leftover from porous topomerge.

- Spacing within expressions was uneven and made multiplation look like
  POW functions. Leftover from merging #3.
- No answer changes.

* Fix out-of-bounds k index in PPM flux

- An errant use of the porous face area led to an out-of-bounds k-index
  reported in #19.
- Closes #19

* Adding Channel drag figure

* Take cite out of figure caption.

* Copyright year 2022
@Hallberg-NOAA Hallberg-NOAA added the enhancement New feature or request label Feb 1, 2022
kshedstrom pushed a commit to ESMG/MOM6 that referenced this pull request Apr 12, 2024
…GFDL#3)

* mod g_tracer interface

* mod g_tracer interface

* Simplify g_tracer interface changes

* add description
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants