-
Notifications
You must be signed in to change notification settings - Fork 30
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
Divide-by-zero in cvmix_math_cubic_root_find() #65
Comments
Would it be possible to include a block of code along the lines of
Just prior to Relatedly, another ocean parameterization library I'm working on has a logging module that makes it easy to pass status messages / error codes back to the GCM. While it wouldn't fix your problem, I think having something like that (and being able to trace the calls that are resulting in the divide-by-zero, in this case perhaps getting I'll try to find some time this weekend or next to work on this, but if you guys make any progress at GFDL please keep me in the loop! |
Hi Michael, slope = 0.000000000000000E+000 |
Michael, would you accept a temporary solution to this problem whereby we do newton's method on the cubic/quadratic and if that fails we fall back to doing it on a linear function? This will involve adding a return value to cvmix_math_cubic_root_find(). It has the advantage of being quick/easy to implement so I can close the MOM6 issue. We can then come up with a more robust solution later (perhaps using brent or similar). If you're happy with this then I'll send a pull request with the changes. |
Falling back to a linear function sounds fine, although adjusting your initial guess might be a good option as well. For this particular case, our initial guess of Also, it might be worth investigating why the initial guess is a point where the slope is 0. I'm a little surprised (but I suppose it could be a coincidence), so it would be nice to see |
depth(k-1:k+1): -2.05240021240362 -2.05240021250362 |
…t guess if this is the case. Closes CVMix#65.
This is a case where the column has some vanished layers in the interior. The block that needs to catch this situation is: if (k.eq.size(Ri_bulk)) then
OBL_depth = abs(OBL_limit)
elseif (k.eq.0) then
OBL_depth = abs(zt_cntr(1))
else
if (k.eq.1) then
call cvmix_math_poly_interp(coeffs, CVmix_kpp_params_in%interp_type, &
depth(k:k+1), Ri_bulk(k:k+1))
else
call cvmix_math_poly_interp(coeffs, CVmix_kpp_params_in%interp_type, &
depth(k:k+1), Ri_bulk(k:k+1), depth(k-1), &
Ri_bulk(k-1))
end if
coeffs(1) = coeffs(1)-CVmix_kpp_params_in%ri_crit
OBL_depth = -cvmix_math_cubic_root_find(coeffs, 0.5_cvmix_r8 * &
(depth(k)+depth(k+1)))
.
.
.
end if I think something like this would avoid calling the solver with constant depths: if ( abs( (depth(k+1)-depth(k) ) > 0. ) then ! This could use a tolerance rather than 0., to be robust
OBL_depth = -cvmix_math_cubic_root_find(coeffs, 0.5_cvmix_r8 * &
(depth(k)+depth(k+1)))
else
! Ri_bulk crossed the Ri_crit between k and k+1 but the thickness is vanished
OBL_depth = 0.5_cvmix_r8 * (depth(k)+depth(k+1))
endif |
Yeah, I was going to say if
and it's computing a 4th constraint that is probably
I'm not familiar with what you guys are calling "vanishing levels", and wrote most of this code assuming that |
Just to be clear, depth(k) > depth(k+1) does hold, e.g. in this case depth(k-1:k+1) is: -2.05240021240362 However I see that suitability of the interpolation is probably limited in this case! To avoid this the tolerance mentioned in @adcroft code suggestion would need to be dependent on the minimum layer thickness. |
The highlighted digits make all the difference. I guess better than the tolerance variant above would be a normalization of the local coordinate so that if (k.eq.size(Ri_bulk)) then
OBL_depth = abs(OBL_limit)
elseif (k.eq.0) then
OBL_depth = abs(zt_cntr(1))
else
! dstar is the locally normalized coordinate d* = ( d-depth(k-1) ) / ( depth(k-1) - depth(k+1) )
dstar(1) = 0. ! Corresponds to depth(k-1)
dstar(3) = 1. ! Corresponds to depth(k+1)
if (k>1 .and. depth(k-1) - depth(k+1) > 0.) then
dstar(2) = ( depth(k) - depth(k+1) ) / ( depth(k-1) - depth(k+1) ) ! 0. < dstar(2) < 1.
call cvmix_math_poly_interp(coeffs, CVmix_kpp_params_in%interp_type, &
dstar(2:3), Ri_bulk(k:k+1), dstar(1), Ri_bulk(k-1))
else
! For k=1 or vanished separations resort to linear interpolation
dstar(2) = 0.0_cvmix_r8
call cvmix_math_poly_interp(coeffs, CVmix_kpp_params_in%interp_type, &
dstar(2:3), Ri_bulk(k:k+1))
endif
coeffs(1) = coeffs(1)-CVmix_kpp_params_in%ri_crit
OBL_depth = depth( max(1,k-1) ) &
- cvmix_math_cubic_root_find(coeffs, &
0.5_cvmix_r8 * ( dstar(2)+dstar(3) ) ) &
* ( depth( max(1,k-1) ) - depth(k+1) )
.
.
.
end if |
Wow, I totally missed the single digit change! I'll use these values of |
As detailed in issue CVMix#65 there is a problem with our polynomial interpolation / root-finding process if the levels are very close together (GFDL ran into an issue when levels were 1e-10 m thick). This commit adds a new test to try to reproduce that problem and also see if normalizing the depths before interpolating would help avoid the problem.
Okay, I have a pretty basic test in
We expect the root to be between -2.05240021260362 and -2.05240021240362; the normalized depth provides a root in this range while the non-normalized depth does not (we only have 5 sig figs in the latter case!). Intel and NAG show a similar degradation in the precision for non-normalized depth, while PGI [16.5] for some reason is the opposite
Based on these results, though, I definitely support using the normalized-depth option in
to normalize the depth and
to transform back to physical space. |
Hi @mnlevy1981 thanks for this. I'll make a pull request based on the work above. Alternatively, if your solution is already complete let me know. |
Newton's method is encountering a stationary point. Suggest using one of the bracketed algorithms?
See NOAA-GFDL/MOM6#297 .
The text was updated successfully, but these errors were encountered: