New Distribution Testing Infastructure

[JacobHass8]

Currently, the new helper function, test_invalid_support, only ensures that a domain error is thrown if the cdf, pdf, logcdf, logpdf or quantile is sampled outside the support of the distribution. I think there are still edge cases that need to be addressed. Here are my following thoughts

• How should the cdf/pdf behave when sampled at +/- infinity? The current testing only checks for distributions with finite support. If the support is infinite should I check that the pdf and cdf evaluate properly at +/- infinity?
• The current function relies on distributions having default arguments. Hopefully this is the case and doesn't get me in trouble.
• I'm unsure how to test incorrect construction of a distribution. I think that the best way would be to define something similar to the support function but here we define a vector of pairs that list each parameter's space.

I've deleted some tests that I previously added for the arcsine distribution just to see if I am getting the same code coverage as before (100%).

Here's the code coverage report.

Here's an ongoing list of distributions that I've added generic testing for

• arcsine (100%)
• bernoulli (100%)
• beta (100%)
• binomial (98.51% - don't know how to hit last 3 lines)
• cauchy (100%)
• exponential (100%)
• fisher (95% - missing 8 lines but looks like a bug in codecov)
• gamma (100%)
• geometric (96%)
• hyperexponential (96%)
• hypergeometric (96%)
  • This doesn't throw an error if the input is infinity or nan. This is because the inputs are being converged to an integer type before I can do any bound checks.
• inverse gamma (Need to go back and finish these)
• inverse gaussian (Need to go back and finish these)

[jzmaddock]

Thanks for this,

How should the cdf/pdf behave when sampled at +/- infinity? The current testing only checks for distributions with finite support. If the support is infinite should I check that the pdf and cdf evaluate properly at +/- infinity?

I guess so, and actually we can check the values are correct too I think? PDF should be zero at +-INF and CDF should be zero at -INF and 1 at +INF.

[codecov]

Codecov Report

❌ Patch coverage is 97.53086% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.60%. Comparing base (7a0975e) to head (4ab9190).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
test/test_dist_helpers.hpp	96.87%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1400      +/-   ##
===========================================
- Coverage    95.61%   95.60%   -0.01%     
===========================================
  Files          825      826       +1     
  Lines        68487    68501      +14     
===========================================
+ Hits         65481    65489       +8     
- Misses        3006     3012       +6     

Files with missing lines	Coverage Δ
include/boost/math/distributions/arcsine.hpp	100.00% <100.00%> (ø)
test/test_arcsine.cpp	99.10% <100.00%> (-0.15%)	⬇️
test/test_bernoulli.cpp	100.00% <100.00%> (ø)
test/test_cauchy.cpp	100.00% <100.00%> (ø)
test/test_dist_helpers.hpp	96.87% <96.87%> (ø)

... and 4 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a0975e...4ab9190. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[JacobHass8]

I'm trying to write a function to test bad constructors. My idea was to pass in a vector of vectors where each element is a vector of invalid parameters. However, it's not working as I intended and I have no clue why.

I'm first trying to do this for the arcsine distribution. I began by defining a new constructor from a vector

BOOST_MATH_GPU_ENABLED arcsine_distribution(std::vector<RealType> params) : arcsine_distribution(params[0], params[1]) {}

which just unpacks the vector and calls the original constructor. Then in test_arcsine.hpp, I have

std::vector<std::vector<RealType> > invalid_params = {{1, 0}, // x_min > x_max
                                                      {1, -1}}; 
test_invalid_parameters<arcsine_distribution, RealType>(invalid_params);

The function test_invalid_parameters is defined as

template <template <typename...> typename Distribution, class Real>
void test_invalid_parameters(std::vector<std::vector<Real> > invalid_parameters)
{
    typedef Distribution<Real> dist;

    std::vector<Real> params;
    for (unsigned i=0; i<invalid_parameters.size(); i++)
    {
        params = invalid_parameters[i];
        BOOST_CHECK_THROW(dist(params), std::domain_error); // This should throw an error, but doesn't! 
        BOOST_CHECK_THROW(dist x(params), std::domain_error); // This throws a domain_error
        BOOST_CHECK_THROW(boost::math::pdf(dist(params), 0.5), std::domain_error); // This also throws a domain_error
    }
}

I don't understand why dist(params) doesn't throw an error but dist x(params) does. @jzmaddock do you have any thoughts? I'm still new to c++ and this is getting beyond my understanding.

[jzmaddock]

I don't understand why dist(params) doesn't throw an error but dist x(params) does. @jzmaddock do you have any thoughts? I'm still new to c++ and this is getting beyond my understanding.

That's not clear to me either, I always step through the code in situations like this.

BTW I'd rather not start adding lots of new constructors just for testing purposes, although we could conceivably add a std::initializer_list constructor. Even that gets complicated, because a new public constructor will need careful bounds checking and error handling, which would preclude it being a simple forwarding constructor, which sort of defeats the object of the exercise!

OK how about:

template <class Dist, class Container>
Dist make_distribution(const Container& c)
{
    using value_type = typename Dist::value_type;
    if constexpr (std::is_constructible_v<Dist, value_type, value_type, value_type>)
    {
        if (c.size() >= 3)
            return Dist(c.data()[0], c.data()[1], c.data()[2]);
    }
    if constexpr (std::is_constructible_v<Dist, value_type, value_type>)
    {
        if (c.size() >= 2)
            return Dist(c.data()[0], c.data()[1]);
    }
    if constexpr (std::is_constructible_v<Dist, value_type>)
    {
        if (c.size() >= 1)
            return Dist(c.data()[0]);
    }
}

template <class Dist, class V>
Dist make_distribution(const std::initializer_list<V>& c)
{
    return make_distribution<Dist, std::initializer_list<V>>(c);
}
``

You can call it with a vector, but also an initializer_list, so:

make_distribution< binomial_distribution>({ 1.0, 2.0 })

Throws the expected exception.

[JacobHass8]

I don't understand why dist(params) doesn't throw an error but dist x(params) does. @jzmaddock do you have any thoughts? I'm still new to c++ and this is getting beyond my understanding.

That's not clear to me either, I always step through the code in situations like this.

BTW I'd rather not start adding lots of new constructors just for testing purposes, although we could conceivably add a std::initializer_list constructor. Even that gets complicated, because a new public constructor will need careful bounds checking and error handling, which would preclude it being a simple forwarding constructor, which sort of defeats the object of the exercise!

This is exactly what I was looking for! Thanks for implementing this. I was over complicating things.

[JacobHass8]

Is there a way to only run the codecov report when I push a commit? I'm running all the tests and they don't really need to be run on every commit.

[jzmaddock]

Is there a way to only run the codecov report when I push a commit? I'm running all the tests and they don't really need to be run on every commit.

No not really, not unless you temporarily disable the other runs in the github actions scripts: If you do that, make that one commit in it's own right, and then make sure you revert that commit before you're ready to merge.

[JacobHass8]

I've spent the better part of today trying to make some changes to the gamma distribution and discovered a very weird bug. The definition of the quantile for the gamma distribution is here

template <class RealType, class Policy>
BOOST_MATH_GPU_ENABLED inline RealType quantile(const gamma_distribution<RealType, Policy>& dist, const RealType& p)
{
   BOOST_MATH_STD_USING  // for ADL of std functions

   constexpr auto function = "boost::math::quantile(const gamma_distribution<%1%>&, %1%)";

   RealType shape = dist.shape();
   RealType scale = dist.scale();

   RealType result = 0;
   if(false == detail::check_gamma(function, scale, shape, &result, Policy()))
      return result;
   if(false == detail::check_probability(function, p, &result, Policy()))
      return result;

   if(p == 1)
      return policies::raise_overflow_error<RealType>(function, 0, Policy());

   result = gamma_p_inv(shape, p, Policy()) * scale;

   return result;
}

Note that if p==1, an overflow error should be raised with the default policy. Running the following .cpp file I correctly get an overflow error

int main(){ 
    boost::math::quantile(boost::math::gamma_distribution<double, boost::math::policies::policy<> >(1, 1), static_cast<double>(1));
    return 0;
}

However, when I can't verify this in the testing environment! I reverted all my changes to test_gamma_dist.cpp and just added the following line on line 248

BOOST_MATH_CHECK_THROW(quantile(boost::math::gamma_distribution<RealType>(1, 1), 1), std::overflow_error);

When I run the testing I don't get an error but get this output

====== BEGIN OUTPUT ======
Running 1 test case...
Tolerance for type f is 0.000238419 %
Tolerance for type f is 0.001 %
test_gamma_dist.cpp(248): error: in "test_main": exception std::overflow_error expected but not raised
Tolerance for type d is 5e-12 %
Tolerance for type d is 0.001 %
test_gamma_dist.cpp(248): error: in "test_main": exception std::overflow_error expected but not raised
Tolerance for type e is 5e-12 %
Tolerance for type e is 0.001 %
test_gamma_dist.cpp(248): error: in "test_main": exception std::overflow_error expected but not raised
Tolerance for type N5boost4math8concepts12real_conceptE is 5e-12 %
Tolerance for type N5boost4math8concepts12real_conceptE is 0.001 %
test_gamma_dist.cpp(248): error: in "test_main": exception std::overflow_error expected but not raised

In fact, the output is infinite for numeric types and max_value for real_concept. I'm not sure how this is going wrong. I've tried just about everything and nothing seems to fix the issue (short of just throwing std::overflow_error). The only conclusion I can make is that something is effecting the policy handling for overflow errors in gamma testing. For now, I'm just going to move on since this is an issue with the test framework not the distribution itself.

Edit: It looks like somehow the default policy is set to ignore overflow errors. Not sure how this happened just for the gamma distribution though.

Resolution: Sorry for such a long thread. The workaround I found was to pass overflow_error<throw_on_error>. Then everything works properly.

[JacobHass8]

@jzmaddock I found a couple of oddities when writing tests for the gamma distribution. The pdf always return 0 when x=0. However, the pdf goes to infinity when the shape is less than 1 and is finite for shape=1. It's not a huge deal, but I thought I would point it out.

I also changed the support for the gamma function. I thought this made sense because it's more consistent with previous distributions I've written tests for. It also looks like the inverse_gamma distribution doesn't support the full range from [0, inf) but uses [0, max_value]. Should I change this or leave it alone?

It also looks like "test_out_of_range.hpp" is very similar to what is in "test_dist_helpers.hpp" that we've added here.