Fix problems with higher order gradients in probability test framework by bbbales2 · Pull Request #2042 · stan-dev/math

bbbales2 · 2020-08-26T20:35:40Z

Fixes #2036 and #2037 and #2038

Release notes

Fix problems with higher order gradients in probability test framework

Checklist

Math issue Probability test framework doesn't clean up memory properly #2036, Probability test framework not computing some gradients correctly #2037, High order functions not being tested in test_repeat_as_vector #2038
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

…4.1 (tags/RELEASE_600/final)

…Issue #2037 #2038)

…4.1 (tags/RELEASE_600/final)

…into bugfix/issue-2037

…ASE_601/final)

…into bugfix/issue-2037

…ASE_601/final)

bbbales2 · 2020-08-28T21:36:20Z

stan/math/prim/prob/frechet_lpdf.hpp

                T_partials_return, T_y>
      inv_y(size(y));
  for (size_t i = 0; i < stan::math::size(y); i++) {
-    inv_y[i] = 1.0 / value_of(y_vec[i]);


The problem here was this second value_of (inv_y has already had one value_of).

This is probably unnecesary once my frechet PR is in.

Yeah whichever goes first is fine with me.

Now its in and you have a merge conflict.

bbbales2 · 2020-08-28T21:36:40Z

test/prob/binomial/binomial_cdf_test.hpp


    for (int i = 0; i <= n; i++) {
-      cdf *= binomial_coefficient<double>(N, i)
+      cdf += binomial_coefficient<double>(N, i)


This code is the same as the log cdf code without the log: https://github.com/stan-dev/math/blob/develop/test/prob/binomial/binomial_cdf_log_test.hpp

bbbales2 · 2020-08-28T21:37:08Z

test/prob/gamma/gamma_ccdf_log_test.hpp

-    param[0] = 16.0;  // y
-    param[1] = 3.0;   // alpha
-    param[2] = 3.0;   // beta
+    param[0] = 9.0;  // y


I made the test case easier because the test implementations were struggling numerically. See: #2006

The Stan implementations of the ccdf didn't seem to have this problem.

bbbales2 · 2020-08-28T21:37:20Z

test/prob/gamma/gamma_ccdf_log_test.hpp

+    using stan::math::log1m;

-    return log(gamma_q(alpha, beta * y));
+    return log1m(gamma_p(alpha, beta * y));


Doing log1m(cdf) worked better than log(ccdf) for forward mode. Check here for the lcdf: https://github.com/stan-dev/math/blob/develop/test/prob/gamma/gamma_cdf_log_test.hpp#L84

bbbales2 · 2020-08-28T21:37:40Z

test/prob/gumbel/gumbel_test.hpp

      const T_y& y, const T_loc& mu, const T_scale& beta, const T3&, const T4&,
      const T5&) {
-    return -log(beta) - (y - mu) / beta + exp((mu - y) / beta);
+    return -log(beta) - (y - mu) / beta - exp((mu - y) / beta);


Gumbel pdf here: https://en.wikipedia.org/wiki/Gumbel_distribution

bbbales2 · 2020-08-28T21:37:53Z

test/prob/inv_chi_square/inv_chi_square_ccdf_log_test.hpp

    using stan::math::gamma_p;

-    return log(1.0 - gamma_p(0.5 * nu, 0.5 / y));
+    return log(gamma_p(0.5 * nu, 0.5 / y));


Here's the math version: https://github.com/stan-dev/math/blob/develop/stan/math/prim/prob/inv_chi_square_lccdf.hpp#L92

(this is checked with finite differences so I assume it's the correct one)

…sue #2037)

…into bugfix/issue-2037

… making the tolerances bigger (Issue #2037)

…ASE_601/final)

bbbales2 · 2020-09-01T19:29:33Z

test/prob/normal/normal_cdf_log_test.hpp

-    param[0] = -2;  // y
-    param[1] = 0;   // mu
-    param[2] = 1;   // sigma
+    param[0] = -2;   // y


I changed this test cause we test the gradients with absolute error, and because the gradients here were like 17.1234, an error on the 4th digit past the decimal meant that I would have to have lowered the tolerance to 1e-3 when this has more precision than that.

This error happened with fvar<var> variables. Anyway we should be using relative tolerances where it makes sense and autodiff-mode aware tolerances, but I'm trying to skip that to keep it simple.

Anyway I changed the parameters here and the function works better again.

bbbales2 · 2020-09-02T17:57:49Z

@serban-nicusor-toptal had an out of memory disk space error here on the distribution tests: https://jenkins.mc-stan.org/blue/organizations/jenkins/Math%20Pipeline/detail/PR-2042/15/pipeline/40/

Does that mean anything specific or does it just look like a random failiure?

stan-buildbot · 2020-09-03T02:48:46Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	4.2	4.29	0.98	-2.19% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	1.01	0.8% faster
eight_schools/eight_schools.stan	0.09	0.09	1.01	0.52% faster
gp_regr/gp_regr.stan	0.2	0.2	0.99	-1.18% slower
irt_2pl/irt_2pl.stan	5.29	5.23	1.01	1.09% faster
performance.compilation	90.2	87.37	1.03	3.14% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	8.24	8.2	1.0	0.46% faster
pkpd/one_comp_mm_elim_abs.stan	27.41	29.97	0.91	-9.36% slower
sir/sir.stan	134.64	135.25	1.0	-0.45% slower
gp_regr/gen_gp_data.stan	0.05	0.05	0.98	-1.58% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.42	3.33	1.03	2.74% faster
pkpd/sim_one_comp_mm_elim_abs.stan	0.38	0.4	0.96	-3.83% slower
arK/arK.stan	2.53	2.55	1.0	-0.49% slower
arma/arma.stan	0.73	0.74	0.98	-1.73% slower
garch/garch.stan	0.72	0.73	0.99	-0.66% slower
Mean result: 0.992355182312

Jenkins Console Log
Blue Ocean
Commit hash: aae66bb

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

serban-nicusor-toptal · 2020-09-03T08:56:55Z

@bbbales2 Thanks a lot of letting me know, looks like some machines had a different physical disk name under fdisk -l ( /dev/xvdb vs /dev/nvme1n1 ) so the machine it failed on didn't mount the disk ( 500 GB ) at all and it used the base one ( 20 GB )
I've now fixed the provisioning script to include both cases and it should be fine from now on.

bbbales2 · 2020-09-03T16:26:31Z

@serban-nicusor-toptal I'm glad to ~~here~~ it wasn't just ghosts! Thanks!

Edit: I'm glad to hear* -- my mom is an English teacher she wouldn't like this :(

bbbales2 · 2020-09-03T22:02:50Z

@t4c1 This is ready to review. It's the bugfix part of #1989 . I'm also putting the allocation stuff from #2041 here since that separate branch wasn't passing tests (I don't know why).

There are bunch of little small weird changes. I tried to add explanations inline. Ask if it's not clear why something is okay/what happened.

stan-buildbot · 2020-09-04T03:20:38Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	4.25	4.37	0.97	-2.76% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.96	-4.48% slower
eight_schools/eight_schools.stan	0.09	0.09	0.97	-3.02% slower
gp_regr/gp_regr.stan	0.2	0.2	1.02	2.36% faster
irt_2pl/irt_2pl.stan	5.23	5.33	0.98	-1.96% slower
performance.compilation	90.21	87.55	1.03	2.95% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	8.23	8.22	1.0	0.12% faster
pkpd/one_comp_mm_elim_abs.stan	27.33	28.06	0.97	-2.69% slower
sir/sir.stan	134.44	140.02	0.96	-4.15% slower
gp_regr/gen_gp_data.stan	0.04	0.05	0.99	-0.53% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.38	3.33	1.02	1.55% faster
pkpd/sim_one_comp_mm_elim_abs.stan	0.38	0.39	0.97	-3.61% slower
arK/arK.stan	2.54	2.56	0.99	-0.99% slower
arma/arma.stan	0.74	0.73	1.01	0.94% faster
garch/garch.stan	0.73	0.74	0.99	-1.02% slower
Mean result: 0.989116189237

Jenkins Console Log
Blue Ocean
Commit hash: aae66bb

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

t4c1

I don't see any obvious bugs. However I am hesitant to say this is ok, as I don't know the code. I am also not a mathematician, so there is come confusion regarding math changes.

Maybe it would be better if some more knowledge about prob testing and math reviewed this? Or I can persist, but there will need to be quite some explaining.

t4c1 · 2020-09-04T09:41:48Z

stan/math/prim/prob/frechet_lpdf.hpp

                T_partials_return, T_y>
      inv_y(size(y));
  for (size_t i = 0; i < stan::math::size(y); i++) {
-    inv_y[i] = 1.0 / value_of(y_vec[i]);


This is probably unnecesary once my frechet PR is in.

test/prob/gamma/gamma_ccdf_log_test.hpp

test/prob/normal/normal_cdf_log_test.hpp

test/prob/test_fixture_cdf.hpp

t4c1 · 2020-09-04T10:22:59Z

test/prob/utility.hpp

  for (size_t n = 0; n < parameters.size(); n++)
    if (p < parameters[0].size())
-      param(n) = parameters[n][p];
+      param(n) = get_param<stan::scalar_type_t<T>>(parameters[n], p);


Why/where do you need this change?

This is important for forward mode autodiff when we initialize a vector vs. when we initialize scalars.

If the test scalar type is fvar<var>, then the scalars get initialized with get_param.

This looks like:

param = params[n]; param.d_ = 1.0;

Previously, when initializing vectors, the initialization looked like:

param(n) = parameters[n][p];

I think that leaves the .d_ initialized to zero, which means forward mode autodiff tests fail. Anyway, now we just use the same code to initialize scalars as to initialize each element of a vector so we don't have these problems.

Hmm, do you know why were forward tests passing than before this PR? Actually this PR seems like it fixes quite a number of serious bugs. Any idea why nothing failed before?

I'm not sure, and given the seriousness of the bugs I kinda worry there's more.

Well, I know there's more problems lol. There will be a follow up pull request to this that tests that: cdf([a, b, c, d]) evaluates to the same thing as cdf(a) * cdf(b) * cdf(c) * cdf(d). Lack of those tests caused a bug previously. I also know in the other branch where I put this all together there were problems with discrete_range which didn't show up here. Not sure what the deal is. Maybe adding that other test will uncover them again so I can put the fixes back in?

stan-buildbot · 2020-09-10T01:18:43Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	4.29	4.33	0.99	-1.04% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.99	-1.09% slower
eight_schools/eight_schools.stan	0.09	0.09	0.98	-2.53% slower
gp_regr/gp_regr.stan	0.2	0.2	1.01	0.89% faster
irt_2pl/irt_2pl.stan	5.28	5.27	1.0	0.19% faster
performance.compilation	88.46	87.57	1.01	1.01% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	8.28	8.3	1.0	-0.25% slower
pkpd/one_comp_mm_elim_abs.stan	27.64	27.35	1.01	1.07% faster
sir/sir.stan	140.0	132.25	1.06	5.54% faster
gp_regr/gen_gp_data.stan	0.05	0.05	1.0	-0.19% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.35	3.36	1.0	-0.38% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.38	0.38	1.0	-0.44% slower
arK/arK.stan	2.53	2.56	0.99	-1.14% slower
arma/arma.stan	0.73	0.73	1.0	0.26% faster
garch/garch.stan	0.73	0.73	0.99	-0.52% slower
Mean result: 1.00122053421

Jenkins Console Log
Blue Ocean
Commit hash: 7f8b548

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

t4c1

This looks ok.

bbbales2 and others added 4 commits August 26, 2020 15:55

Fix probability framework allocations (Issue #2036)

522b960

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

26b33a5

…4.1 (tags/RELEASE_600/final)

Make probability unit tests work better with higher order gradients (…

af06de7

…Issue #2037 #2038)

Merge branch 'bugfix/issue-2036' into bugfix/issue-2037

8909206

bbbales2 mentioned this pull request Aug 26, 2020

Add probability tests to check vectorized/scalar lpmfs/lpdfs are the same #1989

Closed

5 tasks

bbbales2 and others added 8 commits August 27, 2020 19:05

Fixed high order gradients in test_repeat_as_vector test (Issue #2038)

80c540b

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

204dd7b

…4.1 (tags/RELEASE_600/final)

Added using statements to avoid pow ambiguities (Issue #2038)

afdd7ce

Merge branch 'bugfix/issue-2037' of https://github.com/stan-dev/math …

7862419

…into bugfix/issue-2037

[Jenkins] auto-formatting by clang-format version 6.0.1-14 (tags/RELE…

97a83bb

…ASE_601/final)

Fixed a few tests and update tolerances (Issue #2037)

bb23473

Merge branch 'bugfix/issue-2037' of https://github.com/stan-dev/math …

a1ffc7c

…into bugfix/issue-2037

[Jenkins] auto-formatting by clang-format version 6.0.1-14 (tags/RELE…

13221ca

…ASE_601/final)

bbbales2 commented Aug 28, 2020

View reviewed changes

bbbales2 and others added 4 commits August 31, 2020 23:56

Loosened tolerances in probability cdf and cdf log gradient tests (Is…

cb78306

…sue #2037)

Merge branch 'bugfix/issue-2037' of https://github.com/stan-dev/math …

24aa71e

…into bugfix/issue-2037

Picked an easier set of parameters for normal cdf log test instead of…

320132e

… making the tolerances bigger (Issue #2037)

[Jenkins] auto-formatting by clang-format version 6.0.1-14 (tags/RELE…

3c42e1f

…ASE_601/final)

bbbales2 commented Sep 1, 2020

View reviewed changes

Merge remote-tracking branch 'origin' into bugfix/issue-2037

aae66bb

bbbales2 changed the base branch from bugfix/issue-2036 to develop September 3, 2020 21:56

bbbales2 mentioned this pull request Sep 3, 2020

Fix probability framework allocations (Issue #2036) #2041

Closed

5 tasks

t4c1 reviewed Sep 4, 2020

View reviewed changes

Merge remote-tracking branch 'origin/develop' into bugfix/issue-2037

7f8b548

t4c1 approved these changes Sep 10, 2020

View reviewed changes

bbbales2 merged commit 430c735 into develop Sep 10, 2020

rok-cesnovar mentioned this pull request Oct 13, 2020

Release of Stan Math 3.4 #2128

Closed

This was referenced Nov 16, 2020

Probability test framework not computing some gradients correctly #2037

Closed

High order functions not being tested in test_repeat_as_vector #2038

Closed

Uh oh!

Comments

Conversation

bbbales2 commented Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release notes

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbbales2 commented Sep 2, 2020

Uh oh!

stan-buildbot commented Sep 3, 2020

Uh oh!

serban-nicusor-toptal commented Sep 3, 2020

Uh oh!

bbbales2 commented Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bbbales2 commented Sep 3, 2020

Uh oh!

stan-buildbot commented Sep 4, 2020

Uh oh!

t4c1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbbales2 Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stan-buildbot commented Sep 10, 2020

Uh oh!

t4c1 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bbbales2 commented Aug 26, 2020 •

edited

Loading

bbbales2 commented Sep 3, 2020 •

edited

Loading

bbbales2 Sep 9, 2020 •

edited

Loading