USE_QG_LEITH_VISC=True uses code with numerous bugs #1590
Comments
|
We aren't using this option to my best knowledge, and I don't think anyone else is either. I would not be bothered if police tape was put up around this code for now. |
VarMix_CS%slope_x was being set with units of [Z L-1 ~> nondim], but described in comments as though it was simply [nondim], and then used in the (apparently unused?) calculate_slopes=.false. branch in calc_slope_functions_using_just_e as though its units actually were [nondim]. This commit corrects this inconsistency, while also rescaling the internal slope variables in that routine to also have the proper units of [Z L-1 ~> nondim]. In so doing, several rescaling factors could be eliminated from the calculations. In addition, the slopes used in calc_QG_Leith_viscosity were also being rescaled with the wrong factor or had dimensionally incorrect tiny values in some denominators, and this has been corrected as well. In testing this rescaling fix, a number of other bugs were identified with USE_QG_LEITH_VISC=True (as described at github.com/mom-ocean/issues/1590), so a fatal error message was added if this option is enabled. All answers in the MOM6-examples test suite are bitwise identical, but the code will now give a fatal error if USE_QG_LEITH_VISC=.true.
|
Thanks for the quick response, @sdbachman. I have a PR pending via dev/gfdl at NOAA-GFDL#314 that includes a fatal error message if anyone tries to use the problematic option. |
|
I have a draft commit that corrects the reproducibility issues when USE_QG_LEITH_VISC=True at Hallberg-NOAA@567f7bd . However, this commit involves sufficiently widespread (albeit minor) changes to public interfaces and the potential to slightly degrade the computational performance of cases that are not using USE_QG_LEITH_VISC=True (because of its expanded halo updates), that I think that further discussion is warranted about how to handle it. |
VarMix_CS%slope_x was being set with units of [Z L-1 ~> nondim], but described in comments as though it was simply [nondim], and then used in the (apparently unused?) calculate_slopes=.false. branch in calc_slope_functions_using_just_e as though its units actually were [nondim]. This commit corrects this inconsistency, while also rescaling the internal slope variables in that routine to also have the proper units of [Z L-1 ~> nondim]. In so doing, several rescaling factors could be eliminated from the calculations. In addition, the slopes used in calc_QG_Leith_viscosity were also being rescaled with the wrong factor or had dimensionally incorrect tiny values in some denominators, and this has been corrected as well. In testing this rescaling fix, a number of other bugs were identified with USE_QG_LEITH_VISC=True (as described at github.com/mom-ocean/issues/1590), so a fatal error message was added if this option is enabled. All answers in the MOM6-examples test suite are bitwise identical, but the code will now give a fatal error if USE_QG_LEITH_VISC=.true.
The QG Leith viscosity option in MOM6 has serious problems that need to be corrected before it is usable.
Specifically, when
USE_QG_LEITH_VISC=True, MOM6 does not reproduce across PE count or layout, and fails the dimensional consistency testing. Moreover, this option is using arrays with stored slopes that are only calculated ifTHICKNESS_DIFFUSE=TrueandUSE_STORED_SLOPES=True, and one ofKHTR_SLOPE_CFF>0orKHTR_SLOPE_CFF>0orMEKE=True. Moreover, these slopes will only be updated after they used for the horizontal viscosity ifTHICKNESSDIFFUSE_FIRST=False, which will break reproducability across restarts and may be completely inconsistent after an ALE vertical remapping step. In other words, this option fails to reproduce across PE count, restarts, or dimensional rescaling, and it may give random numbers during the first time step such that repeated runs of the code do not give the same answers.I have a code fix in hand that corrects the dimensional rescaling issues (without changing answers much), but addressing the other issues may be more of a challenge.
How we deal with these problems may depend on whether anyone is actually using this code. Given all of these problems, I can not imagine that
USE_QG_LEITH_VISC=Trueis not being used in any runs that anyone cares about, and that it was just added as the rough-draft of an idea. If (as I sincerely hope) no one is using this code option, we will have leeway to make the necessary corrections to address all of the issues described above. On the other hand, if someone is using this option, we have a serious problem with wide-ranging consequences.In the mean-time, I am inclined to temporarily add a fatal error if anyone tries to use this code to avoid creating any serious issues by anyone who unwittingly uses this code.
I would appreciate a response from anyone who is using this code so that we can figure out what to do about these problems. In particular, @sdbachman and @gustavo-marques, you contributed or worked on this code (about 4 years ago) and probably are in the best position to know whether this option is actively being used and by whom.
The text was updated successfully, but these errors were encountered: