Adds views for var_value<Eigen::Matrix> class #2024

SteveBronder · 2020-08-20T00:17:55Z

Summary

This adds the views block(), row(), col(), operator()(), coeff() segment(), head(), tail() to var_value and vari_value.

Some things to note

These views are read only and so the returning type and member functions are const.
Blocks of eigen matrices need different Strides so this adds a new template parameter to vari_value that allows the underlying Eigen::Map to have an InnerStride<> or OuterStride<>.
Because the mapped type is const and we need to specify a stride for the vari_type inside of var_value there's an additional template VariType for var_value that allows for the underlying vari_type to be overridden. So for example in the block() function we have

  inline const auto block(Eigen::Index i, Eigen::Index j, Eigen::Index p, Eigen::Index q) const {
    using vari_sub = decltype(vi_->block(1, 1, 3, 3));
    using var_sub = var_value<const typename vari_sub::PlainObject, const typename vari_sub::vari_type>;
    return var_sub(new vari_sub(vi_->block(i, j, p, q)));
  }

where vari_sub::vari_type is an vari_value<Eigen::MatrixXd, Eigen::OuterStride<>>

I'd really like to get rid of the extra templates here but I'm not sure how else to handle the InnerStride<> and OuterStride<> for the underlying Eigen::Map without them.

Tests

Tests were added to var_test and vari_test to check whether subsets are taken correctly.

Side Effects

Not a side effect but some todo's include

Add var_value<Matrix> overrides for the functions head() etc for the stan language to access
Add these subsetting ops to var_value<SparseMatrix>. I wanted to wait on that to make sure the pattern here looked okay before I wrote it out for sparse as well.

Release notes

Replace this text with a short note on what will change if this pull request is merged in which case this will be included in the release notes.

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

…4.1 (tags/RELEASE_600/final)

…ed once

…4.1 (tags/RELEASE_600/final)

stan/math/rev/core/var.hpp

stan/math/rev/core/vari.hpp

t4c1 · 2020-08-20T09:57:53Z

stan/math/rev/core/vari.hpp

+   * @param j Column index
+   */
+  inline const auto coeff(Eigen::Index i, Eigen::Index j) const {
+    return vari_value<double>(val_(i, j), adj_(i, j));


Making just a copy of the value and adjoint is not the correct thing to do here, as setting adjoint of the copy will not propagate it back to the matrix.

Yes that's intented since you can't assign to subsets of a var_value<>. That's also why these are all returning const types

Unfortunately there still is a problem. Even if you only use a value in a rev operation (and not change it), that rev operation still needs to update the adjoint and that adjoint needs to propagate. I think we need a scalar var view with separate pointers to value and adjoint.

Yes so I agree with this, what I'm trying to figure out right now is that for the var to decide if it holds a view or an actual vari_value I'm doing

using vari_type = std::conditional_t<is_plain_type<value_type>::value, vari_value<value_type>, vari_view<T>>;

So this is fine and good for Block types and other views, but for views into coefficients value_type/T will be a double which is the plain type so this will become a vari_value<double>. We don't want to explicitly have a second template parameter to set the vari_type to, can you think of another way to handle this?

I would make a separate class var_view (not vari). This also avoids two pointer dereferences (one to get vari and second to get value/adjoint) )when operationg on such a scalar. Also since this class is only used for scalars we don't need to template it.

We could also make this object be implicitly convertible to var_value<double>, registering the callback to propagate adjoint on the stack. Although for performance reason we could also not (or at least make it explicit) and explicitly use var_view wherever this is used.

Interesting! Okay I'll work on this today

So I had a bit of a hard time getting what you suggested to work. In var<mat>.coeff I added a callback to propogate the adjoint of the var_value<double> back to the original var<mat>. I think this makes since assuming that the two cases this will be used 99% of the time are

assigning a cell of a static matrix to a var

real a = -B[i];

iterating over a static matrix to assign to the cells in a dynamic matrix

matrix[N, M] A; // assume static matrix[N, M] B; // assume dynamic for (i in 1:N) { B[i] = -A[i]; // very inefficient but a thing a user might want to do }

so for both of these we will need to make a var anyway so having a var_view would not help. Are there normal cases I'm missing that a var_view would be more efficient? If you have any time to take a crack at this I'd be interested in seeing what you have in mind!

stan/math/rev/core/vari.hpp

…4.1 (tags/RELEASE_600/final)

…ASE_601/final)

SteveBronder · 2020-08-25T00:07:33Z

