correctness fixes in neg_binomial_* functions

[mcol]

Summary

The various neg_binomial and neg_binomial_2 functions suffer from several cases in which the loops that construct VectorBuilders are executed the wrong number of times (see examples pointed out in #1662 and #1531). Besides that, these functions can be cleaned up a bit so that intermediate computations are stored in VectorBuilders if they are reused later on, as well as using inv and square, and a few other touches here and there.

If it helps, I've split the correctness fixes and the rest of cleanup in two different commits. Fixes #1662, fixes #1531, fixes #595.

Tests

Existing tests should continue to pass. I'm not sure that the current distribution test framework allows to test inputs of different sizes.

Side Effects

None.

Checklist

• Math issue wrong gradient computations in neg_binomial_*cdf functions #1662

• Copyright holder: Marco Colombo

  The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
  - Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
  - Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

• the basic tests are passing

  • unit tests pass (to run, use: ./runTests.py test/unit)
  • header checks pass, (make test-headers)
  • dependencies checks pass, (make test-math-dependencies)
  • docs build, (make doxygen)
  • code passes the built in C++ standards checks (make cpplint)

• the code is written in idiomatic C++ and changes are documented in the doxygen

• the new changes are tested

[bob-carpenter]

The code looks great. This is a lot of work!

I didn't see any new tests. When there are bugs, there should be new tests that fail in the current setup but succeed after the fix.

[mcol]

I've added a bunch of tests (one revealed that one of my patches was incomplete). Ideally this sort of things would be tested with a specific framework or inside the distribution tests, but that would be quite a large undertaking.

These test revealed that for inputs of different lengths:

• neg_binomial_cdf, neg_binomial_lcdf, neg_binomial_lccdf, neg_binomial_2_cdf and neg_binomial_2_lccdf had buggy derivatives
• neg_binomial_lpmf, neg_binomial_2_lpmf, neg_binomial_2_lcdf and neg_binomial_2_log_lpmf were fine

The issues reported in #1531, albeit very confusing, were actually benign, as the variables that appeared to be reassigned in the for loop, were actually local variables within the loop, so they had no negative impact on the rest of the function. Those got cleaned up anyway by earlier patches.

[mcol]

I'm getting a failure in the neg_binomial_2_lcdf distribution tests:

unknown file: Failure
C++ exception with description "beta_lcdf: Second shape parameter[1] is -2.14748e+09, but must be > 0!" thrown in the test body.
[  FAILED  ] AgradCdfLogNegBinomial2_ffv_80/AgradCdfLogTestFixture/0.UpperBound, where TypeParam = boost::mpl::vector<AgradCdfLogNegBinomial2, boost::mpl::vector<Eigen::Matrix<int, -1, 1, 0, -1, 1>, Eigen::Matrix<stan::math::fvar<stan::math::fvar<stan::math::var> >, -1, 1, 0, -1, 1>, Eigen::Matrix<stan::math::fvar<stan::math::fvar<stan::math::var> >, -1, 1, 0, -1, 1>, empty, empty, empty, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na> (0 ms)

The only change I had in that file was replacing a 1.0 with a 1. Locally, switching it back makes the test pass. I don't understand why this would be a problem, but I'm reverting that optional change and hope tests will pass.

[bob-carpenter]

As long as that's not an argument, you should be OK. If it succeeds with 1.0 as an argument to the lcdf but not 1, that's a bug.

…

On Feb 4, 2020, at 11:33 AM, Marco Colombo ***@***.***> wrote: I'm getting a failure in the neg_binomial_2_lcdf distribution tests: unknown file: Failure C++ exception with description "beta_lcdf: Second shape parameter[1] is -2.14748e+09, but must be > 0!" thrown in the test body. [ FAILED ] AgradCdfLogNegBinomial2_ffv_80/AgradCdfLogTestFixture/0.UpperBound, where TypeParam = boost::mpl::vector<AgradCdfLogNegBinomial2, boost::mpl::vector<Eigen::Matrix<int, -1, 1, 0, -1, 1>, Eigen::Matrix<stan::math::fvar<stan::math::fvar<stan::math::var> >, -1, 1, 0, -1, 1>, Eigen::Matrix<stan::math::fvar<stan::math::fvar<stan::math::var> >, -1, 1, 0, -1, 1>, empty, empty, empty, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na> (0 ms) The only change I had in that file was replacing a 1.0 with a 1. Locally, switching it back makes the test pass. I don't understand why this would be a problem, but I'm reverting that optional change and hope tests will pass. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[stan-buildbot]

---

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	4.82	4.95	0.97	-2.64% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.99	-1.48% slower
eight_schools/eight_schools.stan	0.09	0.09	1.01	1.33% faster
gp_regr/gp_regr.stan	0.22	0.22	1.01	0.69% faster
irt_2pl/irt_2pl.stan	6.07	6.08	1.0	-0.05% slower
performance.compilation	89.23	87.03	1.03	2.46% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	7.42	7.42	1.0	0.02% faster
pkpd/one_comp_mm_elim_abs.stan	20.68	20.9	0.99	-1.11% slower
sir/sir.stan	90.51	90.7	1.0	-0.21% slower
gp_regr/gen_gp_data.stan	0.05	0.05	0.98	-1.86% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	2.99	2.99	1.0	-0.2% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.3	0.32	0.94	-6.78% slower
arK/arK.stan	1.76	1.76	1.0	-0.24% slower
arma/arma.stan	0.8	0.79	1.01	1.25% faster
garch/garch.stan	0.62	0.63	1.0	-0.06% slower
Mean result: 0.994528853344

---

Jenkins Console Log
Blue Ocean
Commit hash: ec1031d

---

Machine information

ProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010

CPU:
Intel(R) Xeon(R) CPU E5-1680 v2 @ 3.00GHz

G++:
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

Clang:
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

[mcol]

It was in an assigmnent to a VectorBuilder, see the last commit.