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 epipycnal pairing array size declarations #1186

Merged

Conversation

Hallberg-NOAA
Copy link
Collaborator

Corrected the size of 4 arrays used to describe layer pairings in
tracer_epipycnal_ML_diff, to avoid the possibility of segmentation faults
when there are very few interior layers compared with the number of mixed and
buffer layers. Also corrected a number of spelling errors in comments. In
runs that previously worked, all answers should be bitwise identical, and they
are identical in all MOM6-examples test cases.

Hallberg-NOAA and others added 2 commits August 21, 2020 16:52
  Corrected the size of 4 arrays used to describe layer pairings in
tracer_epipycnal_ML_diff, to avoid the possibility of segmentation faults
when there are very few interior layers compared with the number of mixed and
buffer layers.  Also corrected a number of spelling errors in comments.  In
runs that previously worked, all answers should be bitwise identical, and they
are identical in all MOM6-examples test cases.
@marshallward marshallward self-assigned this Aug 26, 2020
@marshallward
Copy link
Collaborator

marshallward commented Aug 26, 2020

This appears to resolve #966.

Regression testing on Gaea: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/11074

✔️ No regressions.

@marshallward
Copy link
Collaborator

My last comment was not quite correct. This resolves the error with NK = 6 in tc1 but not NK = 4.

@marshallward
Copy link
Collaborator

If I am reading this right, then it seems that num_srt (which sets the upper bound on iteration) is itself bounded by 2*nkmb, or 2*(nkml + nkbl).

https://github.com/NOAA-GFDL/MOM6/blob/f093750526283c909885c346ccbd8ce9ee524ebe/src/tracer/MOM_tracer_hor_diff.F90#L761-L778

In the case of tc1, 2*(nkml + nkbl) is 8, and I hit the out-of-bound index at 7, which is larger than NK but still within bounds.

These declarations also appear to work correctly:

 685   integer, dimension(2*GV%nk_rho_varies) :: &                                   
 686     kbs_Lp, &   ! The sorted indices of the Left and Right columns for          
 687     kbs_Rp      ! each pairing.                                                 
 688   logical, dimension(2*GV%nk_rho_varies) :: &                                   
 689     left_set, &  ! If true, the left or right point determines the density of   
 690     right_set    ! of the trio.  If densities are exactly equal, both are true. 

Would it be better to use this rather than 2*NK?


Also, the NK = 4 fail is already violating the requirement that NK >= NKML + NKBL + 1 when BULKMIXEDLAYER = .true., so that constraint needs to be checked somewhere. But that can perhaps be addressed separately.

@Hallberg-NOAA
Copy link
Collaborator Author

I am pretty sure the upper limit on num_srt is PEmax_krho, which in turn can be as large as GV%ke. With this, I think that the most restrictive limit on the number pairings in this code is GV%ke+GV%nk_rho_varies, but as this is less than 2*GV%ke and I know for certain that there will be fewer pairings than this, and also because these are only 1-d arrays, it might be OK to leave this as 2*GV%ke

@marshallward
Copy link
Collaborator

Sorry, I misread the second loop, yes you are right. Disregard my comment.

@marshallward marshallward merged commit 9d16493 into mom-ocean:dev/gfdl Aug 26, 2020
@Hallberg-NOAA Hallberg-NOAA deleted the epipycnal_ML_diff_alloc branch July 30, 2021 18:12
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