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

Fix for MASK_OUTSIDE_OBCS with MASKING_DEPTH #752

Open
wants to merge 11 commits into
base: dev/gfdl
Choose a base branch
from
Open

Fix for MASK_OUTSIDE_OBCS with MASKING_DEPTH #752

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6f35d66
Fix for MASK_OUTSIDE_OBCS with MASKING_DEPTH
kshedstrom Oct 30, 2024
c68540e
Fix the tracer advection outside of interior OBCs.
kshedstrom Nov 7, 2024
0692b9b
A more correct fix to tracer advection
kshedstrom Nov 8, 2024
ac162dc
Better fix to eta outside of OBCs issue
kshedstrom Nov 8, 2024
7ee7ad7
More tinkering with OBC_in vs CS%OBC
kshedstrom Nov 21, 2024
0980cb0
Another OBC fix, this for the barotropic mode.
kshedstrom Nov 22, 2024
2682015
More OBC rotation foxes. Still not out of the woods.
kshedstrom Nov 23, 2024
cc26659
Fixing case on tr_Reg.
kshedstrom Nov 28, 2024
dfe0f22
Looks good for the first few steps, then not so much.
kshedstrom Nov 28, 2024
56dc95e
Making barotropic fix look symmetric.
kshedstrom Dec 4, 2024
ec17094
Making barotropic fix look symmetric.
kshedstrom Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions src/core/MOM.F90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2897,6 +2897,26 @@ subroutine initialize_MOM(Time, Time_init, param_file, dirs, CS, &
! reservoirs are used.
call open_boundary_register_restarts(HI, GV, US, CS%OBC, CS%tracer_Reg, &
param_file, restart_CSp, use_temperature)
if (turns /= 0) then
Copy link
Member

Choose a reason for hiding this comment

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

We are getting segmentation faults in the TC testing from code related to the rotated tracer reservoirs, which means that we can not accept this PR because all future PRs will also fail testing.

I suspect that the problem might be that when there are is rotation by 1 or 3 turns, we should have OBC_in%tres_x corresponding to CS%OBC%tres_y and vice versa. However, when there is rotation, these are (grid-space) arrays with different orientations and sizes so one can not simply point to the other. There has to be an explicit copy instead. These are scalars, so the sign will not change.

The specific problem that is leading to a segmentation fault might be something as simple as the fact that the pointers are just not nullified when they are declared on line 372 of MOM_open_boundary.F90, or it could be that by setting a pointer to point to another pointer, the associated test appears to be true, even if the pointer that is pointed to is not itself pointing to allocated data.

The same issues would be true of OBC_in%rx_oblique_u and the other fields, but these are only used for restarts, and I am pretty sure that these fields are never used from within OBC_in, so they might not even matter.

Also, although the OBC type is not opaque, I think that we should treat it as though it were to the extent possible, so if it does still prove necessary to retain this code, I think that this whole block of code should be moved into a subroutine in MOM_open_boundary.F90 that copies these fields from one ocean_OBC_type to another.

if (CS%OBC%radiation_BCs_exist_globally) then
OBC_in%rx_normal => CS%OBC%rx_normal
OBC_in%ry_normal => CS%OBC%ry_normal
endif
if (CS%OBC%oblique_BCs_exist_globally) then
OBC_in%rx_oblique_u => CS%OBC%rx_oblique_u
OBC_in%ry_oblique_u => CS%OBC%ry_oblique_u
OBC_in%rx_oblique_v => CS%OBC%rx_oblique_v
OBC_in%ry_oblique_v => CS%OBC%ry_oblique_v
OBC_in%cff_normal_u => CS%OBC%cff_normal_u
OBC_in%cff_normal_v => CS%OBC%cff_normal_v
endif
if (any(CS%OBC%tracer_x_reservoirs_used)) then
OBC_in%tres_x => CS%OBC%tres_x
endif
if (any(CS%OBC%tracer_y_reservoirs_used)) then
OBC_in%tres_y => CS%OBC%tres_y
endif
endif
endif

if (present(waves_CSp)) then
Expand Down
57 changes: 55 additions & 2 deletions src/core/MOM_barotropic.F90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -737,6 +737,7 @@ subroutine btstep(U_in, V_in, eta_in, dt, bc_accel_u, bc_accel_v, forces, pbce,
integer :: isd, ied, jsd, jed, IsdB, IedB, JsdB, JedB
integer :: ioff, joff
integer :: l_seg
real :: factor(SZI_(G),SZJ_(G)) ! If non-zero, work on given points.

if (.not.CS%module_is_initialized) call MOM_error(FATAL, &
"btstep: Module MOM_barotropic must be initialized before it is used.")
Expand Down Expand Up @@ -2448,17 +2449,69 @@ subroutine btstep(U_in, V_in, eta_in, dt, bc_accel_u, bc_accel_v, forces, pbce,
haloshift=iev-ie, unscale=US%L_to_m**2*GV%H_to_m)
endif

do j=jsv,jev
Copy link
Member

Choose a reason for hiding this comment

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

I see where this is going, but I have 4 specific problems with the changes to MOM_barotropic.F90:

  1. This code setting factor does not change from one barotropic timestep to the next, and it might not even change between calls to btstep if the OBC segments don't change. It should be moved before the barotropic time-stepping loop that starts at line 1815, and perhaps it should be made into an element of barotropic_CS and set in barotropic_init().
  2. Why isn't there a similar change from using CS%IareaT to factor where eta_pred is calculated on lines 1928, 1942 or 1948?
  3. factor is not a very specific name that it does not reflect what this array does. Something like IareaT_OBCmask would reflect what this array contains, even if it is somewhat less pithy than I would like.
  4. The comment describing factor does not indicate that its units are [L-2 ~> m-2].

Copy link
Author

Choose a reason for hiding this comment

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

You have some good points about the factor code. The BT_OBC structure is set up for each call to btstep, so if we don't fix that, it will be hard to move anything to barotropic_init().

do i=isv,iev
factor(i,j) = CS%IareaT(i,j)
enddo
enddo

! Update factor so that nothing changes outside of the OBC (problem for interior OBCs only)
if (associated(OBC)) then ; if (OBC%OBC_pe) then
! do j=jsv,jev
! if (OBC%specified_u_BCs_exist_globally .or. OBC%open_u_BCs_exist_globally) then
! do i=isv,iev-1 ; if (OBC%segnum_u(I,j) /= OBC_NONE) then
! if (OBC%segment(OBC%segnum_u(I,j))%direction == OBC_DIRECTION_E) then
! factor(i+1,j) = 0.0
! elseif (OBC%segment(OBC%segnum_u(I,j))%direction == OBC_DIRECTION_W) then
! factor(i,j) = 0.0
! endif
! endif ; enddo
! endif
do i=isv,iev
if (OBC%specified_u_BCs_exist_globally .or. OBC%open_u_BCs_exist_globally) then
do j=jsv,jev
if (OBC%segnum_u(I-1,j) /= OBC_NONE) then
if (OBC%segment(OBC%segnum_u(I-1,j))%direction == OBC_DIRECTION_E) then
factor(i,j) = 0.0
endif
endif
if (OBC%segnum_u(I,j) /= OBC_NONE) then
if (OBC%segment(OBC%segnum_u(I,j))%direction == OBC_DIRECTION_W) then
factor(i,j) = 0.0
endif
endif
enddo
endif
enddo
do j=jsv,jev
if (OBC%specified_v_BCs_exist_globally .or. OBC%open_v_BCs_exist_globally) then
do i=isv,iev
if (OBC%segnum_v(i,J-1) /= OBC_NONE) then
if (OBC%segment(OBC%segnum_v(i,J-1))%direction == OBC_DIRECTION_N) then
factor(i,j) = 0.0
endif
endif
if (OBC%segnum_v(i,J) /= OBC_NONE) then
if (OBC%segment(OBC%segnum_v(i,J))%direction == OBC_DIRECTION_S) then
factor(i,j) = 0.0
endif
endif
enddo
endif
enddo
endif ; endif

if (integral_BT_cont) then
!$OMP do
do j=jsv,jev ; do i=isv,iev
eta(i,j) = (eta_IC(i,j) + n*eta_src(i,j)) + CS%IareaT(i,j) * &
eta(i,j) = (eta_IC(i,j) + n*eta_src(i,j)) + factor(i,j) * &
((uhbt_int(I-1,j) - uhbt_int(I,j)) + (vhbt_int(i,J-1) - vhbt_int(i,J)))
eta_wtd(i,j) = eta_wtd(i,j) + eta(i,j) * wt_eta(n)
enddo ; enddo
else
!$OMP do
do j=jsv,jev ; do i=isv,iev
eta(i,j) = (eta(i,j) + eta_src(i,j)) + (dtbt * CS%IareaT(i,j)) * &
eta(i,j) = (eta(i,j) + eta_src(i,j)) + (dtbt * factor(i,j)) * &
((uhbt(I-1,j) - uhbt(I,j)) + (vhbt(i,J-1) - vhbt(i,J)))
eta_wtd(i,j) = eta_wtd(i,j) + eta(i,j) * wt_eta(n)
enddo ; enddo
Expand Down
Loading
Loading