Fixing propto tests in probability test framework #1764

bbbales2 · 2020-03-05T23:34:32Z

This solves issue #1763

Tests

This is a bugfix to a function in the test framework. I did not add any tests for this. Do we have tests for the probability test framework?

Release Notes

Fixed propto tests in probability test framework (and fixed bugs this revealed in neg_binomial_lpdf and pareto_type_2_lpdf).

Checklist

Math issue propto tests not testing the correct things in probability unit tests #1763
Copyright holder: Columbia University

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

…f always picking the 0th parameter (Issue #1763)

mcol · 2020-03-06T17:45:21Z

This makes the prop tests for neg_binomial_lpdf, but I see that normal_lpdf fails as well, and perhaps other distributions too. Looking at the normal is much easier, as the distribution is much simpler and there are no numerical cutoffs. Right now I don't see anything wrong there, which may indicate that there's still something wrong in the distribution test, but before reaching that conclusion could you have a look at that too?

mcol · 2020-03-06T18:00:28Z

This is not an exhaustive list, but also cauchy_lpdf, chi_square_lpdf, exponential and gamma_lpdf fail, while bernoulli_lpmf, binomial_lpmf, poisson_lpmf and std_normal_lpdf pass.

bbbales2 · 2020-03-06T19:11:43Z

Good catch. I think the check here: https://github.com/stan-dev/math/blob/develop/test/prob/test_fixture_distr.hpp#L254

Needs changed from:

EXPECT_TRUE(reference_logprob_false - logprob_false
            == reference_logprob_true - logprob_true)

to:

EXPECT_NEAR(value_of(reference_logprob_false - reference_logprob_true),
            value_of(logprob_false - logprob_true), 1e-12)

This just checks to make sure the two numbers are within 1e-12 of each other (absolute tolerance).

With this in place only negative binomial fails.

bob-carpenter · 2020-03-06T19:14:47Z

You might need `value_of_rec` to get down to a`double` value from an arbitrary autodiff variable.

…

On Fri, Mar 6, 2020 at 2:11 PM Ben Bales ***@***.***> wrote: Good catch. I think the check here: https://github.com/stan-dev/math/blob/develop/test/prob/test_fixture_distr.hpp#L254 Needs changed from: EXPECT_TRUE(reference_logprob_false - logprob_false == reference_logprob_true - logprob_true) to: EXPECT_NEAR(value_of(reference_logprob_false - reference_logprob_true), value_of(logprob_false - logprob_true), 1e-12) This just checks to make sure the two numbers are within 1e-12 of each other (absolute tolerance). With this in place only negative binomial fails. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1764?email_source=notifications&email_token=AAZ2D757ZHUEFKCN266SSNTRGFDHDA5CNFSM4LCUTQKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOCPROI#issuecomment-595916985>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZ2D75CSUXMOUSEZZ7CNS3RGFDHDANCNFSM4LCUTQKA> .

mcol · 2020-03-06T19:40:04Z

With this in place only negative binomial fails.

Great! I went back as far as 2.18.1 and the neg_binomial bug was already there, so it's nothing new. I couldn't go further back as I had some compilation errors I didn't think it was worth trying to fix (probably the makefiles for 2.17 were not as clever as what we are used to now).

bbbales2 · 2020-03-06T23:58:42Z

I suspect the negative binomial is something @martinmodrak might know about. He got super deep on these functions a while back. I asked a question over here: #1497 (comment).

martinmodrak · 2020-03-07T16:45:56Z

Yes, I believe the problem might be fixed with the new code, which, unfortunately, got a bit stuck, will try to move it forward a bit.

…diff types (Issue #1763)

…numerics a bit (Issue #1763)

bbbales2 · 2020-05-02T16:58:38Z

@martinmodrak I removed the poisson stuff from neg_binomial in this branch and then updated the numerics a bit. I'm not sure I did it right though. Mind taking a look when you get the chance (no hurry)?

For n = 13 alpha = 1e11 and beta = 1e10, the version of negative_binomial in this branch is giving me:

-2.6185576442208003

R gives me:

-2.618557685656014211

with this code:

n = 13
alpha = 1e11
beta = 1e10
lp = dnbinom(n, mu = alpha / beta, size = alpha, log = TRUE)
sprintf("%.18f", lp)

Develop math was giving:

-2.618380836440565

So we improved but I'm not sure it's fixed yet.

…ests

…to neg_binomial (Issue #1763)

…4.1 (tags/RELEASE_600/final)

bbbales2 · 2020-07-17T23:15:21Z

@martinmodrak with these values:

n = 13
alpha = 1.0e11
beta = 1.0e10

Can you run the code with lots of precision in Mathematica and tell me the result:

LogGamma[n + alpha] - LogGamma[alpha] - LogGamma[n + 1] +  alpha * Log[beta / (beta + 1)] - n * Log[beta + 1]

I'm trying to figure out how off the version of neg_binomial I implemented here is.

…tan-dev/math into bugfix/issue-1763-propto-tests

martinmodrak · 2020-07-18T06:02:22Z

@bbbales2 Unfortunately, Mathematica (at least the free version available in Wolfram Cloud) refuses to compute this (allowed computation time exceeded).... To test for those big values of alpha and beta, the best I could do was to find analytical solutions for n=0 and n=1 (which then can be computed).

andrjohns · 2020-07-18T11:42:26Z

The raspberry pi has Mathematica freely available, I tested with those values (may have done it wrong, not at all familiar with mathematica) to 20 decimals:

In[1]:= n = 13                                                                          

Out[1]= 13

In[2]:= alpha = 100000000000                                                            

Out[2]= 100000000000

In[3]:= beta  = 10000000000                                                             

Out[3]= 10000000000
                                                                       
In[4]:= N[LogGamma[n + alpha] - LogGamma[alpha] - LogGamma[n + 1] +  alpha * Log[beta /  (beta + 1)] - n * Log[beta + 1],{\[Infinity],20}]                                       

Out[4]= -2.6185576442208289933

bbbales2 · 2020-07-19T19:20:29Z

@martinmodrak @andrjohns thanks!

What we have in this branch is: -2.6185576442208003
What Mathematica gives: -2.6185576442208289933

I'll look into the n = 0 / n = 1 thing. I just wanted a spot check that things weren't totally awry. The big fix was dropping the Poisson like @martinmodrak did previously so I think we're probably better off already.

…ng constants (Issue #1763)

…4.1 (tags/RELEASE_600/final)

…_2 (Issue #1763)

…tan-dev/math into bugfix/issue-1763-propto-tests

…4.1 (tags/RELEASE_600/final)

…tan-dev/math into bugfix/issue-1763-propto-tests

…4.1 (tags/RELEASE_600/final)

bbbales2 · 2020-07-20T16:30:40Z

I added an n = 0 and n = 1 comparison against long double calculations to the tests but I only compared with EXPECT_FLOAT_EQ so it's not really testing for much precision.

The numbers are (printed out to 17 digits):

n = 0:
implementation: -9.9999999995
long double:    -9.9999999995

n = 1
implementation: -7.6974149066059496
long double:    -7.6974149066159543

rok-cesnovar · 2020-07-20T18:32:44Z

Are we targeting this for the release? Given that its a test it probably doesnt matter?

bbbales2 · 2020-07-20T18:38:57Z

@rok-cesnovar it's bugfixes, so I wasn't worried about the feature freeze. If it passes it's ready for review though.

rok-cesnovar · 2020-07-20T18:40:07Z

Oh right. Yeah its a bug fix definitely.

stan-buildbot · 2020-07-20T19:41:55Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	4.15	4.13	1.0	0.48% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	1.01	0.54% faster
eight_schools/eight_schools.stan	0.09	0.09	1.03	2.72% faster
gp_regr/gp_regr.stan	0.19	0.19	1.01	0.77% faster
irt_2pl/irt_2pl.stan	5.32	5.3	1.0	0.23% faster
performance.compilation	87.01	85.88	1.01	1.3% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	8.52	8.53	1.0	-0.03% slower
pkpd/one_comp_mm_elim_abs.stan	26.64	27.88	0.96	-4.67% slower
sir/sir.stan	112.39	116.09	0.97	-3.3% slower
gp_regr/gen_gp_data.stan	0.05	0.05	1.01	0.8% faster
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.29	3.34	0.98	-1.73% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.38	0.38	1.0	-0.17% slower
arK/arK.stan	1.82	1.84	0.99	-0.87% slower
arma/arma.stan	0.68	0.64	1.07	6.7% faster
garch/garch.stan	0.52	0.53	1.0	-0.42% slower
Mean result: 1.00219296502

Jenkins Console Log
Blue Ocean
Commit hash: 283a2b0

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

bbbales2 · 2020-07-20T20:14:38Z

@andrjohns @SteveBronder this is ready to be reviewed.

There's three things going on here:

A fix to the original issue: propto tests not testing the correct things in probability unit tests #1763
Changed how neg_binomial worked to fix a problem this caught (avoids the poisson approximation and uses some new numerics copying from: Fixing negative binomial phi cutoff #1497)
A fix a problem with templating in pareto_type_2 this caught

andrjohns · 2020-07-21T00:52:47Z

I can review this one tonight

andrjohns

Mostly comments on the ordering of some of the calcs, and some basic q's around the tests. Otherwise looks great!

I didn't check the any of the numerics, since I assumed the approach was reviewed in Martin's original PR (plus the tests are passing)

stan/math/prim/prob/neg_binomial_lpmf.hpp

stan/math/prim/prob/pareto_type_2_lpdf.hpp

test/prob/test_fixture_distr.hpp

test/unit/math/mix/prob/neg_binomial_test.cpp

test/unit/math/prim/prob/neg_binomial_log_test.cpp

…tan-dev/math into bugfix/issue-1763-propto-tests

…ges I forgot previously (Issue #1763)

…4.1 (tags/RELEASE_600/final)

stan-buildbot · 2020-07-21T23:58:26Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	4.15	4.15	1.0	0.0% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	1.02	1.6% faster
eight_schools/eight_schools.stan	0.09	0.09	1.0	-0.12% slower
gp_regr/gp_regr.stan	0.2	0.2	1.0	-0.12% slower
irt_2pl/irt_2pl.stan	5.37	5.32	1.01	0.87% faster
performance.compilation	88.54	85.76	1.03	3.14% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	8.58	8.52	1.01	0.73% faster
pkpd/one_comp_mm_elim_abs.stan	27.53	27.1	1.02	1.59% faster
sir/sir.stan	115.06	115.35	1.0	-0.25% slower
gp_regr/gen_gp_data.stan	0.05	0.05	1.01	0.78% faster
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.32	3.29	1.01	1.02% faster
pkpd/sim_one_comp_mm_elim_abs.stan	0.41	0.42	0.98	-2.41% slower
arK/arK.stan	1.84	1.82	1.01	0.9% faster
arma/arma.stan	0.63	0.61	1.03	3.37% faster
garch/garch.stan	0.53	0.52	1.01	1.18% faster
Mean result: 1.00843475832

Jenkins Console Log
Blue Ocean
Commit hash: 55cf763

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

andrjohns

All looks good to me, merge when ready

Changed logic for select_var_param select the nth parameter instead o…

cfcff04

…f always picking the 0th parameter (Issue #1763)

mcol linked an issue Mar 6, 2020 that may be closed by this pull request

propto tests not testing the correct things in probability unit tests #1763

Closed

bbbales2 mentioned this pull request Apr 29, 2020

Reuse intermediate computations in distributions part 2 #1752

Closed

5 tasks

bbbales2 and others added 4 commits May 2, 2020 11:49

Check value_of_rec in results so that propto check works for all auto…

4ffbd3c

…diff types (Issue #1763)

Merge branch 'develop' into bugfix/issue-1763-propto-tests

fa14478

Removed poisson approximation from negative binomial and updated the …

5698804

…numerics a bit (Issue #1763)

[Jenkins] auto-formatting by clang-format version 6.0.0

fc3a47b

bbbales2 and others added 3 commits July 17, 2020 18:28

Merge remote-tracking branch 'origin' into bugfix/issue-1763-propto-t…

d100bc7

…ests

Removed vector check from select_var_param and added gradient checks …

ff6046d

…to neg_binomial (Issue #1763)

[Jenkins] auto-formatting by clang-format version 6.0.0-1ubuntu2~16.0…

2ff7275

…4.1 (tags/RELEASE_600/final)

bbbales2 added 2 commits July 17, 2020 19:42

Use long double in test to avoid precision loss (Issue #1763)

49de095

Merge branch 'bugfix/issue-1763-propto-tests' of https://github.com/s…

e8e373f

…tan-dev/math into bugfix/issue-1763-propto-tests

bbbales2 and others added 6 commits July 19, 2020 15:36

Replaced EXPECT_EQ with EXPECT_NEAR for float comparison of normalizi…

483a3c0

…ng constants (Issue #1763)

[Jenkins] auto-formatting by clang-format version 6.0.0-1ubuntu2~16.0…

ed2b7f2

…4.1 (tags/RELEASE_600/final)

Added missing T_loc arguments to include_summand terms in pareto_type…

8213edf

…_2 (Issue #1763)

Merge branch 'bugfix/issue-1763-propto-tests' of https://github.com/s…

edf1b22

…tan-dev/math into bugfix/issue-1763-propto-tests

[Jenkins] auto-formatting by clang-format version 6.0.0-1ubuntu2~16.0…

ea096c0

…4.1 (tags/RELEASE_600/final)

Added n = 0, n = 1 tests to negative binomial pdf (Issue #1763)

ee2cf8d

bbbales2 and others added 2 commits July 20, 2020 12:23

Merge branch 'bugfix/issue-1763-propto-tests' of https://github.com/s…

fa21fe0

…tan-dev/math into bugfix/issue-1763-propto-tests

[Jenkins] auto-formatting by clang-format version 6.0.0-1ubuntu2~16.0…

283a2b0

…4.1 (tags/RELEASE_600/final)

andrjohns requested changes Jul 21, 2020

View reviewed changes

bbbales2 and others added 4 commits July 21, 2020 11:25

Pulled alpha/beta values into their own containers (Issue #1763)

4e4ad06

Merge branch 'bugfix/issue-1763-propto-tests' of https://github.com/s…

a6c5759

…tan-dev/math into bugfix/issue-1763-propto-tests

Switching back to logs of value_ofs in neg_binomial. Adding some chan…

c1822fc

…ges I forgot previously (Issue #1763)

[Jenkins] auto-formatting by clang-format version 6.0.0-1ubuntu2~16.0…

55cf763

…4.1 (tags/RELEASE_600/final)

andrjohns approved these changes Jul 22, 2020

View reviewed changes

bbbales2 merged commit bf2d9a3 into develop Jul 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing propto tests in probability test framework #1764

Fixing propto tests in probability test framework #1764

bbbales2 commented Mar 5, 2020 •

edited

Loading

mcol commented Mar 6, 2020

mcol commented Mar 6, 2020

bbbales2 commented Mar 6, 2020

bob-carpenter commented Mar 6, 2020 via email

mcol commented Mar 6, 2020

bbbales2 commented Mar 6, 2020

martinmodrak commented Mar 7, 2020

bbbales2 commented May 2, 2020

bbbales2 commented Jul 17, 2020

martinmodrak commented Jul 18, 2020

andrjohns commented Jul 18, 2020

bbbales2 commented Jul 19, 2020

bbbales2 commented Jul 20, 2020

rok-cesnovar commented Jul 20, 2020

bbbales2 commented Jul 20, 2020

rok-cesnovar commented Jul 20, 2020

stan-buildbot commented Jul 20, 2020

bbbales2 commented Jul 20, 2020

andrjohns commented Jul 21, 2020

andrjohns left a comment

stan-buildbot commented Jul 21, 2020

andrjohns left a comment

Fixing propto tests in probability test framework #1764

Fixing propto tests in probability test framework #1764

Conversation

bbbales2 commented Mar 5, 2020 • edited Loading

Tests

Release Notes

Checklist

mcol commented Mar 6, 2020

mcol commented Mar 6, 2020

bbbales2 commented Mar 6, 2020

bob-carpenter commented Mar 6, 2020 via email

mcol commented Mar 6, 2020

bbbales2 commented Mar 6, 2020

martinmodrak commented Mar 7, 2020

bbbales2 commented May 2, 2020

bbbales2 commented Jul 17, 2020

martinmodrak commented Jul 18, 2020

andrjohns commented Jul 18, 2020

bbbales2 commented Jul 19, 2020

bbbales2 commented Jul 20, 2020

rok-cesnovar commented Jul 20, 2020

bbbales2 commented Jul 20, 2020

rok-cesnovar commented Jul 20, 2020

stan-buildbot commented Jul 20, 2020

bbbales2 commented Jul 20, 2020

andrjohns commented Jul 21, 2020

andrjohns left a comment

Choose a reason for hiding this comment

stan-buildbot commented Jul 21, 2020

andrjohns left a comment

Choose a reason for hiding this comment

bbbales2 commented Mar 5, 2020 •

edited

Loading