-
Notifications
You must be signed in to change notification settings - Fork 235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Recover topography clipping when not specifying MINIMUM_DEPTH #1517
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: