Boost.Math.StatisticalDistributions
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 Thanks, John
johneddy101@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.
John Maddock
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
participants (3)
-
John Eddy -
John Maddock -
johneddy101＠comcast.net