Put CVMix convective viscosity into shear term unless KPP is used #129
Conversation
Codecov Report
@@ Coverage Diff @@
## dev/gfdl #129 +/- ##
=========================================
Coverage 33.45% 33.45%
=========================================
Files 262 262
Lines 71436 71434 -2
Branches 13340 13339 -1
=========================================
Hits 23896 23896
+ Misses 43066 43064 -2
Partials 4474 4474
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
At one point, visc%Kv_shear and visc%Kv_slow were the same array. I don't exactly remember why these were split up, but it may have been to create a diagnostic of the non-shear viscosity, or it may have been that when we added the vertex-point shear mixing, visc%Kv_shear_Bu, we needed to differentiate between the two, or it may have been something else to to do with refactoring the code that sets the diffusivity.
However, there is clearly a problem that this PR is addressing, in that when USE_KPP=False, but USE_CVMix_CONVECTION=True, an unallocated pointer is being passed to calculate_CVMix_conv() in an array argument at lines 756 and 1313 (which this PR corrects), and also at line 1897, which it does not. The proposed code change will change answers, but only in cases where there would have been a segmentation fault, which must imply that no one had ever tried this particular configuration before.
I think that the right answer here is to use visc%Kv_shear in all of the calls to calculate_CVMix_conv(). This PR corrects two such instances, at lines 756 and 1313, but undoes the correct form at line 1311. I would also like to request that the same change be made to line 1897.
After this PR has been accepted, I think that it would be worthwhile for us to revisit how Kv_slow is being handled in other cases where USE_KPP = .false., and perhaps start adding those viscosities subject to the control of a Prandtl number. I would note, however, that initializing visc%Kv_slow(:,:,:) = CS%Kv at about line 312 of MOM_set_diffusivity.F90 may lead to double-counting of this background diffusivity, which is also being used as a floor value in find_coupling_coef, but the would have visc%Kv_shear added to it.
|
Thanks for the reviews @Hallberg-NOAA and @gustavo-marques. It's good to know the context behind this, I was definitely just searching a bit blindly for a change that wouldn't segfault. I'll push a revision shortly. |
In both the legacy and non-legacy diabatic drivers, the viscosity output from CVMix convection went to the `Kv_slow` term. However, this term is only allocated if KPP is being used, leading to a segfault if `USE_KPP = False`. This may have been the intended purpose of `Kv_slow`, but its contribution is already added to the shear term before convection is calculated, so convection can't be accounted using `Kv_slow`. To ensure we don't hit a segfault, and that the enhanced viscosity is actually present, we change convection to go into `Kv_shear` in all cases.
|
This now looks correct to me. Others OK with it? |
|
I am approving this PR, in that the changes that in this PR are correct. However, the conversation around this commit requested an additional similar change at about line 1897 of MOM_diabatic_driver.F90, which was not included in this PR. More broadly, there should be an examination of the role of visc%Kv_slow and visc%Kv_shear, and whether these should continue to exist as separate arrays. |
|
This PR has passed TC testing and pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/15842 . |
Use visc%Kv_shear in the calculate_CVMix_conv call in layered_diabatic, to avoid segmentation faults when USE_REGRIDDING=True and USE_CVMix_CONVECTION=True but USE_KPP=False. This bug was identified when evaluating the changes in NOAA-GFDL#129, and a correction was requested, but the correction was omitted from that PR. Answers could change, but it is likely that any cases that would change would previously have encountered a segmentation fault, so I suspect that no such cases exist.
Use visc%Kv_shear in the calculate_CVMix_conv call in layered_diabatic, to avoid segmentation faults when USE_REGRIDDING=True and USE_CVMix_CONVECTION=True but USE_KPP=False. This bug was identified when evaluating the changes in #129, and a correction was requested, but the correction was omitted from that PR. Answers could change, but it is likely that any cases that would change would previously have encountered a segmentation fault, so I suspect that no such cases exist.
In both the legacy and non-legacy diabatic drivers, the viscosity output from CVMix convection went to the
Kv_slowterm. However, this term is only allocated if KPP is being used, leading to a segfault ifUSE_KPP = False.The
Kv_shearterm is allocated for CVMix convection and/or KPP. In fact, the non-llegacy diabatic driver was checking to see whether KPP is being used, and putting the viscosity into theKv_shearterm if that is the case.It seems as though
Kv_shearshould be the default target for the convective viscosity, andKv_slowshould only be used in the case that KPP is also being used. I'm not entirely sure about the difference between the intended use for these two terms however, so I don't know if this is valid!