Ugh I think we updated doxygen and it's catching a new doc error. Gonna look into it

SteveBronder · 2020-08-25T00:24:08Z

Ack this might be a doxygen bug. @serban-nicusor-toptal do you know what version of doxygen Jenkin's uses? If it's from 1.8.14-1.8.17 it might be the bug below.

doxygen/doxygen#6754

serban-nicusor-toptal · 2020-08-25T00:29:50Z

Hey @SteveBronder
gelman-linux has:

:~$ doxygen --version
1.8.17
(1.8.17-0ubuntu2).

Should I update it ? Latest or any specific version ?

SteveBronder · 2020-08-25T00:47:26Z

It looks like the amici folks were able to work around this and I just asked them what the problem was for them so we could potentially fix it here.

AMICI-dev/AMICI#507

I'm not sure updating doxygen would work. I'm going to set doxygen warnings to not error out for a bit. I'm on a debian buster setup right now that only has access to doxygen 1.8.13 so can't replicate the error but I think I can replicate this on my home desktop

stan-buildbot · 2020-08-28T06:40:34Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	4.28	4.26	1.01	0.59% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.98	-2.19% slower
eight_schools/eight_schools.stan	0.09	0.09	1.03	3.12% faster
gp_regr/gp_regr.stan	0.2	0.2	0.99	-0.53% slower
irt_2pl/irt_2pl.stan	5.23	5.25	1.0	-0.32% slower
performance.compilation	89.86	87.57	1.03	2.55% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	8.26	8.2	1.01	0.68% faster
pkpd/one_comp_mm_elim_abs.stan	26.82	28.04	0.96	-4.56% slower
sir/sir.stan	136.57	142.32	0.96	-4.21% slower
gp_regr/gen_gp_data.stan	0.04	0.04	1.0	0.0% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.29	3.47	0.95	-5.36% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.38	0.39	0.97	-2.69% slower
arK/arK.stan	1.84	2.57	0.72	-39.65% slower
arma/arma.stan	0.59	0.73	0.8	-24.67% slower
garch/garch.stan	0.61	0.72	0.85	-18.14% slower
Mean result: 0.949659224092

Jenkins Console Log
Blue Ocean
Commit hash: 7a06a38

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/math/prim/meta/is_eigen_dense_plain_type.hpp

stan/math/prim/meta/plain_type.hpp

…ASE_601/final)

…into feature/vari-eigen-slices

…ASE_601/final)

…n eigen type

…ASE_601/final)

SteveBronder · 2020-09-03T02:47:14Z

stan/math/prim/meta/plain_type.hpp

@@ -70,7 +70,7 @@ struct plain_type<T, require_t<is_detected<T, internal::plain_type_check_t>>> {
 */
 template <typename T>
 struct plain_type<T,
-                  require_t<bool_constant<


@t4c1 I had to add this so that it would use the default for kernel expressions. Or should we add a PlainObject to kernel expressions?

Or should we add a PlainObject to kernel expressions?

No. Adding a require for eigen types here is fine. If/when we do need a plain_type for kernel expressions we should add a specialization, not PlainObject.

SteveBronder · 2020-09-03T05:35:46Z

@serban-nicusor-toptal it looks like the distrubtion tests are failing because the jenkins machine ran out of disk storage, do you know how to fix that?

t4c1 · 2020-09-03T06:42:49Z

stan/math/rev/core/var.hpp

+    reverse_pass_callback([vari_coeff, this, i]() mutable {
+      this->vi_->adj_(i) += vari_coeff->adj_;
+    });


I don't like having to do a copy for the reverse pass. I guess it works, so we can leave it for now, but I sure am revisiting this soon.

Yeah it's a bummer, but I think given the use case it make sense no? Like if most of the time we are using .coeff() to assign to a var then I think we need to do something like this right? But I'm fine with it for now so I can go start the plumbing up in the stan repo so we can kick of work on the compiler stuff

Can you wait a few days with changes in stan repo and compiler? I have an idea I would like to try on this views. I think we can make them assignable, so there is no need for thius type of matrices to be static. This would allow us to eventuallly completely replace matrix<var> with var<matrix>.

stan/math/prim/meta/plain_type.hpp

stan-buildbot · 2020-09-04T01:52:59Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	4.22	4.16	1.01	1.29% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.97	-2.86% slower
eight_schools/eight_schools.stan	0.09	0.09	0.98	-2.05% slower
gp_regr/gp_regr.stan	0.2	0.2	1.0	-0.37% slower
irt_2pl/irt_2pl.stan	5.21	5.27	0.99	-1.17% slower
performance.compilation	89.92	87.77	1.02	2.39% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	8.23	8.5	0.97	-3.24% slower
pkpd/one_comp_mm_elim_abs.stan	27.06	29.0	0.93	-7.14% slower
sir/sir.stan	139.32	130.55	1.07	6.3% faster
gp_regr/gen_gp_data.stan	0.04	0.05	1.0	-0.22% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.36	3.37	1.0	-0.34% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.38	0.38	1.01	1.3% faster
arK/arK.stan	2.58	2.55	1.01	1.23% faster
arma/arma.stan	0.73	0.72	1.01	1.16% faster
garch/garch.stan	0.73	0.73	1.0	-0.13% slower
Mean result: 0.998270965974

Jenkins Console Log
Blue Ocean
Commit hash: 434261b

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 · 2020-09-04T02:53:44Z

@t4c1 when you get a min can you take a look at this?

t4c1

Looks good, there is just some cleanup left to do.

stan/math/prim/meta/plain_type.hpp

stan/math/rev/core/var.hpp

stan/math/rev/core/vari.hpp

…ASE_601/final)

