You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
@bob-carpenter has a nice discourse thread discussing how a static_matrix type would work in the stan language. This idea has come up again now for efficient autodiff with Eigen #1785 and I think is the path forward to use the OpenCL backend for autodiff #1639
My main question is how we would go about implementing this. Should we make a stan Matrix type that for doubles just inherits from Eigen and for var is two Eigen matrices? Something like
One other fun idea with this is to add an ExecutionPolicy template argument sort of like in std parallel. This would be an enum with values like sequential, parallel_opencl, parallel_tbb etc. and could allow for pretty simple swaps of execution backends.
I think the first suggestion could be simplified a bit by writing a specialization for Eigen::Matrix<var>. Also we can make non-static version with second member Eigen::Matrix<vari*, -1, -1> adj_;. This is not as efficient as what you suggested, but can be modified.
The second suggestion (ExecutionPolicy) seems very complex.
I think the first suggestion could be simplified a bit by writing a specialization for Eigen::Matrix. Also we can make non-static version with second member Eigen::Matrix<vari*, -1, -1> adj_;. This is not as efficient as what you suggested, but can be moddified.
I'm not sure what you have in mind here can you elaborate?
The second suggestion (ExecutionPolicy) seems very complex.
Maybe I need to think about it
Why not just something like the following, which would work for matrices, vectors, and row vectors?
Yeah mine was just pseudocode
For the dynamic case, both members need to be Matrix<vari*, ...> because both would change under an assignment.
I thought for static matrices we wanted to not allow assignment after the first declaration? In my mind this would never be used by users, but the compiler would do an optimization sweep and if a matrix was never assigned after first initialization it would change it from a normal matrix to a static matrix
I thought for static matrices we wanted to not allow assignment after the first declaration?
That's for the dynamic case, not the static case. We do.
I'm in favor of enabling users to write efficient code rather than hoping the compiler can sort it out. But this might be a case where we can guarantee the compiler finds all the relevant optimizations.
Also, I was wrong about need two vari* --- just one of them has the value and the adjoint in it. That's essentially what we do with var, which is just a pointer to implementation (PIMPL) of the vari.
Was going to make a separate issue, but in a side email Bob brought up a point that if we templated var and vari ala
template <typename T>
structvar {
vari<T>* vi_;
}
template <typename T>
structvari {
T val_;
T adj_;
}
This would work nicely for static matrices via var<Eigen::Matrix<double, -1, -1>>. One of the reasons I've harked and barked about C++17 is that making this change now would break essentially everything, but with c++17 templates can be deduced from the constructor so having a template on var would be fully backwards compatible
The example below is a little more far out / groovy, but we can also pass through our vari classes as a template. I think that would let us do some pretty fun things
@t4c1 do you think our first step here is just to make the var and vari templates and generically just do var<double> / vari<double> everywhere? Then once we do that we can slowly unwind everything to actually take template types
I don't think we want any backward-compatibility breaking moves from this.
tbh I'm not really sure how we do this without breaking backwards compatibility in some form. We either have to move up to C++17 for the template constructor deduction or have at least var<> / var<double> everywhere
I'm mixed about it, we can discuss it in the meeting tmrw. This is a really nice solution to have pretty much everything use the OpenCL backend as well
Activity
t4c1 commentedon Mar 30, 2020
I think the first suggestion could be simplified a bit by writing a specialization for
Eigen::Matrix<var>. Also we can make non-static version with second memberEigen::Matrix<vari*, -1, -1> adj_;. This is not as efficient as what you suggested, but can be modified.The second suggestion (
ExecutionPolicy) seems very complex.bob-carpenter commentedon Mar 30, 2020
Why not just something like the following, which would work for matrices, vectors, and row vectors?
For the dynamic case, both members need to be
Matrix<vari*, ...>because both would change under an assignment.SteveBronder commentedon Mar 30, 2020
I'm not sure what you have in mind here can you elaborate?
Maybe I need to think about it
Yeah mine was just pseudocode
I thought for static matrices we wanted to not allow assignment after the first declaration? In my mind this would never be used by users, but the compiler would do an optimization sweep and if a matrix was never assigned after first initialization it would change it from a normal matrix to a static matrix
bob-carpenter commentedon Mar 30, 2020
That's for the dynamic case, not the static case. We do.
I'm in favor of enabling users to write efficient code rather than hoping the compiler can sort it out. But this might be a case where we can guarantee the compiler finds all the relevant optimizations.
Also, I was wrong about need two
vari*--- just one of them has the value and the adjoint in it. That's essentially what we do withvar, which is just a pointer to implementation (PIMPL) of thevari.SteveBronder commentedon Apr 22, 2020
Was going to make a separate issue, but in a side email Bob brought up a point that if we templated var and vari ala
This would work nicely for static matrices via
var<Eigen::Matrix<double, -1, -1>>. One of the reasons I've harked and barked about C++17 is that making this change now would break essentially everything, but with c++17 templates can be deduced from the constructor so having a template on var would be fully backwards compatibleThe example below is a little more far out / groovy, but we can also pass through our vari classes as a template. I think that would let us do some pretty fun things
https://godbolt.org/z/rZJJld
SteveBronder commentedon Apr 22, 2020
Hmm,
var<matrix_cl<double>>lol, that seems pretty cool to me!
t4c1 commentedon Apr 22, 2020
I guess there will be some rough edgess to smooth. But this doees look like the simplest way forward.
SteveBronder commentedon Apr 22, 2020
@t4c1 do you think our first step here is just to make the var and vari templates and generically just do
var<double>/vari<double>everywhere? Then once we do that we can slowly unwind everything to actually take template typesSteveBronder commentedon Apr 22, 2020
Also I'm going to bring this up in the stan meeting tmrw, maybe we could also host an impromptu stan math meeting about it
bob-carpenter commentedon Apr 22, 2020
I'm not sure what you mean. What would the
varandvariclasses look like?I don't think we want any backward-compatibility breaking moves from this.
SteveBronder commentedon Apr 22, 2020
Ack markdown got rid of my brackets, just added them
SteveBronder commentedon Apr 22, 2020
tbh I'm not really sure how we do this without breaking backwards compatibility in some form. We either have to move up to C++17 for the template constructor deduction or have at least
var<>/var<double>everywherebob-carpenter commentedon Apr 22, 2020
Then I think we should wait for C++17.
SteveBronder commentedon Apr 22, 2020
I'm mixed about it, we can discuss it in the meeting tmrw. This is a really nice solution to have pretty much everything use the OpenCL backend as well
24 remaining items
Added Phi comment (Issue #1805)
Updated doxygen a bit (Issue #1805)
Added missing comma (Issue #1805)
Fixed initialization in constrain mix tests (Issue #1805)
Removed a couple rep_matrix specializations and tests (Issue #1805)
Uncommented test (Issue #1805)
Added prim tests for offset_multiplier (Issue #1805)
Adding tests (Issue #1805)
Added consistent dimensions check (Issue #1805)
Added dimension checks and tests (Issue #1805)
Updated test names to not collide (Issue #1805)
Added missing test (Issue #1805)
Part of merge (Issue #1805)
Check return type is double (Issue #1805)