Skip to content
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

Merged

Conversation

raphaeldussin
Copy link

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

@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Merging #481 (cefe39f) into dev/gfdl (ddb88f8) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

❗ Current head cefe39f differs from pull request most recent head 8878535. Consider uploading reports for the commit 8878535 to get more accurate results

@@             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     
Files Coverage Δ
...parameterizations/vertical/MOM_diabatic_driver.F90 46.86% <0.00%> (ø)
...meterizations/vertical/MOM_internal_tide_input.F90 0.00% <0.00%> (ø)
...c/parameterizations/lateral/MOM_internal_tides.F90 0.00% <0.00%> (ø)

... 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
@raphaeldussin raphaeldussin force-pushed the internal_tides_split_freqs_modes branch from 3978254 to e40f516 Compare September 15, 2023 17:51
@raphaeldussin raphaeldussin marked this pull request as ready for review September 25, 2023 14:51
@marshallward marshallward self-assigned this Sep 25, 2023
Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a 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.

  1. 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.

  2. 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.

Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a 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().

@marshallward
Copy link
Member

Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/2113 ✔️

Squash-merging.

@marshallward marshallward merged commit 615e57f into NOAA-GFDL:dev/gfdl Oct 28, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants