-
Notifications
You must be signed in to change notification settings - Fork 63
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
Fix for using tidal amplitude when determining the BBL thickness #595
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev/gfdl #595 +/- ##
============================================
+ Coverage 37.07% 37.12% +0.04%
============================================
Files 271 271
Lines 80968 80804 -164
Branches 15118 15084 -34
============================================
- Hits 30021 30000 -21
+ Misses 45324 45203 -121
+ Partials 5623 5601 -22 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is a valuable addition that addresses a real bug in the code. I am happy with this change, provided that a comment is added to line 2983 reminding someone looking at the code why the enormous hard-coded value does not actually break dimensional consistency (as was already explained in the description of the PR).
Setting `BBL_USE_TIDAL_BG` in OM5_b003 revealed non-conservation and non-reproducibility across layouts. A scalar variable was correctly being set based on the tidal amplitude but was being used after a break between loops. - Replaced the scalar variable U_bg_sq with a vector u2_bg(:) that is set to either a constant or the square of the tidal amplitude. - The module parameter CS%drag_bg_vel was not set if using the tidal amplitude but WAS being used for conditionals. We now set it to 1e30 because it should NOT be used or impact the solution. Setting it to zero, or anything meaningful would allow code to use it inadvertently. This "constant" does not need to satisfy any dimensional testing.
d71ed45
to
1910a20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the recent minor change, I am now happy to approve this PR.
Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/23134 ✔️ |
Setting
BBL_USE_TIDAL_BG
in OM5_b003 revealed non-conservation and non-reproducibility across layouts. A local scalar variable was correctly being set based on the tidal amplitude but was being re-used after a break between loops.