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

+*Non-Boussinesq wave_speed calculations #429

Merged
merged 3 commits into from
Oct 5, 2023

Conversation

Hallberg-NOAA
Copy link
Member

Modified the internal wave speed calculation to work in non-Boussinesq mode without any dependencies on the Boussinesq reference density (RHO_0). Many factors of GV%H_to_Z or its inverse are cancelled out in MOM_wave_speed.F90 by working directly in thickness units. The code now uses specific volume derivatives to set the values of g_prime at interfaces when in non-Boussinesq mode, regardless of whether an equation of state is used.

This commit also modifies the wave structure calculations in wave_speeds, which includes the use of thickness_to_dz, changes to the units of three arguments to wave_speeds and their counterparts in int_tide_CS, and a number of duplicated calculations of vertical extents mirroring the calculations of thicknesses. Some diagnostic conversion factors were modified accordingly in MOM_internal_tides.

This commit involves changing the units of 19 internal variables in wave_speed and 17 internal variables in wave_speeds to use thickness units or other related units. There are 6 new or renamed internal variables in wave_speed and 10 new or renamed variables in wave_speeds. A total 34 thickness rescaling factors or references to GV%Rho0 were cancelled out or replaced. Missing comments describing the units of several real variables were also added.

All answers are bitwise identical in Boussinesq mode, but answers do change in non-Boussinesq solutions that depend on the internal wave speed. This commit eliminates the dependencies of the non-Boussinesq wave speed calculations on the Boussinesq reference density.

@Hallberg-NOAA Hallberg-NOAA added enhancement New feature or request answer-changing A change in results (actual or potential) labels Aug 2, 2023
@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Merging #429 (6f8d095) into dev/gfdl (e2d244f) will decrease coverage by 0.05%.
The diff coverage is 19.63%.

❗ Current head 6f8d095 differs from pull request most recent head 824812f. Consider uploading reports for the commit 824812f to get more accurate results

@@             Coverage Diff              @@
##           dev/gfdl     #429      +/-   ##
============================================
- Coverage     37.88%   37.84%   -0.05%     
============================================
  Files           270      270              
  Lines         78127    78231     +104     
  Branches      14457    14477      +20     
============================================
+ Hits          29602    29607       +5     
- Misses        43146    43236      +90     
- Partials       5379     5388       +9     
Files Coverage Δ
...parameterizations/vertical/MOM_diabatic_driver.F90 46.86% <66.66%> (+<0.01%) ⬆️
...c/parameterizations/lateral/MOM_internal_tides.F90 0.00% <0.00%> (ø)
src/diagnostics/MOM_wave_speed.F90 33.10% <22.70%> (-4.41%) ⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@raphaeldussin
Copy link

OM4 crashes with and without internal tides with extreme value

WARNING from PE   199: btstep: negative eta in a non-Boussinesq barotropic solver.


WARNING from PE    34: Extreme surface sfc_state detected: i=1315 j= 115 lon=  28.625 lat= -69.442 x=  29.038 y= -69.442 D= 9.5000E+00 SSH=-3.8235E+01 SST=-1.0669E+00 SSS= 3.3621E+01 U-= 4.8804E+00 U+=-4.8804E+00 V-= 5.6169E+00 V+=-5.6435E+00

@raphaeldussin
Copy link

raphaeldussin commented Aug 25, 2023

problem also happens in dev/gfdl and requires #override Z_INIT_REMAP_GENERAL = True to work

Copy link

@raphaeldussin raphaeldussin left a comment

Choose a reason for hiding this comment

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

internal tides crashes in both Boussinesq and non-Boussinesq because of division by zero. From the traceback, the suspects are the lines aforementioned. The new variable dzc appear problematic.

Copy link

@raphaeldussin raphaeldussin left a comment

Choose a reason for hiding this comment

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

internal tides crashes in both Boussinesq and non-Boussinesq because of division by zero. From the traceback, the suspects are the lines aforementioned. The new variable dzc appear problematic.

src/diagnostics/MOM_wave_speed.F90 Outdated Show resolved Hide resolved
src/diagnostics/MOM_wave_speed.F90 Outdated Show resolved Hide resolved
@Hallberg-NOAA
Copy link
Member Author

I have updated this PR to calculate the full range of wave speeds that are used in the internal_tides code, and to restrict the loop ranges in the internal_tides code to those that seem to be necessary. The separate issue of the 0 values of tv%SpV_avg still needs to be investigated, but I am optimistic that once that is addressed we will have working internal_tides capabilities.

  Modified the internal wave speed calculation to work in non-Boussinesq mode
without any dependencies on the Boussinesq reference density (RHO_0).  Many
factors of GV%H_to_Z or its inverse are cancelled out in MOM_wave_speed.F90 by
working directly in thickness units.  The code now uses specific volume
derivatives to set the values of g_prime at interfaces when in non-Boussinesq
mode, regardless of whether an equation of state is used.

  This commit also modifies the wave structure calculations in wave_speeds,
which includes the use of thickness_to_dz, changes to the units of three
arguments to wave_speeds and their counterparts in int_tide_CS, and a number of
duplicated calculations of vertical extents mirroring the calculations of
thicknesses.  Some diagnostic conversion factors were modified accordingly in
MOM_internal_tides.

  This commit involves changing the units of 19 internal variables in wave_speed
and 17 internal variables in wave_speeds to use thickness units or other related
units.  There are 6 new or renamed internal variables in wave_speed and 10 new
or renamed variables in wave_speeds.  A total 34 thickness rescaling factors or
references to GV%Rho0 were cancelled out or replaced.  Missing comments
describing the units of several real variables were also added.

  All answers are bitwise identical in Boussinesq mode, but answers do change in
non-Boussinesq solutions that depend on the internal wave speed.  This commit
eliminates the dependencies of the non-Boussinesq wave speed calculations on the
Boussinesq reference density.
  Replace the optional full_halos logical argument to wave_speed and wave_speeds
with an optional halo_size integer argument, following the pattern used
elsewhere in the MOM6 code, with a similar change to itidal_lowmode_loss.  This
halo_size argument is used in the call to thickness_to_dz in wave_speeds, and
the call to wave_speeds in propagate_int_tides was modified accordingly.  In
addition, the loop bounds in the MOM_internal_tides code were modified to avoid
excessive calculations over the full data domain, some of which would use
uninitialized variables.  Also modified the logic determining the value of
diabatic_halo as returned by extract_diabatic_member to account for the wider
halos that are needed when the internal tide code is used.  This commit changes
the interfaces publicly visible routines and it changes answers in cases when
the internal_tides are used by doing calculations of the wave speeds in halo
points that are used in that code.
@Hallberg-NOAA
Copy link
Member Author

I have now updated the halo size over which tv%SpV_avg is calculated when the internal tides code is in use, which may (hopefully) solve the remaining issues with this PR.

@raphaeldussin
Copy link

I have tested the latest change and they fixed the aforementioned issues. Approved!

@marshallward
Copy link
Member

Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/20836 ✔️

Copy link
Member

@marshallward marshallward left a comment

Choose a reason for hiding this comment

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

@marshallward marshallward merged commit 2047676 into NOAA-GFDL:dev/gfdl Oct 5, 2023
10 checks passed
@Hallberg-NOAA Hallberg-NOAA deleted the nonBous_wave_speed branch November 8, 2023 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
answer-changing A change in results (actual or potential) enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants