Fixes for visc_rem in split mode #639
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The "eta has dropped below bathT" warning message has indices off by 1. This is because G%isd_global = G%isd + HI%idg_offset and G%isd is (most of the time) 1. G%isd_global/G%jsd_global are now replaced by G%HI%idg_offset/G%HI%jdg_offset.
Add an option to normalize wt_[uv] in the barotropic solver. This is fix step 1 of 2 to address the issue that visc_rem is applied incorrectly to the barotropic solver. A runtime flag BAROTROPIC_VERTICAL_WEIGHT_FIX is added to control the behavior of this commit. The current default is false (not applying the fix).
Add an option to use `dt` instead of `dt_pred` in the calculation of vertvisc_remnant() at the end of predictor stage. The change affects the following `continuity`() call and `btstep`() call in corrector step. Combined with the changes from the previous commit, the new options should solve the issue of inconsistent barotropic velocities from barotropic solver and baroclinic step. A runtime flag VISC_REM_TIMESTEP_FIX is added to control the behavior of this commit. The current default is false (not applying the fix).
This commit is the third fix related to visc_rem in split mode. h_[uv] from `zonal_flux_thickness`() and `meridional_flux_thickness`() is currently multiplied by `visc_rem_[uv]`. This seems to be double-counting as `visc_rem_[uv]` is accounted for in the barotropic solver vertical weights. Moreover, for options other than BT_cont, the velocity point thickness does not include visc_rem. The fix removes the visc_rem factor from h_[uv] when BT_cont is used. A runtime flag VISC_REM_CONT_HVEL_FIX is added to control the behavior of this commit. The current default is false (not applying the fix). Other small changes: * Write out explicitly all keyword arguments in `continuity`() calls in MOM_dynamics_split_RK2. * Rename a runtime parameter BAROTROPIC_VERTICAL_WEIGHT_FIX from a previous commit to VISC_REM_BT_WEIGHT_FIX, to be consistent with the style of the other two flags. * Add VISC_REM_TIMESTEP_FIX parameter to MOM_dynamics_split_RK2b
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev/gfdl #639 +/- ##
============================================
- Coverage 36.89% 35.85% -1.05%
============================================
Files 271 268 -3
Lines 81627 80183 -1444
Branches 15259 15139 -120
============================================
- Hits 30119 28749 -1370
+ Misses 45861 45750 -111
- Partials 5647 5684 +37 ☔ View full report in Codecov by Sentry. |
herrwang0
changed the title
herrwang0
force-pushed
the
add_visc_rem_patches
branch
from
02cb60f to
af64503
Compare
* Correct a bug using dimensional h_neglect in calculating non-dimensional Iwt_[uv]_tot, by replacing the division with an Adcroft_reciprocal style division. * Add a parameter VISC_REM_BUG to control the defaults of all three viscous remnant bug related parameters. The individual parameters should be removed in the future.
herrwang0
force-pushed
the
add_visc_rem_patches
branch
from
af64503 to
d7489ec
Compare
Hallberg-NOAA
approved these changes
Jun 5, 2024
|
@herrwang0 Did you want me to retain all of these commits? I wasn't able to tell if all of them were valid or not. |
|
@marshallward Please do, they are all valid. |
|
Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/mom6ci/MOM6/-/pipelines/23752 ✔️ 🟡 |
This was referenced Nov 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR aims to solve the problem that
visc_remterm is incorrectly applied in the split mode. There are three issues identified:vertvisc_remnant() call at the end of the predictor stage usesdt_pred, while the calculationsvisc_rem_[uv]is applied to (continuity() andbtstep()) usedt. This results in undercounting (baroclinic) vertical viscous term in the barotropic solver.wt_[uv], a function ofvisc_rem_[uv], is not normalized by the vertically-integrated vertical viscosity. This results in doublecounting (baroclinic) vertical viscous term in the barotropic solver.h_[uv]fromzonal_flux_thickness() andmeridional_flux_thickness() calls are multiplied byvisc_rem_[uv]if present. Currently, BT_cont%h_[uv]is only used in calculatingfrhat[uv], a normalized thickness weight parameter used in a) calculating vertical weights aforementionedwt_[uv], which is already multiplyingvisc_rem_[uv]. b) calculating vertical averagedvisc_rem_[uv], which should just use thickness as weights.Besides, for options other than BT_cont,
frhat[uv]calculation is not based on visc_rem modified thickness.The three newly-added runtime flags
VISC_REM_TIMESTEP_FIX,VISC_REM_BT_WEIGHT_FIX, andVISC_REM_CONT_HVEL_FIX, fix the three issues, respectively.Issue 1 and 2 are directly responsible for mismatching barotropic velocities from the barotropic solver and baroclinic steps.
Issue 3 is more subtle as it has a negligible effect in the test case. But physically it seems that the fix makes more sense.
An idealized test case
The fixes are tested in "flow downslope" style run, but with only 1 layer.
Fig.1 Comparison of velocity from barotropic solver and baroclinic steps
Fig.2 Comparison of unsplit (used as a reference for "truth"), old and fix
As it stands, all flags are defaulted to be False, so no answer changes are expected. Turning them on will obviously change the answer.
Some minor points: