Re: [Boost-users] Boost.Math.StatisticalDistributions

From: John Eddy (johneddy101_at_[hidden])
Date: 2008-03-06 11:40:33

John Maddock <john <at> johnmaddock.co.uk> writes:

> 
> johneddy101 <at> comcast.net wrote:
> >> Hello,
> >> I realize that this library is not yet part of the boost
> >> distribution but I am using it and have come accross an issue.  I am
> >> concerned that the logic in the various check functions may be
> >> flawed.  Take for example the check_uniform function.  The logic is
> >> as follows:
> >> if  lower Ok && upper Ok && lower < upper
> >>     return true
> >> else
> >>     raise error stating that lower must be less than upper
> >>     return false
> >> end if
> >>
> >> The functions used to determine if lower and upper are ok will raise
> >> errors if appropriate.  If the policy is such that raising an error
> >> does not cause a throw, then control will return to the
> >> check_uniform method with an indication that lower or upper is not
> >> ok and we will enter the else block.  Herein lies the problem.
> >> Consider the following scenario: creating a uniform distribution
> >> with:
> >> lower = 0.0
> >> upper = postitive infinity
> >> Check upper will fail and send a message about the infinity.  Enter
> >> the else block.
> >> A message will be raised that states that the lower parameter is not
> >> less than the upper.
> >> This is my concern.  I believe the second message is unnecessary and
> >> potentially incorrect/misleading.  Perhaps logic like shown below
> >> would be more appropriate:
> >> if lower not Ok || upper not Ok then
> >>     return false
> >> else if lower >= upper
> >>     raise error stating that lower must be less than upper
> >>     return false
> >> else
> >>     return true
> >> end if
> 
> Yep, will change as suggested: note however you can only actually observe 
> the domain_error handler being called twice (with different messages) if you 
> are using a user-defined error handler *and* that handler doesn't throw an 
> exception.
> 
> Paul, I think this one was one of yours?  Are we likely to need to give the 
> other distros a once-over to check we don't do this elsewhere as well?
> 
> Thanks for spotting this, John. 
> 

I think giving the others a look is a very good idea.  There are some other 
places where I noticed possible flaws.

- In the check_scale function in common_error_handling, the error reported 
is "scale must be greater than 0" regardless of the problem.  If scale is 
positive infinity for example, this is a bit misleading.  Perhaps separate 
blocks would be appropriate.  A block for the infinite test could report the 
value as infinite and a block for the <= 0 test would give the current report.

Note that the above occurs in many places including check_weibull_shape, 
check_weibull_x, check_alpha and check_beta (beta distribution), etc.

- For the lognormal distribution, the location parameter is the mean correct?  
If that is the case, should there not be a check to see that it is >= 0?  At 
the very least, should there not at least be an isfinite check (using 
check_location for example)?

- For the Bernoulli distribution, is it not the case that 0 < p < 1?  I 
believe you allow 0 <= p <= 1.

- For the exponential distribution, there is no check that lambda is finite.  
Should there be?

- For the extreme value distribution, there is no check that the scale is 
finite or that the location is finite.  Should there be?

I think that similar audits of the other distributions will turn up similar 
issues.

Thanks,
John