stan-buildbot · 2020-09-04T19:30:50Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	4.18	4.26	0.98	-1.96% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	1.01	1.28% faster
eight_schools/eight_schools.stan	0.09	0.09	0.97	-2.63% slower
gp_regr/gp_regr.stan	0.19	0.2	0.99	-0.87% slower
irt_2pl/irt_2pl.stan	5.23	5.24	1.0	-0.07% slower
performance.compilation	88.61	87.46	1.01	1.29% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	8.19	8.25	0.99	-0.77% slower
pkpd/one_comp_mm_elim_abs.stan	27.5	28.1	0.98	-2.17% slower
sir/sir.stan	130.36	141.6	0.92	-8.63% slower
gp_regr/gen_gp_data.stan	0.04	0.04	1.0	-0.33% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.35	3.34	1.0	0.43% faster
pkpd/sim_one_comp_mm_elim_abs.stan	0.38	0.38	0.98	-2.19% slower
arK/arK.stan	2.57	2.55	1.01	0.82% faster
arma/arma.stan	0.75	0.74	1.01	0.55% faster
garch/garch.stan	0.73	0.72	1.01	0.56% faster
Mean result: 0.990841536556

Jenkins Console Log
Blue Ocean
Commit hash: 77b3ec2

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 and others added 11 commits August 19, 2020 19:46

Adds views for var_value<Eigen::Matrix> class

3a5288e

remove extra comment

85ba28d

Merge commit '8a67bc7b92939d8c2947761107b3a9287b71ece5' into HEAD

005a032

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

a19f6b0

…4.1 (tags/RELEASE_600/final)

add consts and inlines to subsetting functions

4351cd7

merge to develop

2c8549e

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

9477a0e

…4.1 (tags/RELEASE_600/final)

Fix docs for var_value<>.head()

9bac070

Fix tests that used a double for the second bool parameter

a935086

move call to .col() in vari_value out of the return so it's only call…

1c5e90c

…ed once

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

638e704

…4.1 (tags/RELEASE_600/final)

t4c1 requested changes Aug 20, 2020

View reviewed changes

t4c1 reviewed Aug 20, 2020

View reviewed changes

andrjohns reviewed Aug 20, 2020

View reviewed changes

stan/math/rev/core/vari.hpp Outdated Show resolved Hide resolved

SteveBronder and others added 9 commits August 20, 2020 23:42

Adds a vari_view for getting slices of the inner eigen types

a20d647

Make view hold the Eigen view type instead of making a reference to it

b282ee3

add is_eigen_dense_plain_type

85f5091

Merge commit '05910656cd5537405aed93a76ed183f5a7b74bcd' into HEAD

485ded9

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

f5834a5

…4.1 (tags/RELEASE_600/final)

add type trait for plain dense eigen objects and fix doxygen for acos

92e24fd

merge with clang-format remote changes

2e9a03c

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

47ecd35

…ASE_601/final)

fix acos docs

039fcbf

acos docs

d065ef7

one more try for acos fix

f4d4162

Set doxygen warnings to not cause errors

363122f

This was referenced Aug 30, 2020

Meta Issue for Static Matrices #1875

Open

Add var_value<Matrix> rvalue accessors stan-dev/stan#2951

