Skip to content

Fix interior obcs#814

Merged
Hallberg-NOAA merged 2 commits into
NOAA-GFDL:dev/gfdlfrom
ESMG:fix_interior_obcs
Feb 26, 2025
Merged

Fix interior obcs#814
Hallberg-NOAA merged 2 commits into
NOAA-GFDL:dev/gfdlfrom
ESMG:fix_interior_obcs

Conversation

@kshedstrom
Copy link
Copy Markdown

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

Comment thread src/core/MOM_barotropic.F90 Outdated
Comment thread src/core/MOM_barotropic.F90 Outdated
Comment thread src/core/MOM_barotropic.F90 Outdated
Comment thread src/core/MOM_barotropic.F90
Copy link
Copy Markdown
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
@codecov
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Author

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

Copy link
Copy Markdown
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
Copy Markdown
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
kshedstrom added a commit to ESMG/MOM6 that referenced this pull request Mar 17, 2025
kshedstrom added a commit to ESMG/MOM6 that referenced this pull request Mar 17, 2025
* Adding a flag to turn off the bug fix (mostly)

* Fix OBC check if not associated.
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