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 backscatter divide by zero #62

Merged

Conversation

adcroft
Copy link
Member

@adcroft adcroft commented Jan 21, 2022

A bug fix and an associated cleanup:

One question for @Hallberg-NOAA : could we ever encounter a divide by zero indicated by comment at top of fc3e3ac ?

- I division by zero was encountered when using the back-scatter settings
  (negative viscosity) in NeverWorld2. It appears hrat_min(I,J) can be
  zero. Reading the code, it makes sense that hrat_min can be zero.
  The division was previously made conditional in 14971b4 also
  when using backscatter, but then only one part of the denomitor was used
  in the conditional.
- I'm not sure why the backscatter setup is repeatedly hitting these edge
  cases or specific line of code.
- This fix uses the entire denominator in the conditional.
- A previous re-factor for optimization introduced some inconsistent
  capitalization. This made it hard to understand the code, especially
  with some arrays being re-used at different grid locations.
@codecov
Copy link

codecov bot commented Jan 21, 2022

Codecov Report

Merging #62 (d3e8970) into dev/gfdl (9f0018f) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##           dev/gfdl      #62   +/-   ##
=========================================
  Coverage     28.94%   28.94%           
=========================================
  Files           242      242           
  Lines         71553    71553           
=========================================
  Hits          20714    20714           
  Misses        50839    50839           
Impacted Files Coverage Δ
src/parameterizations/lateral/MOM_hor_visc.F90 43.63% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f0018f...d3e8970. Read the comment docs.

@herrwang0
Copy link

Not sure if this is relevant, but I think h_u and h_v could be zero (rather than an Angstrom) over the masked-out region when USE_LAND_MASK is True (which is the default). If so, hrat_min would be zero as well.

h_u(I,j) = 0.5 * (G%mask2dT(i,j)*h(i,j,k) + G%mask2dT(i+1,j)*h(i+1,j,k))

I never checked the actual values of h_u and h_v over the land though, just something that occurred to me when I was going through this piece of code earlier.

@marshallward
Copy link
Member

marshallward commented Jan 24, 2022

Following on @herrwang0's comment, could this also happen here?
Nevermind, I just saw the comment in the code.

if (CS%better_bound_Kh) then
do j=Jsq,Jeq+1 ; do i=Isq,Ieq+1
if (Kh(i,j) >= hrat_min(i,j) * CS%Kh_Max_xx(i,j)) then
visc_bound_rem(i,j) = 0.0
Kh(i,j) = hrat_min(i,j) * CS%Kh_Max_xx(i,j)
else
visc_bound_rem(i,j) = 1.0 - Kh(i,j) / (hrat_min(i,j) * CS%Kh_Max_xx(i,j))
endif
enddo ; enddo
endif

@marshallward
Copy link
Member

marshallward commented Jan 24, 2022

Since the potential error in the other block is denoted in the comments, and since we're getting ready for a merge to main, I will probably just merge this one and we can address it in a later version.

@marshallward marshallward merged commit e63c405 into NOAA-GFDL:dev/gfdl Jan 25, 2022
@Hallberg-NOAA Hallberg-NOAA added answer-changing A change in results (actual or potential) bug Something isn't working labels Feb 1, 2022
@adcroft adcroft deleted the fix-backscatter-divide-by-zero branch May 31, 2022 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
answer-changing A change in results (actual or potential) bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants