negative_binomial_distribution operator() doesn't properly handle special cases k == 0, p == 0, p == 1

Open
#63 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
3/5
Estimated time
1-2 days
Newbie friendliness
45/100
Issue type
Bug
Clarity
Mostly clear
Activity status
Stale
Tech stack
cpp
Domain
backend

Research direction

Start in include/boost/random/negative_binomial_distribution.hpp at the constructor and operator() implementation, then compare its gamma_distribution and poisson_distribution preconditions. Run the example cases from the issue and inspect the linked gamma_distribution requirements. Done means the k == 0, p == 0, and p == 1 cases have defined behavior without violating distribution preconditions.

Written by the indexing model from the issue text.

Description

Currently negative_binomial_distribution constructor requires k >=0 && 0 <= p <= 1, but operator() is implemented as follows:

template<class URNG>
IntType operator()(URNG& urng) const
{
    gamma_distribution<RealType> gamma(_k, (1-_p)/_p);
    poisson_distribution<IntType, RealType> poisson(gamma(urng));
    return poisson(urng);
}

Since gamma_distribution constructor requires both input parameters to be > 0, the code violates preconditions when having _k == 0 or _p == 1; also case _p == 0 leads to invalid results.

  • Case p == 1: Would seem easy to fix by adding if (_p == 1) return 0; given the trivial distribution.
  • Case p == 0: is almost redundant and for example C++ standard [1] and many other definitions [2] [3] require p > 0: p can be zero only if k == 0, otherwise the distribution is not well defined (P(i|k,0) is zero for all i >= 0 when k > 0).
  • Case k == 0: for example C++ standard [1] and many other definitions [2] [3] require k > 0.

Example program

#include <random>
#include <boost/random/negative_binomial_distribution.hpp>

int main()
{
    std::mt19937 randEng;
    boost::random::negative_binomial_distribution<int>(0, 0.5)(randEng); // BOOST_ASSERT() fails in gamma_distribution constructor
    boost::random::negative_binomial_distribution<int>(10, 0)(randEng); // Returns bogus (gamma_distribution gets inf as second parameter)
    boost::random::negative_binomial_distribution<int>(10, 1)(randEng); // BOOST_ASSERT() fails in gamma_distribution constructor
    boost::random::negative_binomial_distribution<int>(0, 0)(randEng); // BOOST_ASSERT() fails in gamma_distribution constructor
    return 0;
}

[1]: Checked from draft N4842 (2019-11-27)
[2] https://se.mathworks.com/help/stats/prob.negativebinomialdistribution.html
[3] http://search.r-project.org/R/library/stats/html/NegBinomial.html

Dominant language
C++
Stars
42
Forks
76
Avg merge
3d 22h
Merged PRs (30d)
6

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from boostorg/random

All issues in boostorg/random

Similar issues

More C++ issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.