bug: beta distribution not correct (failing tests) #717
Comments
hkershaw-brown
changed the title
|
https://github.com/NCAR/DART/tree/beta_distribution_fix note: default values for distribution type NO_DISTRIBUTION, not bounded? |
|
The normal distribution should be unbounded.
…On Thu, Sep 12, 2024 at 1:42 PM Helen Kershaw ***@***.***> wrote:
https://github.com/NCAR/DART/tree/beta_distribution_fix
note: default values for distribution type NO_DISTRIBUTION, not bounded?
https://github.com/NCAR/DART/blob/71d70e1e346e3cc7c0244ad839415104140cfc9d/assimilation_code/modules/assimilation/distribution_params_mod.f90#L10-L18
—
Reply to this email directly, view it on GitHub
<#717 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ANDHUIVEHBGTTKJ64NYALWTZWHVDDAVCNFSM6AAAAABMO4S2W2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBXGEYDENZZHE>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
Yeah I was just wondering if distribution_params_type if it should be initialized to so if it did not get set, it would trap on anything checking for known distributions. |
|
Helen,
Yes, doing that would probably be a safe practice. I have not tried to
implement that or even comprehensively check for other distributions at
this point. Jeff
…On Thu, Sep 12, 2024 at 2:03 PM Helen Kershaw ***@***.***> wrote:
Yeah I was just wondering if distribution_params_type if it should be
initialized to
distribution_type=NOT_A_DISTRIBUTION
so if it did not get set, it would trap on anything checking for known
distributions.
—
Reply to this email directly, view it on GitHub
<#717 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ANDHUITRDF4XPYHFGCFTVJDZWHXPJAVCNFSM6AAAAABMO4S2W2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBXGEZTINJTHE>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
nest the if statement fix on https://github.com/NCAR/DART/tree/stats-fixes This fix only works for gfortran13, not for ifort (IFORT) 2021.10.0 fix: split if statement, the compound logical condition does not get entered and gives a 'Failed to converge for quantile' when quantile==0 Q. Is this a floating point comparison error? (plus short circuting the if statement) -Wcompare-reals (for distribution mods only) |
|
At least some of the "Failed to converge" seems to be the root finding oscillating in inv_cdf flips +/- del_q, del_q old over and over. |
|
reproducer: hkershaw@derecho4:/glade/derecho/scratch/hkershaw/DART/Bugs/beta_dist/DART/developer_tests/normal_dist/work(stats-fixes)$ cat ../test_oscillation.f90
program test_oscillation
use normal_distribution_mod, only : inv_std_normal_cdf
use types_mod, only : r4, r8, digits12
use utilities_mod, only: initialize_utilities, finalize_utilities
implicit none
real(r8) :: quantile
real(r8) :: x
call initialize_utilities()
quantile = 0.99999996152378834_r8
x = inv_std_normal_cdf(quantile)
call finalize_utilities()
end program test_oscillationprintout in normal_distribution_mod.f90 diff --git a/assimilation_code/modules/assimilation/normal_distribution_mod.f90 b/assimilation_code/modules/assimilation/normal_distribution_mod.f90
index f915ad66b..147bf6e62 100644
--- a/assimilation_code/modules/assimilation/normal_distribution_mod.f90
+++ b/assimilation_code/modules/assimilation/normal_distribution_mod.f90
@@ -418,9 +418,11 @@ do iter = 1, max_iterations
! If we've gone too far, the new error will be bigger than the old;
! Repeatedly half the distance until this is rectified
del_q_old = del_q
+ print*, 'del_q_old', del_q_old
q_new = cdf(x_new, p)
do j = 1, max_half_iterations
del_q = q_new - quantile
+ print*, 'del_q ', del_q
if (abs(del_q) < abs(del_q_old)) then
exit
endif
q_old = q_new
x_new = (x_guess + x_new)/2.0_r8
q_new = cdf(x_new, p)
! If q isn't changing, no point in continuing
if(q_old == q_new) exit
end dooutput: |
|
looking at the guesses for an oscillating case: If you do the normcdf in Matlab for both of these you get the same answer. If you do the norminv, you don't get either of them back. >> normcdf(5.37413211153138)
ans =
0.999999961523788
>> normcdf(5.37413211127050)
ans =
0.999999961523788
>> norminv(0.999999961523788)
ans =
5.374132109841061
>> normcdf(5.374132109841061)
ans =
0.999999961523788 |
|
tests are in release v11.8.6 Fix is not released |
|
For completeness, commit message from https://github.com/NCAR/DART/tree/beta_distribution_fix Commit 5b3850f This commit fixes the errors in the beta_distribution_mod.f90 and gamma_distribution_mod.f90. It Beta_distribution_mod.f90: The key change is to remove partial support for generalized beta Functions inv_beta_cdf_params and beta_cdf_params now include an error check to make sure that the Subroutine set_beta_params_from_ens has changed the distribution_params_type to an intent out Subroutine test_beta has been modified to print the mandated ‘PASS’ or ‘FAIL’ as appropriate. gamma_distribution_mod.f90: The key change is to remove partial support for generalized gamma Functions inv_gamma_cdf_params and gamma_cdf_params now include an error check to make sure that the Subroutine set_gamma_params_from_ens has changed the distribution_params_type to an intent out Subroutine test_gamma has been modified to print the mandated ‘PASS’ or ‘FAIL’ as appropriate. normal_distribution_mod.f90: There were no known errors in this module. Several changes were made Functions inv_std_normal_cdf and set_normal_params_from_ens now set the appropriate values for the The magic number definition of the maximum delta in the inv_cdf root searching routine was changed Subroutine test_normal was modified to print the mandated ‘PASS’ or ‘FAIL’ as appropriate. assim_tools_mod.f90: Deprecated arguments in calls to gamma_cdf and inv_gamma_cdf were removed. Note probit_transform_mod.f90: Deprecated arguments were removed from a call to set_gamma_params_from_ens Directories normal_distribution, gamma_distribution, and beta_distribution were created in |
Functions inv_beta_cdf_params and beta_cdf_params now include an error check to make sure that the lower and upper bounds in the distribution_params_type have been set to 0 and 1 as other values are not supported. HK note it might be better to remove this and have pure functions with no side effects. Subroutine set_beta_params_from_ens has changed the distribution_params_type to an intent out argument and defines all six parameters correctly. It also set the parameter type to BETA_DISTRIBUTION. fixes #717
|
Gamma_distribution_mod also changed in 5b3850f to only support supports the standard gamma Q. what does this mean for user options (bounds) set in QCEFF table? Ignored for Gamma and Beta? |
🐛 Your bug may already be reported!
Please search on the issue tracker before creating a new issue.
Describe the bug
max difference should be less than 1e-14
Error Message
[hkershaw:work] (stats-tests) > ./test_beta_dist
Which model(s) are you working with?
none
Version of DART
Which version of DART are you using?
v11.6.0-4-g2feb86983
Have you modified the DART code?
Yes, added test harnesses
https://github.com/NCAR/DART/tree/stats-tests
Build information
Please describe:
The text was updated successfully, but these errors were encountered: