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

Dimension: Diagnostic fixes #1220

Merged
merged 4 commits into from
Oct 15, 2020

Conversation

marshallward
Copy link
Collaborator

This patch fixes the dimensional scaling of three diagnostics:

  • grid_Re_Ah
  • grid_Re_Kh
  • boundary_forcing_h_tendency

The fix to grid_Re_Ah and grid_Re_Kh removes the Kh_min and
Ah_min parameters, which could not be dimensionally scaled and
replaces them with variables in the control structure.

The minimum Laplacian and biharmonic viscosites are chosed to estimate
the smallest viscosity which is capable to altering the velocity. We
use spacing(1.) * L^2N / T where spacing() is the smallest
difference in floating point. We use the minimum harmonic grid spacing,
precomputed during initialization, rather than the local value, in order
to avoid additional computation and memory access during timestepping.

This will changes the value of grid_Re_[AK]h in certain isolated
examples of negligible viscosity, but will otherwise avoid the
possibility of divide-by-zero while also producing a realizable large
value at near-inviscid upper limit.

The second fix to boundary_focing_h_tendency had its T units set but
not its H units. This patch fixes the dimensionality.

These are required to fix the dimensional verification errors in PR
1210.

This patch fixes the dimensional scaling of three diagnostics:

- grid_Re_Ah
- grid_Re_Kh
- boundary_forcing_h_tendency

The fix to `grid_Re_Ah` and `grid_Re_Kh` removes the `Kh_min` and
`Ah_min` parameters, which could not be dimensionally scaled and
replaces them with variables in the control structure.

The minimum Laplacian and biharmonic viscosites are chosed to estimate
the smallest viscosity which is capable to altering the velocity.  We
use `spacing(1.) * L^2N / T` where `spacing()` is the smallest
difference in floating point.  We use the minimum harmonic grid spacing,
precomputed during initialization, rather than the local value, in order
to avoid additional computation and memory access during timestepping.

This will changes the value of grid_Re_[AK]h in certain isolated
examples of negligible viscosity, but will otherwise avoid the
possibility of divide-by-zero while also producing a realizable large
value at near-inviscid upper limit.

The second fix to `boundary_focing_h_tendency` had its T units set but
not its H units.  This patch fixes the dimensionality.

These are required to fix the dimensional verification errors in PR
1210.
@marshallward
Copy link
Collaborator Author

Sorry I just found a bug in this, missed by the testing. I will update shortly.

The single-line if-block for reading the DT parameter did not contain
the Idt = 1. / dt update.

This patch amends this bug.
@codecov-commenter
Copy link

codecov-commenter commented Oct 5, 2020

Codecov Report

Merging #1220 into dev/gfdl will decrease coverage by 0.00%.
The diff coverage is 35.19%.

Impacted file tree graph

@@             Coverage Diff              @@
##           dev/gfdl    #1220      +/-   ##
============================================
- Coverage     46.08%   46.08%   -0.01%     
============================================
  Files           214      224      +10     
  Lines         69399    70605    +1206     
============================================
+ Hits          31984    32535     +551     
- Misses        37415    38070     +655     
Impacted Files Coverage Δ
...g_src/external/GFDL_ocean_BGC/FMS_coupler_util.F90 0.00% <0.00%> (ø)
...fig_src/external/GFDL_ocean_BGC/generic_tracer.F90 0.00% <0.00%> (ø)
...c/external/GFDL_ocean_BGC/generic_tracer_utils.F90 0.00% <0.00%> (ø)
config_src/external/ODA_hooks/kdtree.f90 0.00% <0.00%> (ø)
config_src/external/ODA_hooks/ocean_da_core.F90 0.00% <0.00%> (ø)
config_src/external/ODA_hooks/ocean_da_types.F90 0.00% <0.00%> (ø)
config_src/external/ODA_hooks/write_ocean_obs.F90 0.00% <0.00%> (ø)
config_src/solo_driver/MESO_surface_forcing.F90 0.00% <0.00%> (ø)
config_src/solo_driver/user_surface_forcing.F90 0.00% <0.00%> (ø)
src/ALE/MOM_regridding.F90 31.47% <0.00%> (-0.17%) ⬇️
... and 214 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 7cb606e...cbe88ae. Read the comment docs.

@marshallward
Copy link
Collaborator Author

This is required for #1210

Copy link
Collaborator

@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.

I may be wrong, but it looks to me like min_grid_sp_h2 and min_grid_sp_h4 in hor_visc_init() will vary between processors (unless a Cartesian grid is used) and that they will not be invariant to domain decomposition. If so, CS%min_grid_Ah and CS%min_grid_Kh will also not be invariant to domain decomposition. Please address this concern before these changes can be approved.

An additional `min_across_PEs` call is added to the min_grid_sp_h[24]
calculations, to accommodate for parallel builds and ensure consistency
across layouts.
@marshallward
Copy link
Collaborator Author

Thank you @Hallberg-NOAA , I have added min_across_PEs calls to account for multiple PEs and layouts.

@codecov-io
Copy link

Codecov Report

Merging #1220 into dev/gfdl will decrease coverage by 0.00%.
The diff coverage is 35.19%.

Impacted file tree graph

@@             Coverage Diff              @@
##           dev/gfdl    #1220      +/-   ##
============================================
- Coverage     46.08%   46.08%   -0.01%     
============================================
  Files           214      224      +10     
  Lines         69399    70605    +1206     
============================================
+ Hits          31984    32535     +551     
- Misses        37415    38070     +655     
Impacted Files Coverage Δ
...g_src/external/GFDL_ocean_BGC/FMS_coupler_util.F90 0.00% <0.00%> (ø)
...fig_src/external/GFDL_ocean_BGC/generic_tracer.F90 0.00% <0.00%> (ø)
...c/external/GFDL_ocean_BGC/generic_tracer_utils.F90 0.00% <0.00%> (ø)
config_src/external/ODA_hooks/kdtree.f90 0.00% <0.00%> (ø)
config_src/external/ODA_hooks/ocean_da_core.F90 0.00% <0.00%> (ø)
config_src/external/ODA_hooks/ocean_da_types.F90 0.00% <0.00%> (ø)
config_src/external/ODA_hooks/write_ocean_obs.F90 0.00% <0.00%> (ø)
config_src/solo_driver/MESO_surface_forcing.F90 0.00% <0.00%> (ø)
config_src/solo_driver/user_surface_forcing.F90 0.00% <0.00%> (ø)
src/ALE/MOM_regridding.F90 31.47% <0.00%> (-0.17%) ⬇️
... and 214 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 48da975...1f02e4e. Read the comment docs.

@marshallward marshallward mentioned this pull request Oct 9, 2020
Copy link
Collaborator

@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.

With the updates, this change should now reproduce properly across PE layouts, and it does address the issues with the hard-coded dimensional constants.

@Hallberg-NOAA
Copy link
Collaborator

This PR is being tested with https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/11386.

@Hallberg-NOAA
Copy link
Collaborator

The pipeline tests and Travis tests are all passing with this PR. It can be merged in.

@Hallberg-NOAA Hallberg-NOAA merged commit f3196d3 into mom-ocean:dev/gfdl Oct 15, 2020
@marshallward marshallward deleted the tc4_dim_fixes branch January 18, 2021 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants