Pushed: optimized multiply() and dot_product(); added mdivide_left functions
Marcus Brubaker
Cheers,
Marcus
Bob Carpenter
On 4/10/12 2:03 PM, Marcus Brubaker wrote:
> So I've pushed the changes which added support for Eigen::Matrix types
> to the vari for dot_product() and implement multiply() in terms of
> dot_product(). Also included was the addition of mdivide_left and
> mdivide_left_tri.
Awesome! I assume you still go the overall speedup
from doing it the long way (thanks for that, by the way).
> I haven't implemented mdivide_right yet but will done
> that soon.
Let me know when you do and I'll add the operators
to the parser and to the doc.
- Bob
Marcus Brubaker
Awesome! I assume you still go the overall speedup
On 4/10/12 2:03 PM, Marcus Brubaker wrote:
So I've pushed the changes which added support for Eigen::Matrix types
to the vari for dot_product() and implement multiply() in terms of
dot_product(). Also included was the addition of mdivide_left and
mdivide_left_tri.
from doing it the long way (thanks for that, by the way).
Yes, this version keeps the significant speedups. I found a way to do the templates which didn't have the problems I had run into when I removed that version of multiply.
Let me know when you do and I'll add the operators
I haven't implemented mdivide_right yet but will done
that soon.
to the parser and to the doc.
Will do.
Cheers,
Marcus
Ben Goodrich
In file included from src/test/agrad/matrix_test.cpp:7:
src/stan/agrad/matrix.hpp:1345:25: error: no matching function for call to 'dot_product'
result(i,j) = dot_product(crow,m2.col(j).eval());
^~~~~~~~~~~
src/test/agrad/matrix_test.cpp:1653:21: note: in instantiation of function template specialization 'stan::agrad::multiply<stan::agrad::var, -1, 1, stan::agrad::var, 1, -1>' requested here
matrix_v output = stan::agrad::multiply(v1, v2);
^
src/stan/agrad/matrix.hpp:611:16: note: candidate function not viable: no known conversion from 'Eigen::Matrix<var, 1, 1>' to 'const std::vector<var>' for 1st argument;
inline var dot_product(const std::vector<var>& v1,
^
src/stan/agrad/matrix.hpp:622:16: note: candidate function not viable: no known conversion from 'Eigen::Matrix<var, 1, 1>' to 'const std::vector<var>' for 1st argument;
inline var dot_product(const std::vector<var>& v1,
^
src/stan/agrad/matrix.hpp:633:16: note: candidate function not viable: no known conversion from 'Eigen::Matrix<var, 1, 1>' to 'const std::vector<double>' for 1st argument;
inline var dot_product(const std::vector<double>& v1,
^
src/stan/agrad/matrix.hpp:531:16: note: candidate template ignored: failed template argument deduction
inline var dot_product(const Eigen::Matrix<var, R1, C1>& v1,
^
src/stan/agrad/matrix.hpp:550:16: note: candidate template ignored: failed template argument deduction
inline var dot_product(const Eigen::Matrix<double, R1, C1>& v1,
^
src/stan/agrad/matrix.hpp:569:16: note: candidate template ignored: failed template argument deduction
inline var dot_product(const Eigen::Matrix<double, R1, C1>& v1,
^
src/stan/agrad/matrix.hpp:512:16: note: candidate template ignored: substitution failure [with R1 = 1, C1 = 1, R2 = 1, C2 = 1]
inline var dot_product(const Eigen::Matrix<var, R1, C1>& v1,
^
src/stan/agrad/matrix.hpp:603:16: note: candidate function not viable: requires 3 arguments, but 2 were provided
inline var dot_product(const double* v1, const var* v2, size_t length) {
^
src/stan/agrad/matrix.hpp:594:16: note: candidate function not viable: requires 3 arguments, but 2 were provided
inline var dot_product(const var* v1, const double* v2, size_t length) {
^
src/stan/agrad/matrix.hpp:585:16: note: candidate function not viable: requires 3 arguments, but 2 were provided
inline var dot_product(const var* v1, const var* v2, size_t length) {
^
src/stan/agrad/matrix.hpp:1345:25: error: no matching function for call to 'dot_product'
result(i,j) = dot_product(crow,m2.col(j).eval());
^~~~~~~~~~~
src/test/agrad/matrix_test.cpp:1666:12: note: in instantiation of function template specialization 'stan::agrad::multiply<stan::agrad::var, -1, 1, double, 1, -1>' requested here
output = stan::agrad::multiply(v1, d2);
^
src/stan/agrad/matrix.hpp:611:16: note: candidate function not viable: no known conversion from 'Eigen::Matrix<var, 1, 1>' to 'const std::vector<var>' for 1st argument;
inline var dot_product(const std::vector<var>& v1,
^
src/stan/agrad/matrix.hpp:622:16: note: candidate function not viable: no known conversion from 'Eigen::Matrix<var, 1, 1>' to 'const std::vector<var>' for 1st argument;
inline var dot_product(const std::vector<var>& v1,
^
src/stan/agrad/matrix.hpp:633:16: note: candidate function not viable: no known conversion from 'Eigen::Matrix<var, 1, 1>' to 'const std::vector<double>' for 1st argument;
inline var dot_product(const std::vector<double>& v1,
^
src/stan/agrad/matrix.hpp:550:16: note: candidate template ignored: failed template argument deduction
inline var dot_product(const Eigen::Matrix<double, R1, C1>& v1,
^
src/stan/agrad/matrix.hpp:569:16: note: candidate template ignored: failed template argument deduction
inline var dot_product(const Eigen::Matrix<double, R1, C1>& v1,
^
src/stan/agrad/matrix.hpp:531:16: note: candidate template ignored: substitution failure [with R1 = 1, C1 = 1, R2 = 1, C2 = 1]
inline var dot_product(const Eigen::Matrix<var, R1, C1>& v1,
^
src/stan/agrad/matrix.hpp:512:16: note: candidate template ignored: failed template argument deduction
inline var dot_product(const Eigen::Matrix<var, R1, C1>& v1,
^
src/stan/agrad/matrix.hpp:603:16: note: candidate function not viable: requires 3 arguments, but 2 were provided
inline var dot_product(const double* v1, const var* v2, size_t length) {
^
src/stan/agrad/matrix.hpp:594:16: note: candidate function not viable: requires 3 arguments, but 2 were provided
inline var dot_product(const var* v1, const double* v2, size_t length) {
^
src/stan/agrad/matrix.hpp:585:16: note: candidate function not viable: requires 3 arguments, but 2 were provided
inline var dot_product(const var* v1, const var* v2, size_t length) {
^
src/stan/agrad/matrix.hpp:1345:25: error: no matching function for call to 'dot_product'
result(i,j) = dot_product(crow,m2.col(j).eval());
^~~~~~~~~~~
src/test/agrad/matrix_test.cpp:1679:12: note: in instantiation of function template specialization 'stan::agrad::multiply<double, -1, 1, stan::agrad::var, 1, -1>' requested here
output = stan::agrad::multiply(d1, v2);
^
src/stan/agrad/matrix.hpp:611:16: note: candidate function not viable: no known conversion from 'Eigen::Matrix<double, 1, 1>' to 'const std::vector<var>' for 1st argument;
inline var dot_product(const std::vector<var>& v1,
^
src/stan/agrad/matrix.hpp:622:16: note: candidate function not viable: no known conversion from 'Eigen::Matrix<double, 1, 1>' to 'const std::vector<var>' for 1st argument;
inline var dot_product(const std::vector<var>& v1,
^
src/stan/agrad/matrix.hpp:633:16: note: candidate function not viable: no known conversion from 'Eigen::Matrix<double, 1, 1>' to 'const std::vector<double>' for 1st argument;
inline var dot_product(const std::vector<double>& v1,
^
src/stan/agrad/matrix.hpp:569:16: note: candidate template ignored: failed template argument deduction
inline var dot_product(const Eigen::Matrix<double, R1, C1>& v1,
^
src/stan/agrad/matrix.hpp:550:16: note: candidate template ignored: substitution failure [with R1 = 1, C1 = 1, R2 = 1, C2 = 1]
inline var dot_product(const Eigen::Matrix<double, R1, C1>& v1,
^
src/stan/agrad/matrix.hpp:512:16: note: candidate template ignored: failed template argument deduction
inline var dot_product(const Eigen::Matrix<var, R1, C1>& v1,
^
src/stan/agrad/matrix.hpp:531:16: note: candidate template ignored: failed template argument deduction
inline var dot_product(const Eigen::Matrix<var, R1, C1>& v1,
^
src/stan/agrad/matrix.hpp:603:16: note: candidate function not viable: requires 3 arguments, but 2 were provided
inline var dot_product(const double* v1, const var* v2, size_t length) {
^
src/stan/agrad/matrix.hpp:594:16: note: candidate function not viable: requires 3 arguments, but 2 were provided
inline var dot_product(const var* v1, const double* v2, size_t length) {
^
src/stan/agrad/matrix.hpp:585:16: note: candidate function not viable: requires 3 arguments, but 2 were provided
inline var dot_product(const var* v1, const var* v2, size_t length) {
^
src/stan/agrad/matrix.hpp:1345:25: error: no matching function for call to 'dot_product'
result(i,j) = dot_product(crow,m2.col(j).eval());
^~~~~~~~~~~
src/test/agrad/matrix_test.cpp:1692:12: note: in instantiation of function template specialization 'stan::agrad::multiply<double, -1, 1, double, 1, -1>' requested here
output = stan::agrad::multiply(d1, d2);
^
src/stan/agrad/matrix.hpp:611:16: note: candidate function not viable: no known conversion from 'Eigen::Matrix<double, 1, 1>' to 'const std::vector<var>' for 1st argument;
inline var dot_product(const std::vector<var>& v1,
^
src/stan/agrad/matrix.hpp:622:16: note: candidate function not viable: no known conversion from 'Eigen::Matrix<double, 1, 1>' to 'const std::vector<var>' for 1st argument;
inline var dot_product(const std::vector<var>& v1,
^
src/stan/agrad/matrix.hpp:633:16: note: candidate function not viable: no known conversion from 'Eigen::Matrix<double, 1, 1>' to 'const std::vector<double>' for 1st argument;
inline var dot_product(const std::vector<double>& v1,
^
src/stan/agrad/matrix.hpp:569:16: note: candidate template ignored: substitution failure [with R1 = 1, C1 = 1, R2 = 1, C2 = 1]
inline var dot_product(const Eigen::Matrix<double, R1, C1>& v1,
^
src/stan/agrad/matrix.hpp:512:16: note: candidate template ignored: failed template argument deduction
inline var dot_product(const Eigen::Matrix<var, R1, C1>& v1,
^
src/stan/agrad/matrix.hpp:531:16: note: candidate template ignored: failed template argument deduction
inline var dot_product(const Eigen::Matrix<var, R1, C1>& v1,
^
src/stan/agrad/matrix.hpp:550:16: note: candidate template ignored: failed template argument deduction
inline var dot_product(const Eigen::Matrix<double, R1, C1>& v1,
^
src/stan/agrad/matrix.hpp:603:16: note: candidate function not viable: requires 3 arguments, but 2 were provided
inline var dot_product(const double* v1, const var* v2, size_t length) {
^
src/stan/agrad/matrix.hpp:594:16: note: candidate function not viable: requires 3 arguments, but 2 were provided
inline var dot_product(const var* v1, const double* v2, size_t length) {
^
src/stan/agrad/matrix.hpp:585:16: note: candidate function not viable: requires 3 arguments, but 2 were provided
inline var dot_product(const var* v1, const var* v2, size_t length) {
^
4 errors generated.
make: *** [test/agrad/matrix.o] Error 1
goodrich@CYBERPOWERPC:/opt/stan$
Ben
Marcus Brubaker
It looks like clang++ is having trouble figuring out the template arguments, but I'm honestly not sure why...
Bob Carpenter
to track down. It's so tempting, yet so not-quite-there
as a programming tool. I've been trying to stay away
templating except in cases where we absolutely need it.
The way Marcus did it's pretty cool. I'm also not
sure exactly what's wrong with it, but I have a couple
of hunches.
For what it's worth, clang++ 2.9 is passing all of our tests.
Have you turned off the c++0x flags? I'm hoping there
aren't new language features/changes that are going to mess us up
later. But it'd be nice to know now if that's an issue.
At the very least, we can fix the problem by stepping
back a bit on the templating.
So far, we've only used
Matrix<T,Dynamic,Dynamic> == Stan matrix
Matrix<T,1,Dynamic> == Stan row vector
Matrix<T,Dynamic,1> == Stan (column) vector
and have never tried to template on the Dynamic vs. 1
distinction.
I'm pretty sure we don't want the templated multiply() function
on line 1337 to match except for R1,R2,C1,C2 = Dynamic.
And this template is called out by the error report in clang++ 3.0.
For many of the other cases, we want a different implementation
for efficiency or a real return type.
And does anyone know how Matrix<T,1,1> behaves in Eigen?
I know Matrix<T,1,Dynamic> and Matrix<T,Dynamic,1> have
template specializations (essentially completely independent
classes with the same name at the implementation level)
for row vectors and column vectors.
Just for reference, here's the list of types for
multiply and the binary * operator,
where real is either stan::agrad::var or double
and a mixed var/double op is promoted to a var return type:
(real,real) -> real
(real,vec) -> vec
(real,row_vec) -> row_vec
(real,matrix) -> matrix
(vec,real) -> vec
*(vec,vec)
(vec,row_vec) -> matrix
*(vec,matrix)
(row_vec,real) -> row_vec
(row_vec,vec) -> real
*(row_vec,row_vec)
(row_vec,matrix) -> row_vec
(matrix,real) -> matrix
(matrix,vec) -> vec
*(matrix,row_vec)
(matrix,matrix) -> matrix
This is in the LaTeX doc now, too.
For instance, multiplying (1,Dynamic) by (Dynamic,1)
should return a single var and use a single instance
of dot_product_vv (or _vd or _dv depending on real type).
And multiplying (Dynamic,1) by (1,Dynamic) should
return a matrix where each entry is a multiply object.
Another issue is that we don't want to match
double times double. It's probably not an issue because
there's a more specific template, but it can get
confusing trying to read the code and figure out what's
going to match.
- Bob
On 4/11/12 12:06 AM, Marcus Brubaker wrote:
> Hmmm, ok, I'll take a look tomorrow. I haven't had much luck getting things to work with clang in the past and I've
> stuck with g++, so this may be tricky to debug.
>
> It looks like clang++ is having trouble figuring out the template arguments, but I'm honestly not sure why...
>
Bob Carpenter
The good news is that the core algorithms all look
great and it's just what we want to do. Marcus took
it all the way through matrix mutiplies.
I'll take a pass over it in the next couple of days
and clean up the templating. Mostly, we just need
to remove some of the generality, replace some templates
with explicit stan::agrad::var instantiations, and
get rid of some to_var.
On line 569, we shouldn't have a function to take
two double matrices and return a var. Two doubles
should return a double. Luckily, we already have that
in stan/math/matrix.hpp, so we just need to specialize
a bit.
I think any calls to the functions starting in
line 585 can be replaced with direct calls to the
dot_product..vari constructors, so we can probably just
get rid of these.
I think we've all agreed (at least Matt hasn't objected
yet) to removing the dot_product implementation for
std::vector.
I see a problem with the min implementation on line 648.
We don't want that to be called for doubles -- that
should just return a double value, not promote to a var.
It won't get called because there's an untemplated definition
for double, but we don't want it to be called for int
values, for instance. We've been burned by that before when
the promotion type gets ambiguous (call conversion of
int to double or call conversion of int to var).
So we need to take all these functions
and replace the template variable with stan::agrad::var
and return the to_var calls on the return. At that point,
we can go back to using the typedefs as Ben suggested.
I think that'll go a long way to more readability, too.
By the way, primitive numbers are already assignable
to var through var's implicit constructors for all
the primitive numerical types. That's where the problem
comes from. (We need it that way or we couldn't use
agrad::var and double interchangeably the way we do in code.)
Exp() looks good -- I didn't even know it was in there.
Thanks for putting in all this work. It sounds like a lot,
but this shouldn't be hard to fix.
- Bob
Marcus Brubaker
Also, turning off the c++0x flags actually seems to have fixed my compile errors. It looks like the c++0x features (at least on RHEL6.2) aren't quite ready for prime time. The net upshot of this bug for me is that I now have stan compiling with clang++, which should make my development cycle much faster :)
I too was a bit perplexed by the existence of the agrad::multiply(matrix_d,matrix_d) agrad::dot_product(vector_d,vector_d) functions when I was working on this. However, there are several tests which explicitly rely on this so I left it in assuming that someone had a good reason I couldn't see. I would be happy to remove these variants (and their corresponding tests) if you think they should go.
Cheers,
Marcus
Ben Goodrich
So I just pushed a change which I think should fix the issue that Ben reported. Or at least, it fixes it for me. I'm not entirely sure I understand it, but so it goes.
It looks to be fixed on clang++ 3.0.
Also, turning off the c++0x flags actually seems to have fixed my compile errors. It looks like the c++0x features (at least on RHEL6.2) aren't quite ready for prime time. The net upshot of this bug for me is that I now have stan compiling with clang++, which should make my development cycle much faster :)
Yes, clang has made great strides from 2.8 to 2.9 to 3.0.
Ben
Marcus Brubaker
I agree that we should get rid of them, but someone may have put them there for a reason...
Bob Carpenter
On 4/11/12 1:45 PM, Marcus Brubaker wrote:
> Just to clarify the issue about stan::agrad functions that take two
> double matrices and return a var. Is it OK to remove these? There are
> some tests which depend on this behaviour which would need to be removed
> as well.
Yes, two double matrices need to return a double matrix.
Any other signature will just cause problems.
I hope there's a (double,double) -> double version, but
if not we'll need to add one.
> I agree that we should get rid of them, but someone may have put them
> there for a reason...
Anyone? I don't see how they could be of any use, yet
I see many ways in which they'd be detrimental for everything
from function signature inference to efficiency.
I can remove them and the associated tests and make sure
everything else still works.
And I'll take another pass over everything to make sure
the tests and type inference we need are all in place.
It'll be easy to fix if they're not.
- Bob
Marcus Brubaker
Yes, two double matrices need to return a double matrix.
On 4/11/12 1:45 PM, Marcus Brubaker wrote:
Just to clarify the issue about stan::agrad functions that take two
double matrices and return a var. Is it OK to remove these? There are
some tests which depend on this behaviour which would need to be removed
as well.
Any other signature will just cause problems.
I hope there's a (double,double) -> double version, but
if not we'll need to add one.
It's there, for sure.
Anyone? I don't see how they could be of any use, yet
I agree that we should get rid of them, but someone may have put them
there for a reason...
I see many ways in which they'd be detrimental for everything
from function signature inference to efficiency.
I can remove them and the associated tests and make sure
everything else still works.
And I'll take another pass over everything to make sure
the tests and type inference we need are all in place.
It'll be easy to fix if they're not.
Ok, done and pushed for dot_product() and multiply(). There are a fair number of other functions which need this treatment though..
Cheers,
Marcus