Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests for var_value<Matrix> for univariate distributions #2304

Merged
merged 33 commits into from
Feb 4, 2021

Conversation

SteveBronder
Copy link
Collaborator

Summary

This modifies the distribution test suite so that var_value<Matrix> is also tested.

Tests

All the standard distribution tests but using var_value<Matrix>

Side Effects

Nope

Release notes

Tests distributions are var_value<Matrix> compatible

Checklist

  • Math issue How to add static matrix? #1805

  • Copyright holder: Steve Bronder

    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

Copy link
Member

@bbbales2 bbbales2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a skim. There were some value_ofs on stuff that I think are integers

check_finite(function, "Probability parameters", theta_vec[i]);
check_bounded(function, "Probability parameters", theta_vec[i], 0.0, 1.0);
check_finite(function, "Probability parameters", value_of(theta_vec[i]));
check_bounded(function, "Probability parameters", value_of(theta_vec[i]), 0.0, 1.0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

theta_vec.val(i)

@@ -43,7 +43,7 @@ return_type_t<T_theta> poisson_binomial_lpmf(const T_y& y,
check_bounded(function, "Successes variable", y_vec[i], 0,
theta_vec[i].size());
check_finite(function, "Probability parameters", theta_vec[i]);
check_bounded(function, "Probability parameters", theta_vec[i], 0.0, 1.0);
check_bounded(function, "Probability parameters", value_of(theta_vec[i]), 0.0, 1.0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

theta_vec.val(i)

stan/math/prim/prob/bernoulli_logit_glm_lpmf.hpp Outdated Show resolved Hide resolved
@@ -47,7 +47,7 @@ return_type_t<T_prob> bernoulli_logit_lpmf(const T_n& n, const T_prob& theta) {
}
T_n_ref n_ref = n;
T_theta_ref theta_ref = theta;
check_bounded(function, "n", n_ref, 0, 1);
check_bounded(function, "n", value_of(n_ref), 0, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Integer, value_of not necessary

Copy link
Collaborator Author

@SteveBronder SteveBronder Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it guaranteed somewhere to always be an integer or do we only check that at the Stan language level?

Copy link
Member

@bbbales2 bbbales2 Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stan language. These are the signatures:

real bernoulli_logit_lpmf(int, real)
real bernoulli_logit_lpmf(int, vector)
real bernoulli_logit_lpmf(int, row_vector)
real bernoulli_logit_lpmf(int, real[])
real bernoulli_logit_lpmf(int[], real)
real bernoulli_logit_lpmf(int[], vector)
real bernoulli_logit_lpmf(int[], row_vector)
real bernoulli_logit_lpmf(int[], real[])

(from: bin/stanc --dump-stan-math-signatures | grep bernoulli_logit_lpmf)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk, idt there's anything bad about calling value_of here. Idt it would be an error and I don't think it would be a performance thing

@@ -41,8 +41,9 @@ return_type_t<T_prob> bernoulli_lpmf(const T_n& n, const T_prob& theta) {
"Probability parameter", theta);
const T_n_ref n_ref = to_ref(n);
const T_theta_ref theta_ref = to_ref(theta);
check_bounded(function, "n", n_ref, 0, 1);
check_bounded(function, "Probability parameter", theta_ref, 0.0, 1.0);
check_bounded(function, "n", value_of(n_ref), 0, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Integer, value_of not necessary

@@ -15,7 +15,7 @@ inline int hypergeometric_rng(int N, int a, int b, RNG& rng) {
using boost::variate_generator;
using boost::math::hypergeometric_distribution;
static const char* function = "hypergeometric_rng";
check_bounded(function, "Draws parameter", N, 0, a + b);
check_bounded(function, "Draws parameter", value_of(N), 0, a + b);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Integer

@@ -74,7 +74,8 @@ return_type_t<T_x, T_beta, T_cuts> ordered_logistic_glm_lpmf(
T_cuts_ref cuts_ref = cuts;
const auto& cuts_val = value_of_rec(cuts_ref);
const auto& cuts_val_vec = to_ref(as_column_vector_or_scalar(cuts_val));
check_bounded(function, "Vector of dependent variables", y_ref, 1, N_classes);
check_bounded(function, "Vector of dependent variables", value_of(y_ref), 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Integer

@@ -103,7 +103,7 @@ return_type_t<T_loc, T_cut> ordered_logistic_lpmf(const T_y& y,
const auto& lambda_arr = as_array_or_scalar(lambda_ref);
ref_type_t<decltype(value_of(lambda_arr))> lambda_val = value_of(lambda_arr);

check_bounded(function, "Random variable", y_ref, 1, K);
check_bounded(function, "Random variable", value_of(y_ref), 1, K);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Integer

stan/math/prim/prob/poisson_binomial_lcdf.hpp Outdated Show resolved Hide resolved
@@ -43,7 +43,8 @@ return_type_t<T_theta> poisson_binomial_lpmf(const T_y& y,
check_bounded(function, "Successes variable", y_vec[i], 0,
theta_vec[i].size());
check_finite(function, "Probability parameters", theta_vec[i]);
check_bounded(function, "Probability parameters", theta_vec[i], 0.0, 1.0);
check_bounded(function, "Probability parameters", value_of(theta_vec[i]),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

theta_vec.val(i)

const auto& bessel1 = modified_bessel_first_kind(-1, kappa_val);
ops_partials.edge3_.partials_ = cos_mu_minus_y - bessel1 / bessel0;
ops_partials.edge3_.partials_ = cos_mu_minus_y -
modified_bessel_first_kind(-1, kappa_val) / modified_bessel_first_kind(0, kappa_val);
Copy link
Member

@bbbales2 bbbales2 Jan 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I didn't* do this inline, I was getting memory problems with expressions.

Edit: did -> didn't*

@bbbales2 bbbales2 changed the title [WIP] Add tests for var_value<Matrix> for univariate distributions Add tests for var_value<Matrix> for univariate distributions Jan 13, 2021
@stan-buildbot
Copy link
Contributor


Name Old Result New Result Ratio Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan 3.42 3.37 1.01 1.36% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan 0.02 0.02 0.98 -2.23% slower
eight_schools/eight_schools.stan 0.11 0.12 0.94 -6.86% slower
gp_regr/gp_regr.stan 0.15 0.15 1.02 1.91% faster
irt_2pl/irt_2pl.stan 5.2 5.14 1.01 1.2% faster
performance.compilation 91.57 89.63 1.02 2.11% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan 8.64 8.65 1.0 -0.14% slower
pkpd/one_comp_mm_elim_abs.stan 29.2 27.81 1.05 4.74% faster
sir/sir.stan 137.49 135.43 1.02 1.49% faster
gp_regr/gen_gp_data.stan 0.04 0.04 0.99 -0.6% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan 3.04 3.05 1.0 -0.25% slower
pkpd/sim_one_comp_mm_elim_abs.stan 0.38 0.4 0.95 -5.25% slower
arK/arK.stan 2.56 2.54 1.01 0.73% faster
arma/arma.stan 0.61 0.62 0.98 -2.56% slower
garch/garch.stan 0.57 0.57 0.99 -0.55% slower
Mean result: 0.997532618535

Jenkins Console Log
Blue Ocean
Commit hash: dd3f0f8


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
Copy link
Member

Greencheck woooo. We got time to polish this a bit more since we aren't trying to merge varmats this release now.

@@ -68,12 +69,12 @@ return_type_t<T_prob> bernoulli_lpmf(const T_n& n, const T_prob& theta) {
if (sum == N) {
logp += N * log(theta_dbl);
if (!is_constant_all<T_prob>::value) {
ops_partials.edge1_.partials_[0] += N / theta_dbl;
ops_partials.edge1_.partials_[0] += N * inv(theta_dbl);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's up with this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh this happened in a few places, why switch from / to * inv()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm gonna add the static cast to double for the division here inf fvar's divide

@stan-buildbot
Copy link
Contributor


Name Old Result New Result Ratio Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan 3.53 3.45 1.02 2.17% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan 0.02 0.02 1.03 3.31% faster
eight_schools/eight_schools.stan 0.12 0.11 1.06 5.59% faster
gp_regr/gp_regr.stan 0.15 0.15 1.0 -0.29% slower
irt_2pl/irt_2pl.stan 5.22 5.26 0.99 -0.83% slower
performance.compilation 92.11 90.13 1.02 2.15% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan 8.75 8.66 1.01 1.01% faster
pkpd/one_comp_mm_elim_abs.stan 29.09 29.93 0.97 -2.87% slower
sir/sir.stan 130.85 125.2 1.05 4.32% faster
gp_regr/gen_gp_data.stan 0.04 0.04 0.98 -1.78% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan 3.13 3.08 1.02 1.59% faster
pkpd/sim_one_comp_mm_elim_abs.stan 0.4 0.41 0.98 -1.85% slower
arK/arK.stan 1.85 1.85 1.0 -0.07% slower
arma/arma.stan 0.62 0.6 1.03 2.5% faster
garch/garch.stan 0.59 0.6 0.99 -0.9% slower
Mean result: 1.01002559519

Jenkins Console Log
Blue Ocean
Commit hash: 034d009


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

@SteveBronder
Copy link
Collaborator Author

Alright I think with the division thing all that's left to do is sort out the integer value of and we are good. Personally idt it hurts and from the math library it is possible to have a var there

@bbbales2
Copy link
Member

bbbales2 commented Feb 1, 2021

I cleaned out the test changes that didn't need to change and removed the value_ofs (gradients aren't going to work even if we do value_of if someone passes in a var). This pull is all good to me.

@SteveBronder
Copy link
Collaborator Author

All good to me as well! though idt I can approve because I'm the author

@stan-buildbot
Copy link
Contributor


Name Old Result New Result Ratio Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan 3.45 3.41 1.01 1.26% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan 0.02 0.02 1.02 1.98% faster
eight_schools/eight_schools.stan 0.11 0.11 0.99 -0.81% slower
gp_regr/gp_regr.stan 0.16 0.15 1.01 1.32% faster
irt_2pl/irt_2pl.stan 5.18 5.36 0.97 -3.46% slower
performance.compilation 92.74 90.17 1.03 2.78% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan 8.65 8.65 1.0 0.05% faster
pkpd/one_comp_mm_elim_abs.stan 28.28 29.23 0.97 -3.36% slower
sir/sir.stan 123.35 125.13 0.99 -1.44% slower
gp_regr/gen_gp_data.stan 0.04 0.04 1.0 0.24% faster
low_dim_gauss_mix/low_dim_gauss_mix.stan 3.13 3.05 1.03 2.53% faster
pkpd/sim_one_comp_mm_elim_abs.stan 0.37 0.38 0.98 -2.39% slower
arK/arK.stan 1.84 1.85 0.99 -0.85% slower
arma/arma.stan 0.6 0.6 1.0 0.21% faster
garch/garch.stan 0.59 0.59 1.0 0.17% faster
Mean result: 0.999180163139

Jenkins Console Log
Blue Ocean
Commit hash: 46e49f8


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

@stan-buildbot
Copy link
Contributor


Name Old Result New Result Ratio Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan 3.49 3.39 1.03 2.71% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan 0.02 0.02 0.98 -1.94% slower
eight_schools/eight_schools.stan 0.11 0.11 0.98 -2.47% slower
gp_regr/gp_regr.stan 0.15 0.15 0.98 -2.4% slower
irt_2pl/irt_2pl.stan 5.2 5.21 1.0 -0.32% slower
performance.compilation 91.43 88.31 1.04 3.41% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan 8.64 8.73 0.99 -1.04% slower
pkpd/one_comp_mm_elim_abs.stan 29.64 29.09 1.02 1.87% faster
sir/sir.stan 133.52 140.26 0.95 -5.05% slower
gp_regr/gen_gp_data.stan 0.04 0.05 0.92 -8.6% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan 3.09 3.05 1.01 1.23% faster
pkpd/sim_one_comp_mm_elim_abs.stan 0.4 0.38 1.05 5.07% faster
arK/arK.stan 2.55 2.55 1.0 0.22% faster
arma/arma.stan 0.61 0.61 1.0 0.03% faster
garch/garch.stan 0.53 0.53 0.99 -0.81% slower
Mean result: 0.995681017563

Jenkins Console Log
Blue Ocean
Commit hash: 57d4438


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants