Recover topography clipping when not specifying MINIMUM_DEPTH #1517
Conversation
PRs #1428 and #1457 extended the topography clipping to allow flooding but missed the use case for positive-only depths where the MASKING_DEPTH parameter alone was in use. There were two bugs: 1. The new code assumed that MINIMUM_DEPTH would be deeper than MASKING_DEPTH (which is intuitive). However, the point of MASKING_DEPTH was only to specify the determination of the land mask. The new code assigned depths the value of MASKING_DEPTH which broke cases that were using MASKING_DEPTH as documented and were leaving MINIMUM_DEPTH=0. 2. The values of variable masking_depth were altered and subsequently not consistent with the logged parameters. A warning was issued but the behavior was nevertheless not as intended. Changes: 1. Removed the test that masking_depth > min_depth, and warning 2. Adjusted the condition and assigned value when clipping depths. This now uses the shallower of min_depth and masking_depth to decide when to clip and for the value to use otherwise. The expression for the land mask is unaltered. 3. Corrected documentation to retain original purpose of MASKING_DEPTH 4. Added some comments for declaration with units 5. Added some clarifying comments in code Todo: - resolve the need for the alternative negative depth pathway associated with the 0.5*min_depth expression.
Codecov Report
@@ Coverage Diff @@
## dev-gfdl-main-candidate-2021-10-04 #1517 +/- ##
===================================================================
Coverage 29.06% 29.06%
===================================================================
Files 237 237
Lines 71643 71637 -6
===================================================================
- Hits 20823 20822 -1
+ Misses 50820 50815 -5
Continue to review full report at Codecov.
|
|
@herrwang0 Needless to say, I need you to test this since we don't have a flooding case. My first attempt at resolving this introduced a new logical parameter but that involved much expanded code. I finally found a way to avoid that. |
|
Thanks for doing this! The min(min_depth, mask_depth) is a very nice way of doing it. I did a quick test and the PR writes the correct topography for my runs. In ocean_geometry, D==-10 is masked and D==-5 is retained. |
- Following feedback from @herrwang0, we have removed the possibility for a user to try using negative depths without the MASKING_DEPTH parameter being set appropriately. This avoids the asymmetric use of MINIMUM_DEPTH that was proposed. A FATAL is now issued. - Corrected a spelling error in a comment. - Removed an unused "use" that should have been done in previous commit.
|
Visual inspection suggests that these changes are correct, and they have conditionally passed the MOM6-examples pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/13779. However, it should be noted that this PR changes some comments in the MOM_parameter_doc files, so those will have to be updated when this updated code is merged into the relevant testing branch. |
PRs #1428 and #1457 extended the topography clipping to allow flooding but missed the use case for positive-only depths where the MASKING_DEPTH parameter alone was in use. There were two bugs:
Changes: