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

Fix interior obcs #814

Merged
merged 2 commits into from
Feb 26, 2025
Merged

Fix interior obcs #814

merged 2 commits into from
Feb 26, 2025

Conversation

kshedstrom
Copy link

Just the non-rotational parts of PR #752 for now.

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.

There are a handful of issued related to the use of wide halos in the barotropic solver that will need to be addressed (see the detailed comments), but otherwise this PR is looking good to me.

kshedstrom added a commit to ESMG/MOM6 that referenced this pull request Feb 4, 2025
Copy link

codecov bot commented Feb 4, 2025

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.

Project coverage is 21.91%. Comparing base (bb66d8a) to head (0171c8d).

Files with missing lines Patch % Lines
src/core/MOM_open_boundary.F90 0.00% 8 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (bb66d8a) and HEAD (0171c8d). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (bb66d8a) HEAD (0171c8d)
2 1
Additional details and impacted files
@@              Coverage Diff              @@
##           dev/gfdl     #814       +/-   ##
=============================================
- Coverage     38.15%   21.91%   -16.25%     
=============================================
  Files           296      136      -160     
  Lines         87322    32868    -54454     
  Branches      16303     5847    -10456     
=============================================
- Hits          33316     7202    -26114     
+ Misses        48012    25109    -22903     
+ Partials       5994      557     -5437     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kshedstrom
Copy link
Author

Thanks for pointing out that the array needs wide halos. Hopefully this takes care of the problem - I don't have wide halo tests since they aren't compatible with OBCs.

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.

I am convinced that this revised PR is now correct as written.

The one point about this commit that is still troubling me is that some diagnostics are changing slightly, so this PR is tripping over the automated TC testing. Moreover there does not appear to be way to recover the previous answers. If we were to add a new runtime parameter (like EXTERIOR_OBC_BUG) and a logical bug-fix test (like changing line 4900 of MOM_barotropic.F90 to if (associated(OBC) .and. (CS%exterior_OBC_bug == .false.)) then and something similar at lines 652 and 1056 of MOM_tracer_advect.F90, and perhaps add some logical tests at about lines 5079-5080 of MOM_open_boundary.F90, could we recover the previous answers with the right run-time parameter settings?

If such adding the new runtime parameter did work recover the previous (incorrect?) answers, it would greatly ease my unease about bending the rules for our testing, and I would by very happy to accept this PR without reservations.

@kshedstrom
Copy link
Author

Thank you for the suggestions! I will add the logic to the MASKING_DEPTH code if it fails the tc testing.

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 the recent changes to this PR, all of my previous concerns have been addressed.

@Hallberg-NOAA
Copy link
Member

This PR has passed pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/mom6ci/MOM6/-/pipelines/26527 with the expected warnings about a new runtime parameter.

@Hallberg-NOAA Hallberg-NOAA merged commit 5e4f97b into NOAA-GFDL:dev/gfdl Feb 26, 2025
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.

2 participants