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

+REMAP_AUX needs at least one more halo update. #496

Merged
merged 7 commits into from
Oct 13, 2023

Conversation

kshedstrom
Copy link

  • Only when REMAP_AUXILIARY_VARS is true.
  • Addresses OBC problem in tangential_vel #495
  • There is still a problem with diffu and diffv when REMAP_AUXILIARY_VARS is true. The first call to horizontal_viscosity after the first REMAP returns with a tile boundary mismatch.

- This one is for CS%u_av, CS%v_av, which need to be updated coming
  into step_MOM_dyn_split_RK2.
@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

Merging #496 (c4cf7f4) into dev/gfdl (c399372) will decrease coverage by 0.01%.
Report is 1 commits behind head on dev/gfdl.
The diff coverage is 31.57%.

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

@@             Coverage Diff              @@
##           dev/gfdl     #496      +/-   ##
============================================
- Coverage     37.82%   37.82%   -0.01%     
============================================
  Files           270      270              
  Lines         78340    78350      +10     
  Branches      14503    14507       +4     
============================================
+ Hits          29636    29638       +2     
- Misses        43300    43307       +7     
- Partials       5404     5405       +1     
Files Coverage Δ
src/core/MOM_barotropic.F90 58.17% <0.00%> (ø)
src/core/MOM_dynamics_split_RK2.F90 64.61% <66.66%> (-0.18%) ⬇️
src/core/MOM_open_boundary.F90 24.39% <0.00%> (-0.05%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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 agree that these halo updates are necessary and correct.

As written, there will be two separate points of synchronization for the two pass vector calls. This could be reduced to a single point of synchronization (and made more efficient) by adding the argument complete=.false. on the first call and (optionally) complete=.true. on the second, or they could be combined into a group_pass.

@kshedstrom
Copy link
Author

Thanks for the comments, @Hallberg-NOAA. I will submit a fix. Meanwhile, I will also attempt a fix to the diffu issue, which is due to needing a halo update on r[xy]_normal after the call to remap_OBC_fields.

- This fixes the Bering ORLANSKI OBCs for differing processor counts.
- This is either the wrong way to do group_pass for OBLIQUE OBC's or
  there is more wrong with them.
- Problem is in tangential_vel at tile boundaries. It matches
  right at the boundary, but needs some halo points to match too.
- Without this, u_av and v_av don't update a wide enough halo to
  get answers to reproduce across different proocessor counts with
  oblique OBCs.
@marshallward
Copy link
Member

@kshedstrom Is this ready to go in? Or did you want to continue working on it?

@kshedstrom
Copy link
Author

You can merge this now. The Bering domain now behaves, but Mehmet and I are seeing other lingering issues with different processor counts for both the North Atlantic and the Arctic. But I think the REMAP_AUXILIARY_VARS and the OBCs should both be good now.

@marshallward
Copy link
Member

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

@marshallward marshallward merged commit 0c491ce into NOAA-GFDL:dev/gfdl Oct 13, 2023
10 checks passed
@kshedstrom kshedstrom deleted the pass_aux_vars branch November 10, 2023 23:44
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