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.

Sign up for GitHub

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

Jump to bottom

Refactoring internal tide reflection #1468

Merged
merged 3 commits into from
Oct 2, 2021
Merged

Refactoring internal tide reflection #1468

merged 3 commits into from
Oct 2, 2021

Conversation

raphaeldussin
Copy link
Contributor

perform trig operations with the discrete angles instead of radians, which simplify code quite a bit.

@raphaeldussin raphaeldussin force-pushed the refactor_int_tide_reflection branch from e71e806 to 9960b92 Compare August 18, 2021 14:46
@codecov
Copy link

codecov bot commented Aug 18, 2021 •
edited
Loading

Codecov Report

Merging #1468 (7019dd5) into dev/gfdl (bb9cb35) will increase coverage by 0.00%.
The diff coverage is 0.00%.

❗ Current head 7019dd5 differs from pull request most recent head 650fe9a. Consider uploading reports for the commit 650fe9a to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##           dev/gfdl    #1468   +/-   ##
=========================================
  Coverage     29.06%   29.06%           
=========================================
  Files           237      237           
  Lines         71645    71642    -3     
=========================================
  Hits          20822    20822           
+ Misses        50823    50820    -3
Impacted Files Coverage Δ
...c/parameterizations/lateral/MOM_internal_tides.F90 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb9cb35...650fe9a. Read the comment docs.

@raphaeldussin raphaeldussin force-pushed the refactor_int_tide_reflection branch from 9960b92 to a2671c3 Compare September 10, 2021 20:21
@raphaeldussin raphaeldussin marked this pull request as ready for review September 25, 2021 02:02
@raphaeldussin raphaeldussin force-pushed the refactor_int_tide_reflection branch from 1548914 to 16128ef Compare September 25, 2021 02:09
Hallberg-NOAA
Hallberg-NOAA reviewed Sep 28, 2021
View reviewed changes
src/parameterizations/lateral/MOM_internal_tides.F90 Outdated
integer :: i, j, a, a_r, na
!integer :: isd, ied, jsd, jed ! start and end local indices on data domain
! ! (values include halos)
integer :: angle_wall ! angle of coast/ridge/shelf wrt equator [rad]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From inspection of the code, it looks to me like angle_wall and angle_wall0, and angle_r and angle_r0 all must the same units (probably [nondim], because as integers I don't see how they could have units of [rad]). Please verify that the documented units of these variables are correct.

It might also be worthwhile to document in a comment that the beams are being reflected into a single discrete angle, and are not splitting into multiple angles to approximate an arbitrary new angle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@marshallward
Copy link
Collaborator

The tc5 that you sent me showed some partial success. Most of the issues seem to mostly be related to diagnostics, but there's a more serious one related to restarts.

PASS: Solutions tc5.grid agree.
PASS: Solutions tc5.layout agree.
PASS: Solutions tc5.dim.t agree.
PASS: Solutions tc5.dim.l agree.
PASS: Solutions tc5.dim.h agree.
PASS: Solutions tc5.dim.z agree.
PASS: Solutions tc5.dim.q agree.
PASS: Solutions tc5.openmp agree.
PASS: Solutions tc5.rotate agree.
PASS: Solutions tc5.nan agree.
PASS: Solutions tc5.dim.r agree.

FAIL: tc5.restart failed at runtime.
FAIL: Diagnostics tc5.grid.diag have changed.
FAIL: Diagnostics tc5.layout.diag have changed.
FAIL: Diagnostics tc5.dim.t.diag have changed.
FAIL: Diagnostics tc5.dim.l.diag have changed.
FAIL: Diagnostics tc5.dim.h.diag have changed.
FAIL: Diagnostics tc5.dim.z.diag have changed.
FAIL: Diagnostics tc5.dim.q.diag have changed.
FAIL: Diagnostics tc5.openmp.diag have changed.
FAIL: Diagnostics tc5.nan.diag have changed.
FAIL: Diagnostics tc5.dim.r.diag have changed.

We don't need to necessarily sort through all of these now, but the dimensional ones ought to be easy enough to fix.

Hallberg-NOAA
Hallberg-NOAA approved these changes Oct 1, 2021
View reviewed changes
Copy link
Collaborator

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

I am now satisfied that these changes are correct, and are moving the code in the right direction, even if the internal tide capability is not yet fully operational and passing all of the tests.

@marshallward
Copy link
Collaborator

Gaea Regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/13701 ✔️

@marshallward marshallward merged commit 503df0e into mom-ocean:dev/gfdl Oct 2, 2021
@marshallward marshallward mentioned this pull request Oct 4, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hallberg-NOAA Hallberg-NOAA Hallberg-NOAA approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@raphaeldussin @marshallward @Hallberg-NOAA