Closed

t4c1 reviewed Aug 31, 2020

View reviewed changes

stan/math/prim/meta/is_eigen_dense_plain_type.hpp Outdated Show resolved Hide resolved

stan/math/prim/meta/plain_type.hpp Outdated Show resolved Hide resolved

SteveBronder mentioned this pull request Aug 31, 2020

Add var<matrix> creation methods for reader stan-dev/stan#2952

Closed

SteveBronder and others added 7 commits September 2, 2020 16:06

set coeff access to propogate adjoints back to var<matrix> cells

06b2756

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

8dbe3ba

…ASE_601/final)

fix header

44f7c64

Merge branch 'feature/vari-eigen-slices' of github.com:stan-dev/math …

1f35f19

…into feature/vari-eigen-slices

update plain_type_t to handle EigenBase<Derived> types

df9d679

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

e863a51

…ASE_601/final)

add compile time rows and cols to vari_view

09a23c6

SteveBronder mentioned this pull request Sep 2, 2020

Add var<mat> softmax and function for testing mat<var> and var<mat> methods #2050

Merged

5 tasks

SteveBronder and others added 2 commits September 2, 2020 22:46

add extra condition for plain_type specialization to check if it is a…

a8ed00f

…n eigen type

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

239c675

…ASE_601/final)

SteveBronder commented Sep 3, 2020

View reviewed changes

t4c1 reviewed Sep 3, 2020

View reviewed changes

SteveBronder added 2 commits September 3, 2020 13:56

remove extra case for plain_type_t

447c2f3

uncomment vari vector view tests

434261b

t4c1 requested changes Sep 4, 2020

View reviewed changes

stan/math/prim/meta/plain_type.hpp Outdated Show resolved Hide resolved

stan/math/rev/core/var.hpp Outdated Show resolved Hide resolved

stan/math/rev/core/vari.hpp Outdated Show resolved Hide resolved

SteveBronder and others added 3 commits September 4, 2020 07:05

remove commented out code and extra headers

bed31a6

Merge commit '5c361743be04f995301477522062aa8d7406d91f' into HEAD

552d800

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

77b3ec2

…ASE_601/final)

t4c1 approved these changes Sep 7, 2020

View reviewed changes

t4c1 merged commit f235b6c into develop Sep 7, 2020

rok-cesnovar mentioned this pull request Oct 13, 2020

Release of Stan Math 3.4 #2128

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds views for var_value<Eigen::Matrix> class #2024

Adds views for var_value<Eigen::Matrix> class #2024

SteveBronder commented Aug 20, 2020

t4c1 Aug 20, 2020 •

edited

Loading

SteveBronder Aug 20, 2020

t4c1 Aug 21, 2020

SteveBronder Aug 30, 2020

t4c1 Aug 31, 2020

SteveBronder Aug 31, 2020

SteveBronder Sep 2, 2020

SteveBronder commented Aug 25, 2020

SteveBronder commented Aug 25, 2020

serban-nicusor-toptal commented Aug 25, 2020 •

edited

Loading

SteveBronder commented Aug 25, 2020

stan-buildbot commented Aug 28, 2020

SteveBronder Sep 3, 2020

t4c1 Sep 3, 2020

SteveBronder commented Sep 3, 2020

t4c1 Sep 3, 2020

SteveBronder Sep 3, 2020

t4c1 Sep 4, 2020

stan-buildbot commented Sep 4, 2020

SteveBronder commented Sep 4, 2020

t4c1 left a comment

stan-buildbot commented Sep 4, 2020

Adds views for var_value<Eigen::Matrix> class #2024

Adds views for var_value<Eigen::Matrix> class #2024

Conversation

SteveBronder commented Aug 20, 2020

Summary

Tests

Side Effects

Release notes

Checklist

t4c1 Aug 20, 2020 • edited Loading

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

SteveBronder commented Aug 25, 2020

SteveBronder commented Aug 25, 2020

serban-nicusor-toptal commented Aug 25, 2020 • edited Loading

SteveBronder commented Aug 25, 2020

stan-buildbot commented Aug 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveBronder commented Sep 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stan-buildbot commented Sep 4, 2020

SteveBronder commented Sep 4, 2020

t4c1 left a comment

Choose a reason for hiding this comment

stan-buildbot commented Sep 4, 2020

t4c1 Aug 20, 2020 •

edited

Loading

serban-nicusor-toptal commented Aug 25, 2020 •

edited

Loading