Adds support for var<matrix> in scalar_seq_view<> distributions #2214

SteveBronder · 2020-11-26T04:40:41Z

Summary

Adds a .val(i) method to scalar_seq_view<T> and a specialization of scalar_seq_view<> for var<matrix> types. When T has a autodiff value type .val(i) get's back the value from the autodiff type. Else if T is an arithmetic type it just returns that value

Tests

I actually don't know how to test this yet, is there a way to conditionally test this for some of the distributions in the distributions tests? Otherwise I have a separate branch for distributions are vectorized and I need to sort out the last bits of vector_seq_view. Should we wait to turn on var<matrix> tests until all of those are tested?

Side Effects

idt so

Release notes

Adds methods to helpers in probability distributions to support var<matrix>

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

…e it can support var matrices

…4.1 (tags/RELEASE_600/final)

…uence-val-probs

…' into feature/scalar-sequence-val-probs2

…4.1 (tags/RELEASE_600/final)

SteveBronder · 2020-12-01T22:39:00Z

stan/math/prim/meta/as_column_vector_or_scalar.hpp

-inline T&& as_column_vector_or_scalar(T&& a) {
+inline auto as_column_vector_or_scalar(T&& a) {
  return std::forward<T>(a);
 }


@t4c1 I changed this because of what we talked about in the past where passing in an rvalue and returning an rvalue can cause the destructor to go off. Should we just have specializations for doubles where it passes back the correct reference type?

We don't need references for doubles. Although this will always return value (even when it should return a lvalue reference). What you wanted here is decltype(auto). What I would prefer here is T - it will behave exactly the same.

SteveBronder · 2020-12-01T22:40:30Z

stan/math/prim/meta/to_value_array_or_scalar.hpp

+}  // namespace math
+}  // namespace stan
+
+#endif


TODO: Need to write tests for all these new functions (I think we can delete some of them)

Also update docs

…(Issue #2101)

…m/stan-dev/math into feature/scalar-sequence-val-probs

…4.1 (tags/RELEASE_600/final)

bbbales2 · 2020-12-02T14:19:38Z

@SteveBronder got the basic normal lpdf/cdf/lcdf/lccdf tests working. I'm running all the tests now to find out if there are broken distributions.

…2101)

…4.1 (tags/RELEASE_600/final)

t4c1

This PR is getting quite large. Can you split it up? Maybe keep just one distribution in the first one and leave the reminder for another one.

t4c1 · 2020-12-04T10:38:26Z

stan/math/prim/meta/scalar_seq_view.hpp

+   * @param i index
+   * @return the element at the specified position in the container
+   */
+  auto operator[](int i) const { return c_.coeffRef(i); }


Suggested change

auto operator[](int i) const { return c_.coeffRef(i); }

auto operator[](int i) const { return c_.coeff(i); }

Same change for other functions returning a value.

t4c1 · 2020-12-04T10:46:11Z

stan/math/prim/meta/to_value_array_or_scalar.hpp

