-
Notifications
You must be signed in to change notification settings - Fork 62
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
extension to the internal tides module #481
extension to the internal tides module #481
Conversation
Codecov Report
@@ Coverage Diff @@
## dev/gfdl #481 +/- ##
============================================
- Coverage 37.54% 37.50% -0.04%
============================================
Files 270 270
Lines 79030 79111 +81
Branches 14635 14636 +1
============================================
- Hits 29671 29670 -1
- Misses 43892 43975 +83
+ Partials 5467 5466 -1
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
the module in now able to read in tidal velocities for different tidal harmonics and distribute the energy and distribute TKE input over the different vertical modes. This involves upsizing dimensions of several arrays and mofiying some API. internal_tide_input_CS is promoted to public to facilitate the passing of energy input to MOM_internal_tides
3978254
to
e40f516
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.
Thank you for this contribution. I appreciate the direction that this PR is going, but there are two significant issues that I think need to be discussed.
-
The magnitude of the velocity in the quadratic bottom drag term should include all of the velocities added in quadrature, not just a single tidal component, as is the case with the modified code.
-
If at all possible, we should explore other code constructs that avoid making any of our control structures transparent. Opaque structures avoid most of the issues related to efficiency and readability that arise from passing around what are effectively common blocks.
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 these change, the concerns that I had with the previous version have been addressed. The only other change I would suggest is that the allocate call for vel_btTide
should be moved outside of get_barotropic_tidal_vel()
so that its pairing with the call to deallocate(vel_btTide)
is more readily evident, and similarly the allocate call for TKE_tidal_input
should be moved out of get_input_TKE()
.
Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/2113 ✔️ Squash-merging. |
the module in now able to read in tidal velocities for different tidal harmonics and distribute the energy and distribute TKE input over the different vertical modes. This involves upsizing dimensions of several arrays and mofiying some API. internal_tide_input_CS is promoted to public to facilitate the passing of energy input to MOM_internal_tides