-
-
Notifications
You must be signed in to change notification settings - Fork 183
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
Conversation
There was a problem hiding this 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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
theta_vec.val(i)
@@ -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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
)
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Integer
@@ -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]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
theta_vec.val(i)
…4.1 (tags/RELEASE_600/final)
…into feature/varmat-dist-tests
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); |
There was a problem hiding this comment.
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*
…4.1 (tags/RELEASE_600/final)
…into feature/varmat-dist-tests
…4.1 (tags/RELEASE_600/final)
…4.1 (tags/RELEASE_600/final)
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
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 |
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. |
…4.1 (tags/RELEASE_600/final)
…into feature/varmat-dist-tests
All good to me as well! though idt I can approve because I'm the author |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
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>
compatibleChecklist
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
./runTests.py test/unit
)make test-headers
)make test-math-dependencies
)make doxygen
)make cpplint
)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested