boost::random::negative_binomial_distribution unnecessary restriction to IntType for parameter k

[Maximilian Kleinert <kleinert.max@…>]

Hi.

In the implementation of boost::random::negative_binomial_distribution there is an unnecessary restriction to IntType for parameter k.

A double parameter k will be casted to integer resulting in a different distribution than desired. If one compiles with -Wconversion a warning is given: warning: conversion to ‘int’ from ‘double’ may alter its value [-Wconversion] see attached example_file. This can lead to unwanted realization of a negative binomial distribution for non integer parameters k.

Further there are no assertion checks on the parameters in instantiating a boost::random::negative_binomial_distribution object. These assertions are checked during the sampling process via boost::random::gamma_distribution. There (and in general for negative binomial distribution) the parameter k must be strictly greater than zero and not as mentioned in the documentation "Requires: k >=0".

This can easily be extended by changing IntType to RealType in 5 occurrences regarding the parameter k.

Thank you, Max

P.S.: I compiled the example file with: g++ -Wconversion -o negbin_test -std=c++11 negbin_test.cpp

[Maximilian Kleinert <kleinert.max@…>]

Example of double to int conversion in negative_binomial_distribution

[Steven Watanabe]

I'm on the fence about this. This generalization is reasonable, but it is not consistent with std::negative_binomial_distribution and the difference is subtle enough that it could result in non-obvious bugs if someone were to try replacing boost::random with std.

[Maximilian Kleinert <kleinert.max@…>]

Test paramter assertions

[Maximilian Kleinert <kleinert.max@…>]

Test paramter assertions

[Maximilian Kleinert <kleinert.max@…>]

Comparison of assertion tests

[Maximilian Kleinert <kleinert.max@…>]

Hi Steven,

thanks for your reply. I think that this implicit cast can lead to non-obvious bugs as well, since it changes the distribution in case for non integer parameter k. The standard only describes the negative binomial distribution for trials, e.g. integer parameters. So the behavior for a non integer value is unspecified by the standard. In practice, e.g. claim number modeling, real positive parameters are widely used. To avoid the implicit cast bug it would be possible to delete the constructor for non integer types. Then still no real parameters are usable (in fact they are not proper right now) but the compiler will reject it if someone tries.

Regarding the issue about the assertions. I did some tests comparing std::negative_binomial_distribution and the one from boost. Summarizing, the parameter checks are not very satisfying either, see the attached comparison.txt.

In my opinion it makes no sense that a negative_binomial_distribution with invalid parameters can be constructed and then may or may not fail during the sampling process. Especially in the case k=0 were -2^{31 is produced which can happen if someone calculates with the (in principal) valid parameter k = 0.99.}

I produced these results with g++-4.9.real (Ubuntu 4.9.1-16ubuntu6) 4.9.1 and compiled the attached example with: g++ -D_GLIBCXX_DEBUG -std=c++11 -o test test.cpp and without the _GLIBCXX_DEBUG flag.

It would be great if both implementations of the negative binomial distribution will improve.

Thank you, Max

[Steven Watanabe]

Replying to Maximilian Kleinert <kleinert.max@…>:

The standard only describes the negative binomial distribution for trials, e.g. integer parameters. So the behavior for a non integer value is unspecified by the standard.

Not correct. The standard specifies that it takes an integer parameter. It also states that a floating point number can be converted to an integer. Therefore the behavior when passing a floating point number is specified even if it isn't useful.

[Steven Watanabe]

k = 0 and p = 1 are degenerate cases which should be supported.

[Maximilian Kleinert <kleinert.max@…>]

Replying to steven_watanabe:

Replying to Maximilian Kleinert <kleinert.max@…>:

The standard only describes the negative binomial distribution for trials, e.g. integer parameters. So the behavior for a non integer value is unspecified by the standard.

Not correct. The standard specifies that it takes an integer parameter. It also states that a floating point number can be converted to an integer. Therefore the behavior when passing a floating point number is specified even if it isn't useful.

Ok. So the only possibility would be to add an extra distribution, e.g. Polya distribution, which is essentially the same except that it allows a real positive parameter k?

[Maximilian Kleinert <kleinert.max@…>]

Replying to steven_watanabe:

k = 0 and p = 1 are degenerate cases which should be supported.

The case p = 1 (regardless of k) is not supported by the boost version: The gamma distribution fails because the parameter beta = (1-p)/p is not greater than zero. The standard version fails if compiled with -D_GLIBCXX_DEBUG using g++.

The working draft standard (N4926) page 965 requires: 0 < p <= 1 and 0 < k.

Boost and standard version (except compiled with -D_GLIBCXX_DEBUG) return in the case p = 0 the sample -2147483648 which is not very useful.