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

Out of bounds index (por_face_areaU) related to #3 #24

Closed
wfcooke opened this issue Dec 3, 2021 · 4 comments · Fixed by #32
Closed

Out of bounds index (por_face_areaU) related to #3 #24

wfcooke opened this issue Dec 3, 2021 · 4 comments · Fixed by #32

Comments

@wfcooke
Copy link

wfcooke commented Dec 3, 2021

In testing some updates I got an out of bounds index for

CS%vol_CFL, CS%marginal_faces, OBC, por_face_areaU(:,j,k), visc_rem_u)

Subscript #3 of the array POR_FAC_AREAU has value .....

I'm not seeing a loop for subscript 2 (J) either.

I guess there isn't a test with set_BT_cont=True

line 519 has the same issue.

Will

@marshallward
Copy link
Member

marshallward commented Dec 5, 2021

I agree with @wfcooke that i and j are unset here, and don't match the function definition anyway.

por_face_areau does not even appear to be used in zonal_face_thickness. Maybe it can be removed? (ping @sditkovsky)

Same applies to merid_face_thickness, which is passing full arrays (hence no error) but por_face_areav is also unused in the function.

@Hallberg-NOAA
Copy link
Member

These por_face_area arguments describe fractional horizontal extents, but zonal_face_thickness() and merid_face_thickness() deal only with vertical extents. It is hard to imagine that these por_face_area arguments would every be used in these routines, and I think that they can be safely removed.

@sditkovsky
Copy link

Yes, sorry about this. These arguments can be safely removed. They must be left over from an earlier version of the modifications where I was also modifying the CFL values with the porous factor, but the final version did not include those changes.

@Hallberg-NOAA
Copy link
Member

Hallberg-NOAA commented Dec 7, 2021

Upon further reflection, I think that we actually want to include the faces' fractional open widths in these face thickness estimates. The only use of these thicknesses is as relative weights for the various layers' accelerations in estimating the barotropic accelerations (via the fractional weights frhat[uv] that are set in btcalc(). When visc_rem_[uv] these relative thicknesses are already being modified by a scaling factor in the range of (0,1] to account for the effects of vertical viscosity. I think that we want to the same with por_face_area[UV], but also modify the comments describing the h_u and h_v arguments to btcalc() and the description of the h_u and h_v elements of the BT_cont_type.

Based on this more careful consideration, the right solution for this specific issue with out-of-bounds arrays is not to eliminate these two arguments, but rather to change the calls from call zonal_face_thickness(..., por_face_areaU(:,j,k), visc_rem_u) to call zonal_face_thickness(..., por_face_areaU, visc_rem_u).

For further development of this new porous boundary capability, it might be worth noting that in most of the places where the visc_rem variables are used in MOM, they probably should be (and mostly are) multiplied by a por_face_area variable, although this would change answers when the porous boundaries are enabled. @sditkovsky, since you are probably the only person using this yet, please coordinate with me to figure out how we want to handle this additional change.

@Hallberg-NOAA Hallberg-NOAA linked a pull request Dec 8, 2021 that will close this issue
marshallward pushed a commit that referenced this issue Dec 10, 2021
  Corrected the expressionfs for the por_face_areaU arguments being passed to zonal_face_thickness to
avoid the array out-of-bounds index errors highlighted in MOM6 issue #24. Also
added comments noting where the por_face_area arrays should probably be included
in the effective (relative) face thickness calculations that are later used for
finding the vertically averaged accelerations by the barotropic solver.  All
answers and output are bitwise identical in cases that work, but this should
avoid some run-time or compile-time errors with some compiler settings.
theresa-cordero pushed a commit to theresa-cordero/MOM6 that referenced this issue Feb 25, 2025
…-GFDL#24)

* Sync to dev/gfdl and re-Added generic tracer budget diagnostics

* Fixed trailing space issue
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 a pull request may close this issue.

4 participants