+Refactor set_viscous_BBL and add non-trigonometric find_L_open option #543
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev/gfdl #543 +/- ##
============================================
- Coverage 37.21% 37.16% -0.05%
============================================
Files 271 271
Lines 80472 80655 +183
Branches 15008 15046 +38
============================================
+ Hits 29949 29979 +30
- Misses 44950 45102 +152
- Partials 5573 5574 +1 ☔ View full report in Codecov by Sentry. |
Hallberg-NOAA
force-pushed
the
nontrig_set_viscosity
branch
from
6141eaf
to
0a82457
Compare
There was a problem hiding this comment.
I did my best to crisscross through the branching bits, and to the best of my ability, the new find_L_open_(convex|concave) appear to be equivalent to the existing code.
I did not delve into find_L_open_concave_iterative but the test_L_open* functions and other tests would seem to prove their correctness.
I noted some variables in set_viscous_BBL which appear to no longer be used after the find_L_open_* refactor. I also noted some which are superficially used but can probably be removed.
As for the functions themselves, I generally noted that the arguments could be described in terms of slope, crv, and Vol_open, rather than Dp, Dm, and D_vel. This would prevent recomputation of crv in every function, and perhaps better characterize the calculation.
Several functions use both crv and crv_3, so it would be equally effective to use crv_3 as the input, and compute crv internally. (Replacing crv_3 with crv/3. is not an option, since it risks changing answers.)
I also noted that the if (crv >= 0)/if (crv <= 0) checks are probably redundant, since these are internal functions and their safety can be ensured.
In the future, I think this function probably deserves some additional profiling, but providing an option to avoid the transcendentals is a positive step.
Refactored set_viscous_BBL to separate out the routines setting the open interface lengths used for the channel drag, shortening a 1070 line long routine to 915 lines and reducing the scope of a number of temporary variables. A number of logical branch points have been moved outside of the innermost do loops. This refactoring will also make it easier to provide alternatives to some of the solvers that do not use the trigonometric functions to solve for the roots of a cubic expression and avoiding the issues noted at NOAA-GFDL/issues/483. All answers are bitwise identical and public interfaces are unchanged.
Added the new routine find_L_open_concave_iterative to use iterative Newton's method approaches with appropriate limits to solve the cubic equation for the fractional open face lengths at interfaces that are used by the CHANNEL_DRAG code. These solutions are analogous to those given by the previous expressions that are now in find_L_open_concave_trigonometric, and the two differ at close to roundoff, but the new method is completely independent of the transcendental function library, thereby addressing dev/gfdl MOM6 issue mom-ocean#483. This new routine is called when the new runtime parameter TRIG_CHANNEL_DRAG_WIDTHS is set to false, but by default the previous answers are recovered. By default all answers are bitwise identical, but there is a new runtime parameter in some MOM_parameter_doc files.
Added the new debugging or testing subroutine test_L_open_concave along with extra calls when DEBUG = True that can be used to demonstrate that the iterative solver in find_L_open_concave_iterative is substantially more accurate but mathematically equivalent to the solver in find_L_open_concave_trigonometric. This extra code is only called in debugging mode, and it probably should be deleted in a separate commit after find_L_open_concave_iterative has been accepted onto the dev/gfdl branch of MOM6. All answers are bitwise identical and no output or input is changed.
Carried out minor refactoring in set_viscous_BBL as suggested by the reviews of this PR, including the elimination of some unnecessary error handling and the replacement of C2pi_3 as an argument to find_L_open_concave_trigonometric with an internal parameter. All answers are bitwise identical and there are no changes to publicly visible interfaces.
Hallberg-NOAA
force-pushed
the
nontrig_set_viscosity
branch
from
0a82457
to
f534b0e
Compare
|
I think this is good enough and we can come back to refactoring when this is profiled more rigorously. |
|
Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/22528 ✔️ 🟡 |
The series of 3 commits in this pull request refactors set_viscous_BBL to separate out some of the calculations related to the channel drag parameterizations and added an option to calculate L_open in the concave case without using trigonometric functions, thereby addressing dev/gfdl MOM6 issue #483. This new option in the new subroutine
find_L_open_concave_iterative()is more accurate than the mathematically equivalent trigonometric version, and it is enabled by setting the new runtime parameter TRIG_CHANNEL_DRAG_WIDTHS = False. There is also extensive code that can be used via a debugger to verify that the new option is both correct and more accurate that its predecessor. This debugging code could perhaps be removed in a subsequent commit. By default all answers are bitwise identical, but there is a new runtime parameter in some MOM_parameter_doc files.The commits in this PR include: