-
Notifications
You must be signed in to change notification settings - Fork 65
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
*+Fix non-Boussinesq MASS_WEIGHT_IN_PGF bug #692
*+Fix non-Boussinesq MASS_WEIGHT_IN_PGF bug #692
Conversation
Optionally corrected a bug (essentially a sign error) in the selection of where MASS_WEIGHT_IN_PRESSURE_GRADIENT is applied in non-Boussinesq test cases. The (incorrect) non-Boussinesq version of the test for where interfaces are outside of the range of hydrostatic consistency due to interactions with bathymetry was converted from the (correct) Boussinesq version without properly taking into account that pressure increases downward while height increases upward. This bug is repeated 12 times in 6 different specific volume integral routines, including in the analytical specific volume integrals with 4 equations of state. To accommodate these corrections as well as a future expansion of the mass weighting to apply near the surface under ice-shelves or icebergs, the previous optional logical argument (useMassWghtInterp) was replaced with a new optional integer argument (MassWghtInterp) that can be used to encode information about where this weighting is applied as well as whether to fix this bug. This combination of settings (MASS_WEIGHT_IN_PRESSURE_GRADIENT = True and non-Boussinesq) does not seem to be widely used yet (it is not used in the GFDL regression suite), so rather than preserving the old (incorrect) solutions by default, this bug is corrected by default. However, the previous answers can be recovered by setting the new runtime parameter MASS_WEIGHT_IN_PGF_NONBOUS_BUG to true. This parameter that is only used (and logged) for non-Boussinesq cases with MASS_WEIGHT_IN_PRESSURE_GRADIENT set to true. It is intended that this new runtime bug-fix parameter will be obsoleted with an aggressive schedule if it is not needed to recreate any production runs. In addition, there are two new diagnostics, MassWt_u and MassWt_v, that show the fractional (0 to 1) effects of the mass weighting in the pressure gradient forces at the velocity points, along with the new diagnostic subroutines diagnose_mass_weight_Z and diagnose_mass_weight_p that calculate these diagnostics by layer in code that replicates the 13 copies for various equations of state and vertical structures within layers. By default, this commit does change answers in non-Boussinesq cases that use MASS_WEIGHT_IN_PRESSURE_GRADIENT = True, and there is a new runtime parameter in such cases. There are also two new diagnostics. Answers are bitwise identical in all Boussinesq cases.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev/gfdl #692 +/- ##
============================================
- Coverage 36.99% 35.87% -1.13%
============================================
Files 272 269 -3
Lines 82312 81345 -967
Branches 15390 15398 +8
============================================
- Hits 30455 29186 -1269
- Misses 46171 46418 +247
- Partials 5686 5741 +55 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is important for the OM5 NB runs. I don't agree with adding the runtime parameter MASS_WEIGHT_IN_PGF_NONBOUS_BUG. However, since it does no harm other than change doc files, we'll move forward with this, pending tests.
Optionally corrected a bug (essentially a sign error) in the selection of where
MASS_WEIGHT_IN_PRESSURE_GRADIENT
is applied in non-Boussinesq test cases. The (incorrect) non-Boussinesq version of the test for where interfaces are outside of the range of hydrostatic consistency due to interactions with bathymetry was converted from the (correct) Boussinesq version without properly taking into account that pressure increases downward while height increases upward. This bug is repeated 12 times in 6 different specific volume integral routines, including in the analytical specific volume integrals with 4 equations of state.To accommodate these corrections as well as a future expansion of the mass weighting to apply near the surface under ice-shelves or icebergs, the previous optional logical argument (
useMassWghtInterp
) was replaced with a new optional integer argument (MassWghtInterp
) that can be used to encode information about where this weighting is applied as well as whether to fix this bug.This combination of settings (
MASS_WEIGHT_IN_PRESSURE_GRADIENT = True
and non-Boussinesq) does not seem to be widely used yet (it is not used in the GFDL regression suite), so rather than preserving the old (incorrect) solutions by default, this bug is corrected by default. However, the previous answers can be recovered by setting the new runtime parameterMASS_WEIGHT_IN_PGF_NONBOUS_BUG
to true. This parameter that is only used (and logged) for non-Boussinesq cases withMASS_WEIGHT_IN_PRESSURE_GRADIENT
set to true. It is intended that this new runtime bug-fix parameter will be obsoleted with an aggressive schedule if it is not needed to recreate any production runs.In addition, there are two new diagnostics,
MassWt_u
andMassWt_v
, that show the fractional (0 to 1) effects of the mass weighting in the pressure gradient forces at the velocity points, along with the new diagnostic subroutinesdiagnose_mass_weight_Z()
anddiagnose_mass_weight_p()
that calculate these diagnostics by layer in code that replicates the 13 copies for various equations of state and vertical structures within layers.By default, this commit does change answers in non-Boussinesq cases that use
MASS_WEIGHT_IN_PRESSURE_GRADIENT = True
, and there is a new runtime parameter in such cases. There are also two new diagnostics. Answers are bitwise identical in all Boussinesq cases.