+template <typename T, require_eigen_t<T>* = nullptr,
+          require_not_plain_type_t<T>* = nullptr>
+inline auto to_value_array_or_scalar(T&& v) {
+  return value_of(v.eval()).array();


Why eval? Also this should use a holder for rvalues (as should other non-scalar overloads of this and other new helper functions).

Actually I think none of these overloads should use eval. If you need eval you should add a separate helper that also evals the result. Or if you dont need non-evaling ones you can skip them, but if some helper also evals it should have eval in its name.

t4c1 · 2020-12-04T10:57:14Z

stan/math/prim/prob/beta_binomial_lpmf.hpp

@@ -159,7 +158,8 @@ return_type_t<T_size1, T_size2> beta_binomial_lpmf(const T_n& n, const T_N& N,
          += digamma_n_plus_alpha[i] + digamma_diff[i] - digamma_alpha[i];
    if (!is_constant_all<T_size2>::value)
      ops_partials.edge2_.partials_[i]
-          += digamma(value_of(N_vec[i] - n_vec[i] + beta_vec[i]))
+          // is this a typo??
+          += digamma(value_of(N_vec[i] - n_vec[i] + beta_vec[i]))  // ??


Obviously invoking value_of on each term separately makes more sense.

t4c1 · 2020-12-04T11:05:53Z

stan/math/prim/prob/gumbel_lpdf.hpp

+                                            + !is_constant_all<T_scale>::value
+                                            + !is_constant_all<T_y>::value
+                                        >= 2>(
+        inv_beta * std::move(exp_y_m_mu_over_beta) - inv_beta);


That move does not do anything.

t4c1 · 2020-12-04T11:06:25Z

stan/math/prim/prob/gumbel_lpdf.hpp

+          = -std::move(y_minus_mu_over_beta) * std::move(scaled_diff)
+            - std::move(inv_beta);


Nor do these.

Yeah my b I think I was doing something silly here when testing this out

t4c1 · 2020-12-04T11:11:25Z

stan/math/prim/prob/normal_id_glm_lpdf.hpp

+  auto&& sigma_val_vec
+      = to_ref(as_column_vector_or_scalar(value_of_rec(sigma_ref)));


if as_column_vector_or_scalar is called woth a lvalue it does not need to construct a holder, so previous version is better.

t4c1 · 2020-12-04T11:11:46Z

stan/math/prim/prob/normal_id_glm_lpdf.hpp

+  auto&& x_val = to_ref_if<!is_constant<T_beta>::value>(value_of_rec(x_ref));
+  auto&& y_val_vec = as_column_vector_or_scalar(value_of_rec(y_ref));
+  auto&& alpha_val_vec = as_column_vector_or_scalar(value_of_rec(alpha_ref));
+  auto&& beta_val_vec = to_ref_if<!is_constant<T_x>::value>(
+      as_column_vector_or_scalar(value_of_rec(beta_ref)));


Do you need these to be non-const for some reason?

t4c1 · 2020-12-04T11:14:34Z

stan/math/prim/prob/pareto_cdf.hpp

-    const T_partials_return alpha_dbl = value_of(alpha_vec[n]);
+    const T_partials_return log_dbl = log(y_min_vec.val(n) / y_vec.val(n));
+    const T_partials_return y_min_inv_dbl = 1.0 / y_vec.val(n);
+    const T_partials_return alpha_dbl = y_vec.val(n);


Suggested change

const T_partials_return alpha_dbl = y_vec.val(n);

const T_partials_return alpha_dbl = alpha_vec.val(n);

bbbales2 · 2020-12-14T17:23:21Z

I haven't looked through this yet. Let me know if I'm holding anything up by not doing that.

SteveBronder · 2020-12-14T22:05:23Z

No idt so, I just have a lot of other stuff on my plate and haven't had time to come back to this

…uence-val-probs

SteveBronder · 2020-12-19T01:12:13Z

@t4c1 I think your right this should be broken up. But the one issue here is that I'd really like to not mess with the distributions test code too much as there's not meta level tests to test it and it's pretty dense/old. I think what would be good here is to break this up into three PRs.

PR for backend stuff like array_or_scalar, seq_view, etc support for var<matrix>
Getting all the LDLT and decomposition stuff working for var<matrix>
A PR that then uses all that stuff in the distributions.

Doing all the distributions at once is brutal but without it we have to tweak the distribution tests to only run for var<matrix> for only the distributions in the successive PRs. Then we'd remove those changes to conditionally run the `var code for the distributions once all those PRs go in. tbh I think both Ben and I tried having a hack at doing the conditional distribution testing and neither of us sorted it out.

bbbales2 · 2020-12-19T01:41:27Z

I think what would be good here is to break this up into three PRs

I like this. Happy to do the big boring review ("A PR that then uses all that stuff in the distributions.")

I'm working on the ldlt/multivariate stuff now.

only run for var for only the distributions in the successive PRs

Yeah, good point, we're writing new test files and we write them for all or none (this file)

bbbales2 · 2021-01-11T13:17:59Z

There's only one big PR left for this right? It should just be the testing framework stuff?

SteveBronder and others added 17 commits November 25, 2020 19:09

replace expr funcs in probs with one function

d86efd2

fix headers

60dcbe0

use auto instead of auto&&

f60c96d

Adds a val(i) method in scalar_sequence_view so distributions that us…

5b5ae40

…e it can support var matrices

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

e3a2663

…4.1 (tags/RELEASE_600/final)

fix for dirichlet with var matrix

49ccb83

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

3353cf0

…4.1 (tags/RELEASE_600/final)

fix template for vector_seq_view<>.val(i)

a95836f

fix template for vector_seq_view<>.val(i)

1e38c29

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

0c52ab0

…4.1 (tags/RELEASE_600/final)

fixup dirichlet and scalar_seq headers

c4e08f5

fixes templates for vector_seq_view

116d61a

Merge remote-tracking branch 'origin/develop' into feature/scalar-seq…

3df142e

…uence-val-probs

Merge remote-tracking branch 'origin/feature/value-column-array-probs…

f002b8d

…' into feature/scalar-sequence-val-probs2

Got rid of var_mat size and added normal var_mat test

7107c81

fix scalar_type_t

df07af9

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

aba8fa4

…4.1 (tags/RELEASE_600/final)

SteveBronder commented Dec 1, 2020

View reviewed changes

bbbales2 and others added 4 commits December 2, 2020 08:59

Updated probability test framework to work with var_value<T> types …

bfbdcca

…(Issue #2101)

Merge branch 'feature/scalar-sequence-val-probs' of https://github.co…

d6ee78a

…m/stan-dev/math into feature/scalar-sequence-val-probs

Merge commit 'ff53436ddff8a156f821f1f8811bb6e8d846f004' into HEAD

70cbe67

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

dd69c18

…4.1 (tags/RELEASE_600/final)

bbbales2 and others added 6 commits December 2, 2020 15:05

Updated exponential cdf to use to_value_column_array_or_scalar (Issue #…

6c93f56

…2101)

get a few of the glm functions working

43867c7

Merge commit 'e98bfbdb70d66f3b28148d400ef197219b0a467b' into HEAD

c82ee29

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

bbbd83d

…4.1 (tags/RELEASE_600/final)

get normal and categorical glm working

f80be36

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

1b9605f

…4.1 (tags/RELEASE_600/final)

t4c1 requested changes Dec 4, 2020

View reviewed changes

This was referenced Dec 6, 2020

[WIP] Converting *_ldlt_* functions to work with varmat #2239

Closed

2.26 release varmat meta-issue stan-dev/cmdstan#958

Closed

Merge remote-tracking branch 'origin/develop' into feature/scalar-seq…

204bfb5

…uence-val-probs

SteveBronder closed this Jan 11, 2021

bbbales2 mentioned this pull request Feb 3, 2021

2.28 Varmat issue stan-dev/cmdstan#980

Open

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for var<matrix> in scalar_seq_view<> distributions #2214

Adds support for var<matrix> in scalar_seq_view<> distributions #2214

SteveBronder commented Nov 26, 2020 •

edited

Loading

SteveBronder Dec 1, 2020

t4c1 Dec 2, 2020

SteveBronder Dec 1, 2020

SteveBronder Dec 1, 2020

bbbales2 commented Dec 2, 2020

t4c1 left a comment

t4c1 Dec 4, 2020

t4c1 Dec 4, 2020

t4c1 Dec 4, 2020

t4c1 Dec 4, 2020

t4c1 Dec 4, 2020

t4c1 Dec 4, 2020

SteveBronder Dec 4, 2020

t4c1 Dec 4, 2020

t4c1 Dec 4, 2020

t4c1 Dec 4, 2020

bbbales2 commented Dec 14, 2020

SteveBronder commented Dec 14, 2020

SteveBronder commented Dec 19, 2020

bbbales2 commented Dec 19, 2020

bbbales2 commented Jan 11, 2021

	auto operator[](int i) const { return c_.coeffRef(i); }
	auto operator[](int i) const { return c_.coeff(i); }

		= -std::move(y_minus_mu_over_beta) * std::move(scaled_diff)
		- std::move(inv_beta);

		auto&& sigma_val_vec
		= to_ref(as_column_vector_or_scalar(value_of_rec(sigma_ref)));

	const T_partials_return alpha_dbl = y_vec.val(n);
	const T_partials_return alpha_dbl = alpha_vec.val(n);

Adds support for var<matrix> in scalar_seq_view<> distributions #2214

Adds support for var<matrix> in scalar_seq_view<> distributions #2214

Conversation

SteveBronder commented Nov 26, 2020 • edited Loading

Summary

Tests

Side Effects

Release notes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbbales2 commented Dec 2, 2020

t4c1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbbales2 commented Dec 14, 2020

SteveBronder commented Dec 14, 2020

SteveBronder commented Dec 19, 2020

bbbales2 commented Dec 19, 2020

bbbales2 commented Jan 11, 2021

SteveBronder commented Nov 26, 2020 •

edited

Loading