Added a few varmat functions (Issue #2101) #2106

bbbales2 · 2020-09-25T20:53:14Z

Summary

This adds var_value<Eigen::Matrix<T, R, C>> implementations for columns_dot_self, determinant, dot_self, inverse, log_determinant, matrix_power, multiply_lower_tri_self_transpose, and tcrossprod. (part of #2101)

Side Effects

Should be none

Release notes

Added var_value<Eigen::Matrix<T, R, C>> specializations for columns_dot_self, determinant, dot_self, inverse, log_determinant, matrix_power, multiply_lower_tri_self_transpose, and tcrossprod.

Checklist

Math issue Make functions with custom autodiff var<mat> friendly #2101
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)

bbbales2

Questions @SteveBronder

bbbales2 · 2020-09-25T20:56:32Z

stan/math/rev/fun/columns_dot_self.hpp

+
+  T_return res = (arena_x_val.array() * arena_x_val.array()).colwise().sum();
+
+  if (x.size() > 0) {


What is the proper way to return a size zero thing?

When the input and output are both size zero, I just pass the input through.

If I have a valid return type and just return a {} for the zero case, sometimes I end up with something that doesn't have a vari and everything downstream explodes.

I'd just return x, since we also know that is size 0 (and that means it doesn't hold anything)

The return type is different than the argument type in this case.

bbbales2 · 2020-09-25T20:57:33Z

stan/math/rev/fun/matrix_power.hpp

+  arena_t<plain_type_t<T>> arena_M = M_ref;
+
+  powers[0] = Eigen::MatrixXd::Identity(N, N);
+  powers[1] = value_of(M_ref).eval();


I needed to add the eval here otherwise I got this error when running the mix tests:

[==========] Running 1 test from 1 test suite. [----------] Global test environment set-up. [----------] 1 test from MathMatrixPower [ RUN ] MathMatrixPower.ad_tests matrix_power_test: lib/eigen_3.3.7/Eigen/src/Core/DenseBase.h:257: void Eigen::DenseBase<Eigen::Map<Eigen::Matrix<double, -1, -1, 0, -1, -1>, 8, Eigen::Stride<0, 0> > >::resize(Eigen::Index, Eigen::Index) [Derived = Eigen::Map<Eigen::Matrix<double, -1, -1, 0, -1, -1>, 8, Eigen::Stride<0, 0> >]: Assertion `rows == this->rows() && cols == this->cols() && "DenseBase::resize() does not actually allow to resize."' failed. Aborted (core dumped) test/unit/math/mix/fun/matrix_power_test --gtest_output="xml:test/unit/math/mix/fun/matrix_power_test.xml" failed exit now (09/25/20 16:44:55 EDT)

I'm not sure what that was about.

I think you need to give them a default size when you make the vector like

arena_t<std::vector<Eigen::MatrixXd>> powers(n + 1, Eigen::MatrixXd(N, N));

Otherwise the vector holds a bunch of default initialized matrices

bbbales2 · 2020-09-25T21:02:26Z

stan/math/rev/fun/tcrossprod.hpp

+  arena_t<plain_type_t<T>> arena_M = M;
+  arena_t<Eigen::MatrixXd> arena_M_val = value_of(arena_M);
+
+  arena_t<T_var> res = arena_M_val * arena_M_val.transpose();


This is a better way to handle vars for the matvar case:

math/stan/math/rev/fun/tcrossprod.hpp

Line 39 in 0aaea95

arena_matrix<Eigen::Matrix<var, T::RowsAtCompileTime, T::RowsAtCompileTime>>

Should we just be in the habit of doing both implementations? The reverse pass callback code is different. They're really kinda different things.

I think we can handle both matvar and varmat in the same overload if we are smart about it.

Well the big difference is that if we know the output is symmetric, then we only need to build new varis on the diagonal and one half of the other side of the matrix.

The issue then is that the reverse mode code is different from how we'd handle the varmats. The off diagonal adjoints of the matvar have been incremented twice, whereas the off diagonal adjoints of the mat var (of which there are two) have each been incremented once.

I think it's totally fine to have separate functions when stuff is different

t4c1 · 2020-09-28T10:25:40Z

stan/math/rev/fun/tcrossprod.hpp

+  arena_t<plain_type_t<T>> arena_M = M;
+  arena_t<Eigen::MatrixXd> arena_M_val = value_of(arena_M);
+
+  arena_t<T_var> res = arena_M_val * arena_M_val.transpose();


I think we can handle both matvar and varmat in the same overload if we are smart about it.

t4c1 · 2020-09-28T10:28:55Z

stan/math/rev/fun/tcrossprod.hpp

-  return MMt;
+
+  arena_t<plain_type_t<T>> arena_M = M;
+  arena_t<Eigen::MatrixXd> arena_M_val = value_of(arena_M);


Hmm, I think we need to do something here to avoid the copy in the varmat case. The easiest option would be to change the var to use the arena matrix internally instead of maps, so the arena_matrix constructor here will be able to do a shallow copy.

^ I was thinking something similar yesterday. idt it would add any unreasonable bulk and would make stuff flow a lot more naturally. I want to get in all the big changes from #2064 then we can do this in a separate PR

Actually this was pretty easy-peasy!

feature/multi-expression-vari-views...feature/vari_value-arena-matrix

Nice, can you open a separate pull request with this? I don't wanna slow this up going through here.

Also the code here: #2106 (comment) reminds me that maybe this would work:

const auto& arena_M_val = to_arena(value_of(arena_M));

If it see's an arena variable it just forwards it along I think.

stan/math/rev/fun/tcrossprod.hpp

stan/math/rev/fun/multiply_lower_tri_self_transpose.hpp

stan/math/rev/fun/matrix_power.hpp

stan/math/rev/fun/log_determinant.hpp

stan/math/rev/fun/inverse.hpp

stan/math/rev/fun/dot_self.hpp

stan/math/rev/fun/determinant.hpp

…ASE_601/final)

into feature/a_few_varmats

SteveBronder

Bunch o' comments!

SteveBronder · 2020-09-30T20:40:23Z

stan/math/rev/fun/columns_dot_self.hpp

+
+  T_return res = (arena_x_val.array() * arena_x_val.array()).colwise().sum();
+
+  if (x.size() > 0) {


I'd just return x, since we also know that is size 0 (and that means it doesn't hold anything)

SteveBronder · 2020-09-30T20:43:38Z

stan/math/rev/fun/columns_dot_self.hpp

+  using T_return = promote_var_matrix_t<Eigen::RowVectorXd, Mat>;
+
+  arena_t<plain_type_t<Mat>> arena_x = x;
+  arena_t<Eigen::MatrixXd> arena_x_val = value_of(arena_x);


Could we do

Suggested change

arena_t<Eigen::MatrixXd> arena_x_val = value_of(arena_x);

arena_t<decltype(value_of(arena_x))> arena_x_val = value_of(arena_x);

So for matrix<var> that will give back an arena_matrix<Matrix<var>> and for var<matrix> will give back an eigen map. That should avoid a copy here in the var<matrix> case

value_of(arena_x) is (an expressions that evaluates to) MatrixXd, so arena_t<decltype(value_of(arena_x))> equals arena_t<Eigen::MatrixXd>

Ah good call.

Another note though, does this make a copy for var<matrix> rn? I think after #2064 this will be better since then we use arena_matrix on the backend of var_value<matrix>

stan/math/rev/fun/columns_dot_self.hpp

stan/math/rev/fun/determinant.hpp

stan/math/rev/fun/multiply_lower_tri_self_transpose.hpp

SteveBronder · 2020-09-30T21:29:29Z

stan/math/rev/fun/tcrossprod.hpp

-  return MMt;
+
+  arena_t<plain_type_t<T>> arena_M = M;
+  arena_t<Eigen::MatrixXd> arena_M_val = value_of(arena_M);


^ I was thinking something similar yesterday. idt it would add any unreasonable bulk and would make stuff flow a lot more naturally. I want to get in all the big changes from #2064 then we can do this in a separate PR

SteveBronder · 2020-09-30T21:30:15Z

stan/math/rev/fun/tcrossprod.hpp

+  arena_t<plain_type_t<T>> arena_M = M;
+  arena_t<Eigen::MatrixXd> arena_M_val = value_of(arena_M);
+
+  arena_t<T_var> res = arena_M_val * arena_M_val.transpose();


I think it's totally fine to have separate functions when stuff is different

test/unit/math/mix/fun/inverse_test.cpp

…4.1 (tags/RELEASE_600/final)

into feature/a_few_varmats

bbbales2 · 2020-10-01T20:46:00Z

I responded to the comments and went through and tried to make these as fast as possible for mat<var> types (so as to not create any slowdown since the last release).

I'm re-running the benchmarks now but not great news.

tcrossprod and multiply_lower_self_tri_transpose are very slow for small values (like half the speed for 2x2, 4x4), but get fast pretty quick (edit: faster than the old versions).

determinant, log_determinant, and inverse are all consistently slightly slower.

columns_dot_self is the one I need some help on depending on how the benchmarks work out. It is quite a bit slower for large inputs on the new branch (small values it does okay). I'll check the benchmarks tomorrow when they finish and update this pull.

I didn't check matrix_power cause the previous benchmarks showed it was already faster everywhere.

Anyway if anyone wants to mess with these to make them faster, have at it, I'm okay with where this is at so I'll just be trying to get a green light.

…4.1 (tags/RELEASE_600/final)

into feature/a_few_varmats

…4.1 (tags/RELEASE_600/final)

into feature/a_few_varmats

bbbales2

Question!

bbbales2 · 2020-10-01T23:53:06Z

stan/math/rev/fun/tcrossprod.hpp

-  if (M.rows() == 0) {
-    return {};
+template <typename T, require_rev_matrix_t<T>* = nullptr>
+inline promote_var_matrix_t<Eigen::MatrixXd, T> tcrossprod(const T& M) {


@t4c1, @SteveBronder I think there's a problem with either plain_type_t<T> or promote_var_matrix_t or my use thereof.

If the return type here is plain_type_t<T>, you'll get errors compiling crossprod (here). crossprod(x) is implemented as tcrossprod(x.transpose()).

When I changed the return type to promote_var_matrix_t<T>, things worked.

I added pretty_prints for the types:

T: stan::math::test::type_name() [Arg = Eigen::Transpose<const Eigen::Matrix<stan::math::var_value<double>, -1, -1, 0, -1, -1> > plain_type_t : stan::math::test::type_name() [Arg = Eigen::Matrix<stan::math::var_value<double>, -1, -1, 1, -1, -1> promote_var_matrix_t : stan::math::test::type_name() [Arg = Eigen::Matrix<stan::math::var_value<double>, -1, -1, 0, -1, -1>

Is it me or it is a template?

Sorry I'll look into this tmrw morning. The jenkins link sends me to

./test/unit/math/serializer.hpp:199:11: error: no matching member function for call to 'push_back' vals_.push_back(x); ~~~~~~^~~~~~~~~ ./test/unit/math/serializer.hpp:291:5: note: in instantiation of function template specialization 'stan::test::serializer<stan::math::var_value<double> >::write<Eigen::Matrix<stan::math::var_value<double>, -1, -1, 1, -1, -1> >' requested here s.write(x);

I'm not seeing how it's related to the return type. Is there a reason you can't use auto as the return type?

Is there a reason you can't use auto as the return type

We need explicit return types here because the return type gets build internally as an arena_t<T_Return> and you gotta set the return type to T_Return to get it to cast to a non-arena thing.

Looking at the different template arguments between the types:

Eigen::Matrix<stan::math::var_value<double>, -1, -1, 1, -1, -1> Eigen::Matrix<stan::math::var_value<double>, -1, -1, 0, -1, -1>

That third integer is the 'Options' flag, which controls storage order. I would guess (emphasis on the guess) that the .transpose() operator maps the matrix to row-major storage, so the plain type of the input (and the eventual return type) is a row-major matrix and then compilation fails when you try to return a column-major matrix instead. No idea why plain_type_t and promote_var_matrix_t would handle that differently though

From the type printouts it seems plain_type_t is returning a row major matrix (yes, it can do that in some cases!). Any function not generalized for expresions is still expecting its inputs to be col major, so I guess that causes compilation failures.

This means that we shouldn't use plain_type_t (or .eval() and auto) for return values in the few functions that eval expresions, for which Eigen prefer row major types.

Won't it be more of an issue with expressions since they'll propagate the row-major matrix which could lead to unexpected behaviour further down the chain?

Functions handling expressions also handle row major matrices, so it won't matter.

I was mostly thinking that any looping is assuming column-major ordering (i.e., loops over column first), so some programs could end up slower if a function returned a row-major matrix at some point. But I don't know how much looping we still have at the moment

We need explicit return types here because the return type gets build internally as an arena_t<T_Return> and you gotta set the return type to T_Return to get it to cast to a non-arena thing.

promote_var_matrix_t returns a var<matrix> or matrix<var> so I usually just wrap the return like return ret_type(ret)

I think we should consider any remaining loops over matrices as potential performance issues and replace them with Eigen expressions.

SteveBronder · 2020-10-02T18:35:33Z

I'm reading the docs right now for eigen products and should we be using noalias() more often with these? In the docs they specifically call out += (which we do all the time for the reverse pass)

https://eigen.tuxfamily.org/dox/TopicWritingEfficientProductExpression.html

So like this

    const auto& adjL = (adj.transpose() + adj)
                       * arena_L_val.template triangularView<Eigen::Lower>();

    arena_L.adj() += adjL.template triangularView<Eigen::Lower>();

should be

    const auto& adjL = (adj.transpose() + adj)
                       * arena_L_val.template triangularView<Eigen::Lower>();

    arena_L.adj().noalias() += adjL.template triangularView<Eigen::Lower>();

bbbales2 · 2020-10-02T18:42:53Z

Oh interesting. I'll try it out real quick.

bbbales2 · 2020-10-02T18:45:19Z

I got a compile error (edit: added full error):

bbales2@tadpole:~/math-benchmarks$ make src/multiply_lower_tri_self_transpose && src/multiply_lower_tri_self_transpose
g++ -std=c++1y -pthread -D_REENTRANT -Wno-sign-compare -Wno-ignored-attributes      -I math/lib/tbb_2019_U8/include   -O3  -I math/ -I math/lib/eigen_3.3.7 -I math/lib/boost_1.72.0 -I math/lib/sundials_5.2.0/include -I math/lib/benchmark_1.5.1/include    -DBOOST_DISABLE_ASSERTS              -Wl,-L,"/home/bbales2/math-benchmarks/math/lib/tbb" -Wl,-rpath,"/home/bbales2/math-benchmarks/math/lib/tbb"     -L math/lib/benchmark_1.5.1/build/src  src/multiply_lower_tri_self_transpose.cpp          -lbenchmark -ltbb -o src/multiply_lower_tri_self_transpose
In file included from math/stan/math/rev/fun.hpp:112:0,
                 from math/stan/math/rev.hpp:11,
                 from math/stan/math.hpp:19,
                 from src/multiply_lower_tri_self_transpose.cpp:2:
math/stan/math/rev/fun/multiply_lower_tri_self_transpose.hpp: In instantiation of ‘stan::math::multiply_lower_tri_self_transpose(const T&)::<lambda()> mutable [with T = Eigen::Matrix<stan::math::var_value<double>, -1, -1>; stan::require_rev_matrix_t<T>* <anonymous> = 0]’:
math/stan/math/rev/fun/multiply_lower_tri_self_transpose.hpp:27:40:   required from ‘struct stan::math::multiply_lower_tri_self_transpose(const T&) [with T = Eigen::Matrix<stan::math::var_value<double>, -1, -1>; stan::require_rev_matrix_t<T>* <anonymous> = 0; stan::plain_type_t<T> = Eigen::Matrix<stan::math::var_value<double>, -1, -1>]::<lambda()>’
math/stan/math/rev/fun/multiply_lower_tri_self_transpose.hpp:27:24:   required from ‘stan::plain_type_t<T> stan::math::multiply_lower_tri_self_transpose(const T&) [with T = Eigen::Matrix<stan::math::var_value<double>, -1, -1>; stan::require_rev_matrix_t<T>* <anonymous> = 0; stan::plain_type_t<T> = Eigen::Matrix<stan::math::var_value<double>, -1, -1>]’
src/multiply_lower_tri_self_transpose.cpp:18:49:   required from ‘multiply_lower_tri_self_transpose(benchmark::State&)::<lambda(const auto:127& ...)> [with auto:127 = {Eigen::Matrix<stan::math::var_value<double>, -1, -1, 0, -1, -1>}]’
math/stan/math/prim/functor/apply.hpp:26:11:   required from ‘constexpr decltype(auto) stan::math::internal::apply_impl(F&&, Tuple&&, std::index_sequence<I ...>) [with F = multiply_lower_tri_self_transpose(benchmark::State&)::<lambda(const auto:127& ...)>&; Tuple = std::tuple<Eigen::Matrix<stan::math::var_value<double>, -1, -1, 0, -1, -1> >&; long unsigned int ...I = {0}; std::index_sequence<I ...> = std::integer_sequence<long unsigned int, 0>]’
math/stan/math/prim/functor/apply.hpp:44:30:   required from ‘constexpr decltype(auto) stan::math::apply(F&&, Tuple&&) [with F = multiply_lower_tri_self_transpose(benchmark::State&)::<lambda(const auto:127& ...)>&; Tuple = std::tuple<Eigen::Matrix<stan::math::var_value<double>, -1, -1, 0, -1, -1> >&]’
src/callback_bench_impl.hpp:31:28:   required from ‘void callback_bench_impl(F_init, F_run, benchmark::State&) [with F_init = multiply_lower_tri_self_transpose(benchmark::State&)::<lambda(benchmark::State&)>; F_run = multiply_lower_tri_self_transpose(benchmark::State&)::<lambda(const auto:127& ...)>]’
src/multiply_lower_tri_self_transpose.cpp:21:39:   required from here
math/stan/math/rev/fun/multiply_lower_tri_self_transpose.hpp:33:29: error: no match for ‘operator+=’ (operand types are ‘Eigen::NoAlias<Eigen::CwiseUnaryView<Eigen::MatrixBase<Eigen::Map<Eigen::Matrix<stan::math::var_value<double>, -1, -1>, 8, Eigen::Stride<0, 0> > >::adj_Op, Eigen::Map<Eigen::Matrix<stan::math::var_value<double>, -1, -1>, 8, Eigen::Stride<0, 0> > >, Eigen::MatrixBase>’ and ‘Eigen::MatrixBase<Eigen::Product<Eigen::CwiseBinaryOp<Eigen::internal::scalar_sum_op<double, double>, const Eigen::Transpose<const Eigen::CwiseUnaryView<Eigen::MatrixBase<Eigen::Map<Eigen::Matrix<stan::math::var_value<double>, -1, -1>, 8, Eigen::Stride<0, 0> > >::adj_Op, Eigen::Map<Eigen::Matrix<stan::math::var_value<double>, -1, -1>, 8, Eigen::Stride<0, 0> > > >, const Eigen::CwiseUnaryView<Eigen::MatrixBase<Eigen::Map<Eigen::Matrix<stan::math::var_value<double>, -1, -1>, 8, Eigen::Stride<0, 0> > >::adj_Op, Eigen::Map<Eigen::Matrix<stan::math::var_value<double>, -1, -1>, 8, Eigen::Stride<0, 0> > > >, Eigen::TriangularView<Eigen::Map<Eigen::Matrix<double, -1, -1>, 8, Eigen::Stride<0, 0> >, 1>, 0> >::ConstTriangularViewReturnType<1>::Type {aka const Eigen::TriangularView<const Eigen::Product<Eigen::CwiseBinaryOp<Eigen::internal::scalar_sum_op<double, double>, const Eigen::Transpose<const Eigen::CwiseUnaryView<Eigen::MatrixBase<Eigen::Map<Eigen::Matrix<stan::math::var_value<double>, -1, -1>, 8, Eigen::Stride<0, 0> > >::adj_Op, Eigen::Map<Eigen::Matrix<stan::math::var_value<double>, -1, -1>, 8, Eigen::Stride<0, 0> > > >, const Eigen::CwiseUnaryView<Eigen::MatrixBase<Eigen::Map<Eigen::Matrix<stan::math::var_value<double>, -1, -1>, 8, Eigen::Stride<0, 0> > >::adj_Op, Eigen::Map<Eigen::Matrix<stan::math::var_value<double>, -1, -1>, 8, Eigen::Stride<0, 0> > > >, Eigen::TriangularView<Eigen::Map<Eigen::Matrix<double, -1, -1>, 8, Eigen::Stride<0, 0> >, 1>, 0>, 1>}’)
     arena_L.adj().noalias() += adjL.template triangularView<Eigen::Lower>();
In file included from math/lib/eigen_3.3.7/Eigen/Core:456:0,
                 from math/lib/eigen_3.3.7/Eigen/Dense:1,
                 from math/stan/math/prim/fun/Eigen.hpp:22,
                 from math/stan/math/rev.hpp:4,
                 from math/stan/math.hpp:19,
                 from src/multiply_lower_tri_self_transpose.cpp:2:
math/lib/eigen_3.3.7/Eigen/src/Core/NoAlias.h:48:41: note: candidate: template<class OtherDerived> ExpressionType& Eigen::NoAlias<ExpressionType, StorageBase>::operator+=(const StorageBase<OtherDerived>&) [with OtherDerived = OtherDerived; ExpressionType = Eigen::CwiseUnaryView<Eigen::MatrixBase<Eigen::Map<Eigen::Matrix<stan::math::var_value<double>, -1, -1>, 8, Eigen::Stride<0, 0> > >::adj_Op, Eigen::Map<Eigen::Matrix<stan::math::var_value<double>, -1, -1>, 8, Eigen::Stride<0, 0> > >; StorageBase = Eigen::MatrixBase]
     EIGEN_STRONG_INLINE ExpressionType& operator+=(const StorageBase<OtherDerived>& other)
                                         ^~~~~~~~
math/lib/eigen_3.3.7/Eigen/src/Core/NoAlias.h:48:41: note:   template argument deduction/substitution failed:
In file included from math/stan/math/rev/fun.hpp:112:0,
                 from math/stan/math/rev.hpp:11,
                 from math/stan/math.hpp:19,
                 from src/multiply_lower_tri_self_transpose.cpp:2:
math/stan/math/rev/fun/multiply_lower_tri_self_transpose.hpp:33:29: note:   ‘Eigen::MatrixBase<Eigen::Product<Eigen::CwiseBinaryOp<Eigen::internal::scalar_sum_op<double, double>, const Eigen::Transpose<const Eigen::CwiseUnaryView<Eigen::MatrixBase<Eigen::Map<Eigen::Matrix<stan::math::var_value<double>, -1, -1>, 8, Eigen::Stride<0, 0> > >::adj_Op, Eigen::Map<Eigen::Matrix<stan::math::var_value<double>, -1, -1>, 8, Eigen::Stride<0, 0> > > >, const Eigen::CwiseUnaryView<Eigen::MatrixBase<Eigen::Map<Eigen::Matrix<stan::math::var_value<double>, -1, -1>, 8, Eigen::Stride<0, 0> > >::adj_Op, Eigen::Map<Eigen::Matrix<stan::math::var_value<double>, -1, -1>, 8, Eigen::Stride<0, 0> > > >, Eigen::TriangularView<Eigen::Map<Eigen::Matrix<double, -1, -1>, 8, Eigen::Stride<0, 0> >, 1>, 0> >::ConstTriangularViewReturnType<1>::Type {aka const Eigen::TriangularView<const Eigen::Product<Eigen::CwiseBinaryOp<Eigen::internal::scalar_sum_op<double, double>, const Eigen::Transpose<const Eigen::CwiseUnaryView<Eigen::MatrixBase<Eigen::Map<Eigen::Matrix<stan::math::var_value<double>, -1, -1>, 8, Eigen::Stride<0, 0> > >::adj_Op, Eigen::Map<Eigen::Matrix<stan::math::var_value<double>, -1, -1>, 8, Eigen::Stride<0, 0> > > >, const Eigen::CwiseUnaryView<Eigen::MatrixBase<Eigen::Map<Eigen::Matrix<stan::math::var_value<double>, -1, -1>, 8, Eigen::Stride<0, 0> > >::adj_Op, Eigen::Map<Eigen::Matrix<stan::math::var_value<double>, -1, -1>, 8, Eigen::Stride<0, 0> > > >, Eigen::TriangularView<Eigen::Map<Eigen::Matrix<double, -1, -1>, 8, Eigen::Stride<0, 0> >, 1>, 0>, 1>}’ is not derived from ‘const Eigen::MatrixBase<Derived>’
     arena_L.adj().noalias() += adjL.template triangularView<Eigen::Lower>();
In file included from math/stan/math/rev/core/var.hpp:10:0,
                 from math/stan/math/rev/meta/is_rev_matrix.hpp:4,
                 from math/stan/math/rev/meta.hpp:8,
                 from math/stan/math/rev/core/accumulate_adjoints.hpp:5,
                 from math/stan/math/rev/core.hpp:4,
                 from math/stan/math/rev.hpp:6,
                 from math/stan/math.hpp:19,
                 from src/multiply_lower_tri_self_transpose.cpp:2:
math/stan/math/rev/functor/reverse_pass_callback.hpp:38:13: error: ‘void stan::math::reverse_pass_callback(F&&) [with F = stan::math::multiply_lower_tri_self_transpose(const T&) [with T = Eigen::Matrix<stan::math::var_value<double>, -1, -1>; stan::require_rev_matrix_t<T>* <anonymous> = 0; stan::plain_type_t<T> = Eigen::Matrix<stan::math::var_value<double>, -1, -1>]::<lambda()>]’, declared using local type ‘stan::math::multiply_lower_tri_self_transpose(const T&) [with T = Eigen::Matrix<stan::math::var_value<double>, -1, -1>; stan::require_rev_matrix_t<T>* <anonymous> = 0; stan::plain_type_t<T> = Eigen::Matrix<stan::math::var_value<double>, -1, -1>]::<lambda()>’, is used but never defined [-fpermissive]
 inline void reverse_pass_callback(F&& functor) {
             ^~~~~~~~~~~~~~~~~~~~~
<builtin>: recipe for target 'src/multiply_lower_tri_self_transpose' failed

…ASE_601/final)

SteveBronder · 2020-10-28T20:01:57Z

I decided it would be better to make is_var_col_vector and friends which should be nice to use in other functions as well.

On a side note I think we should rename is_var_matrix/is_rev_matrix to is_var_eigen/is_rev_eigen. Though that still leaves what to do with renaming is_matrix. is_eigen. We could have is_eigen_or_var_eigen but that's kind of a confusing mouthful

bbbales2 · 2020-10-28T21:20:44Z

stan/math/rev/meta/is_rev_matrix.hpp

@@ -22,5 +22,42 @@ struct is_rev_matrix<
                     math::disjunction<is_eigen<T>, is_eigen<value_type_t<T>>>>>
    : std::true_type {};

+/** \ingroup type_trait


These docs don't appear to be for these functions.

…4.1 (tags/RELEASE_600/final)

bbbales2 · 2020-10-30T16:07:47Z

@rok-cesnovar is this test messed up? It says it's been stuck in one place for 17 hours

rok-cesnovar · 2020-10-30T16:13:56Z

Seems so. Restarted now. If it repeats we need to look into it.

SteveBronder · 2020-10-30T17:02:37Z

Which test is it hanging on?

rok-cesnovar · 2020-10-30T17:03:29Z

Windows. See: https://jenkins.mc-stan.org/blue/organizations/jenkins/Math%20Pipeline/detail/PR-2106/58/pipeline

stan-buildbot · 2020-11-04T09:41:55Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	3.1	3.1	1.0	-0.01% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.92	-8.48% slower
eight_schools/eight_schools.stan	0.12	0.11	1.02	2.41% faster
gp_regr/gp_regr.stan	0.17	0.17	0.99	-1.04% slower
irt_2pl/irt_2pl.stan	5.63	5.65	1.0	-0.39% slower
performance.compilation	90.99	85.61	1.06	5.92% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	8.49	8.48	1.0	0.04% faster
pkpd/one_comp_mm_elim_abs.stan	29.46	30.18	0.98	-2.44% slower
sir/sir.stan	140.44	145.11	0.97	-3.33% slower
gp_regr/gen_gp_data.stan	0.04	0.04	0.98	-1.81% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	2.96	2.96	1.0	0.13% faster
pkpd/sim_one_comp_mm_elim_abs.stan	0.39	0.38	1.02	2.06% faster
arK/arK.stan	1.78	1.77	1.0	0.2% faster
arma/arma.stan	0.6	0.61	0.99	-0.59% slower
garch/garch.stan	0.75	0.75	1.0	0.26% faster
Mean result: 0.996191549822

Jenkins Console Log
Blue Ocean
Commit hash: ec4b37e

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

lgtm! @t4c1 do you want to do another pass through? Else I'm cool to merge this when tests pass

stan-buildbot · 2020-11-05T09:30:21Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	3.07	3.23	0.95	-5.03% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.98	-2.0% slower
eight_schools/eight_schools.stan	0.11	0.11	1.02	1.92% faster
gp_regr/gp_regr.stan	0.18	0.17	1.03	2.62% faster
irt_2pl/irt_2pl.stan	5.69	5.59	1.02	1.62% faster
performance.compilation	88.98	85.49	1.04	3.92% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	8.47	8.47	1.0	-0.06% slower
pkpd/one_comp_mm_elim_abs.stan	29.45	28.97	1.02	1.64% faster
sir/sir.stan	139.31	141.45	0.98	-1.53% slower
gp_regr/gen_gp_data.stan	0.04	0.05	0.95	-5.75% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	2.95	2.98	0.99	-0.95% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.37	0.38	0.97	-2.82% slower
arK/arK.stan	1.79	1.76	1.02	1.79% faster
arma/arma.stan	0.6	0.6	1.01	0.55% faster
garch/garch.stan	0.74	0.74	1.0	-0.37% slower
Mean result: 0.997742198944

Jenkins Console Log
Blue Ocean
Commit hash: 2d99d55

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/err/check_vector.hpp

stan/math/rev/fun/dot_self.hpp

stan/math/rev/fun/log_determinant.hpp

stan/math/rev/fun/matrix_power.hpp

bbbales2 · 2020-11-05T12:58:49Z

@SteveBronder I'm gonna fix the stuff @t4c1 pointed out (just so we aren't doing the same thing at the same time)

…4.1 (tags/RELEASE_600/final)

stan-buildbot · 2020-11-06T12:37:50Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	3.03	3.02	1.01	0.54% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	1.01	1.01% faster
eight_schools/eight_schools.stan	0.12	0.13	0.92	-8.8% slower
gp_regr/gp_regr.stan	0.16	0.17	0.98	-2.25% slower
irt_2pl/irt_2pl.stan	5.66	5.67	1.0	-0.2% slower
performance.compilation	89.93	85.31	1.05	5.14% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	8.48	8.5	1.0	-0.33% slower
pkpd/one_comp_mm_elim_abs.stan	29.92	29.29	1.02	2.13% faster
sir/sir.stan	138.27	135.7	1.02	1.86% faster
gp_regr/gen_gp_data.stan	0.04	0.04	1.0	0.03% faster
low_dim_gauss_mix/low_dim_gauss_mix.stan	2.98	3.0	0.99	-0.63% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.38	0.38	0.99	-0.87% slower
arK/arK.stan	1.77	1.78	0.99	-0.51% slower
arma/arma.stan	0.61	0.6	1.02	1.69% faster
garch/garch.stan	0.74	0.76	0.98	-2.3% slower
Mean result: 0.998499005753

Jenkins Console Log
Blue Ocean
Commit hash: 8dc496c

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-11-06T19:16:31Z

@t4c1 good to go?

Added a few varmat functions (Issue #2101)

77479b6

bbbales2 mentioned this pull request Sep 25, 2020

A few var<mat> function implementations #2102

Closed

5 tasks

bbbales2 and others added 2 commits September 25, 2020 16:59

Removed comments from tcrossprod (Issue #2101)

e97d7c7

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

57e51b4

…4.1 (tags/RELEASE_600/final)

bbbales2 commented Sep 25, 2020

View reviewed changes

t4c1 requested changes Sep 28, 2020

View reviewed changes

bbbales2 and others added 5 commits September 30, 2020 11:33

Review comments (Issue #2101)

e61cce2

Merge commit '2cf4310702eec1b14a355d883e704af0b788abd8' into HEAD

e611bb9

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

0dc1f02

…ASE_601/final)

Switched adj_op() for adj() (Issue #2101)

64cd0d9

Merge branch 'feature/a_few_varmats' of https://github.com/stan-dev/math

6b05cb0

into feature/a_few_varmats

SteveBronder requested changes Sep 30, 2020

View reviewed changes

SteveBronder mentioned this pull request Oct 1, 2020

Allow the var views to compound multiple expressions and remove var.coeff() callbacks #2064

Merged

5 tasks

bbbales2 and others added 6 commits October 1, 2020 13:06

Review comments (Issue #2101)

1f5743f

Merge commit '8e18a041e4901bbf51f021312cb6477295e599e4' into HEAD

0e4f546

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

f609668

…4.1 (tags/RELEASE_600/final)

Optimizing performance (Issue #2101)

515a777

Swapped test order in inverse (Issue #2101)

a773f3d

Merge branch 'feature/a_few_varmats' of https://github.com/stan-dev/math

646cc36

into feature/a_few_varmats

stan-buildbot and others added 6 commits October 1, 2020 20:47

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

d2fe888

…4.1 (tags/RELEASE_600/final)

Fixed cpplint issue with tests (Issue #2101)

db878a6

Merge branch 'feature/a_few_varmats' of https://github.com/stan-dev/math

b9ef0cc

into feature/a_few_varmats

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

b167500

…4.1 (tags/RELEASE_600/final)

Replaced plain_type_t with promote_var_matrix_t (Issue #2101)

68d7b39

Merge branch 'feature/a_few_varmats' of https://github.com/stan-dev/math

e049388

into feature/a_few_varmats

bbbales2 commented Oct 1, 2020

View reviewed changes

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

fb88d3f

…ASE_601/final)

bbbales2 commented Oct 28, 2020

View reviewed changes

SteveBronder and others added 4 commits October 28, 2020 21:45

Add tests for is_rev/var type traits

c676fa6

Merge commit '8464e86fa846d11459c46bc706627583935e7ee3' into HEAD

a4709d0

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

da72d81

…4.1 (tags/RELEASE_600/final)

fix docs

83727cc

bbbales2 and others added 3 commits November 2, 2020 13:31

Merge remote-tracking branch 'origin/develop' into feature/a_few_varmats

3e33abe

fix expr tests for ordered_free and simplex_free

49e4888

Merge remote-tracking branch 'origin/develop' into feature/a_few_varmats

ec4b37e

Merge remote-tracking branch 'origin/develop' into feature/a_few_varmats

2d99d55

SteveBronder previously approved these changes Nov 4, 2020

View reviewed changes

t4c1 requested changes Nov 5, 2020

View reviewed changes

Simplified check_vector, fixed some other stuff (Issue #2101)

6eaaec4

bbbales2 dismissed SteveBronder’s stale review via 6eaaec4 November 5, 2020 13:20

bbbales2 and others added 4 commits November 5, 2020 08:22

Merge remote-tracking branch 'origin/develop' into feature/a_few_varmats

1ac5567

Removed check_vector for unit_vector_constrain (Issue #2101)

4c0596f

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

a500373

…4.1 (tags/RELEASE_600/final)

merge to develop

8dc496c

t4c1 approved these changes Nov 6, 2020

View reviewed changes

bbbales2 merged commit 9b1abaa into develop Nov 6, 2020


		T_return res = (arena_x_val.array() * arena_x_val.array()).colwise().sum();

		if (x.size() > 0) {

	arena_t<Eigen::MatrixXd> arena_x_val = value_of(arena_x);
	arena_t<decltype(value_of(arena_x))> arena_x_val = value_of(arena_x);

Added a few varmat functions (Issue #2101) #2106

Added a few varmat functions (Issue #2101) #2106

Conversation

bbbales2 commented Sep 25, 2020

Summary

Side Effects

Release notes

Checklist

bbbales2 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

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 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

bbbales2 commented Oct 1, 2020 • edited Loading

bbbales2 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

SteveBronder commented Oct 2, 2020

bbbales2 commented Oct 2, 2020

bbbales2 commented Oct 2, 2020 • edited Loading

SteveBronder commented Oct 28, 2020

Choose a reason for hiding this comment

bbbales2 commented Oct 30, 2020

rok-cesnovar commented Oct 30, 2020

SteveBronder commented Oct 30, 2020

rok-cesnovar commented Oct 30, 2020 • edited Loading

stan-buildbot commented Nov 4, 2020

SteveBronder left a comment

Choose a reason for hiding this comment

stan-buildbot commented Nov 5, 2020

bbbales2 commented Nov 5, 2020

stan-buildbot commented Nov 6, 2020

SteveBronder commented Nov 6, 2020

bbbales2 commented Oct 1, 2020 •

edited

Loading

bbbales2 commented Oct 2, 2020 •

edited

Loading

rok-cesnovar commented Oct 30, 2020 •

edited

Loading