Issues with templated matrix functions

[Daniel Lee]

We're having some trouble with the templated matrix functions on Windows using g++ 4.6.3 distributed with Rtools 215. The issues only started happening when I added the -std=c++0x flags to the tests.

A couple of the functions we are having trouble with are elt_multiply() and subtract(). The functions are defined in src/stan/math/matrix.hpp and src/stan/agrad/matrix.hpp.

These issues may have also caused bus errors when compiling with the macports g++ 4.6.3 compiler with optimization level 0 on Mac OS X.

I think the compiler under Windows is actually doing the right thing in this case and the other compilers (clang++ on Mac, macports g++ 4.6.3 on Mac) are not as strict. There are templated functions in the stan::agrad namespace that conflicts with the function definitions in stan::math namespace.

For elt_multiply(), the math/matrix.hpp version's signature looks like:

   vector_d elt_multiply(const vector_d& v1, const vector_d& v2);

and the agrad/matrix.hpp version's signature looks like:

    template <typename T1, typename T2>

    inline Eigen::Matrix<stan::agrad::var,Eigen::Dynamic,1>

    elt_multiply(const Eigen::Matrix<T1,Eigen::Dynamic,1>& v1,

                 const Eigen::Matrix<T2,Eigen::Dynamic,1>& v2) 

When T1 and T2 are double, the function is defined twice for double vector inputs. I believe this is the cause of the problem in Windows, and it may be the cause of the mysterious bus errors in Mac (but the bus error could be something else).

With all that said, I think the solution to these problems is to:

1) rewrite most of the matrix functionality using templates 

2) add tests that instantiates all the different permutations of the inputs and outputs to the functions.

Any takers? Can anyone on another OS verify that the agrad/matrix test fails? 

I think Ben and Marcus are the closest to this code, but if there aren't any volunteers, this is critical enough for me to pick it up to starting tomorrow.

Daniel

 

[Ben Goodrich]

On Tuesday, November 20, 2012 9:07:02 PM UTC-5, Daniel Lee wrote:

We're having some trouble with the templated matrix functions on Windows using g++ 4.6.3 distributed with Rtools 215. The issues only started happening when I added the -std=c++0x flags to the tests.

We seem to have a mix of -std=c++0x and -std=gnu0x going on

goodrich@CYBERPOWERPC:/opt/stan$ git grep -n "std" make/ | cat
make/os_linux:13:  CFLAGS_GTEST += -std=gnu++0x
make/os_linux:19:  CFLAGS_GTEST += -std=gnu++0x
make/os_linux:21:#  LDLIBS += -static-libstdc++
make/os_mac:11:  CFLAGS_GTEST += -std=gnu++0x
make/os_win:22:  CFLAGS_GTEST += -std=c++0x
make/os_win:29:#  CFLAGS_GTEST += -std=c++0x
make/os_win:31:#  LDLIBS += -static-libstdc++

which we should probably harmonize.
 

A couple of the functions we are having trouble with are elt_multiply() and subtract(). The functions are defined in src/stan/math/matrix.hpp and src/stan/agrad/matrix.hpp.

These issues may have also caused bus errors when compiling with the macports g++ 4.6.3 compiler with optimization level 0 on Mac OS X.

I think the compiler under Windows is actually doing the right thing in this case and the other compilers (clang++ on Mac, macports g++ 4.6.3 on Mac) are not as strict. There are templated functions in the stan::agrad namespace that conflicts with the function definitions in stan::math namespace.

For elt_multiply(), the math/matrix.hpp version's signature looks like:

   vector_d elt_multiply(const vector_d& v1, const vector_d& v2);

and the agrad/matrix.hpp version's signature looks like:

    template <typename T1, typename T2>

    inline Eigen::Matrix<stan::agrad::var,Eigen::Dynamic,1>

    elt_multiply(const Eigen::Matrix<T1,Eigen::Dynamic,1>& v1,

                 const Eigen::Matrix<T2,Eigen::Dynamic,1>& v2) 

When T1 and T2 are double, the function is defined twice for double vector inputs. I believe this is the cause of the problem in Windows, and it may be the cause of the mysterious bus errors in Mac (but the bus error could be something else).

I was under the impression that the above was valid C++ and that the compiler should select the more specific definition of the function, but maybe that is not right.
 

With all that said, I think the solution to these problems is to:

1) rewrite most of the matrix functionality using templates 

2) add tests that instantiates all the different permutations of the inputs and outputs to the functions.

Any takers? Can anyone on another OS verify that the agrad/matrix test fails? 

make test-unit with CC=g++-4.6 works for me. I can't run the tests under cross-compilation right now, but that appears to be an unrelated problem.

Can't we just eliminate the versions that are defined for doubles?
 

I think Ben and Marcus are the closest to this code, but if there aren't any volunteers, this is critical enough for me to pick it up to starting tomorrow.

I can do it, but not having Windows would make it a bit harder to trigger failures such as this.

Ben
 

[Daniel Lee]

On Tue, Nov 20, 2012 at 11:01 PM, Ben Goodrich <goodri...@gmail.com> wrote:

On Tuesday, November 20, 2012 9:07:02 PM UTC-5, Daniel Lee wrote:

We're having some trouble with the templated matrix functions on Windows using g++ 4.6.3 distributed with Rtools 215. The issues only started happening when I added the -std=c++0x flags to the tests.

We seem to have a mix of -std=c++0x and -std=gnu0x going on

goodrich@CYBERPOWERPC:/opt/stan$ git grep -n "std" make/ | cat
make/os_linux:13:  CFLAGS_GTEST += -std=gnu++0x
make/os_linux:19:  CFLAGS_GTEST += -std=gnu++0x
make/os_linux:21:#  LDLIBS += -static-libstdc++
make/os_mac:11:  CFLAGS_GTEST += -std=gnu++0x
make/os_win:22:  CFLAGS_GTEST += -std=c++0x
make/os_win:29:#  CFLAGS_GTEST += -std=c++0x
make/os_win:31:#  LDLIBS += -static-libstdc++

which we should probably harmonize.

Yeah, sorry about that. I was trying out both to see if we could get it to compile under either set of flags. I'll switch them all to c++0x.

 

 

A couple of the functions we are having trouble with are elt_multiply() and subtract(). The functions are defined in src/stan/math/matrix.hpp and src/stan/agrad/matrix.hpp.

These issues may have also caused bus errors when compiling with the macports g++ 4.6.3 compiler with optimization level 0 on Mac OS X.

I think the compiler under Windows is actually doing the right thing in this case and the other compilers (clang++ on Mac, macports g++ 4.6.3 on Mac) are not as strict. There are templated functions in the stan::agrad namespace that conflicts with the function definitions in stan::math namespace.

For elt_multiply(), the math/matrix.hpp version's signature looks like:

   vector_d elt_multiply(const vector_d& v1, const vector_d& v2);

and the agrad/matrix.hpp version's signature looks like:

    template <typename T1, typename T2>

    inline Eigen::Matrix<stan::agrad::var,Eigen::Dynamic,1>

    elt_multiply(const Eigen::Matrix<T1,Eigen::Dynamic,1>& v1,

                 const Eigen::Matrix<T2,Eigen::Dynamic,1>& v2) 

When T1 and T2 are double, the function is defined twice for double vector inputs. I believe this is the cause of the problem in Windows, and it may be the cause of the mysterious bus errors in Mac (but the bus error could be something else).

I was under the impression that the above was valid C++ and that the compiler should select the more specific definition of the function, but maybe that is not right.

There are subtle issues with templated functions, their specifications, and overloaded functions with the same function signature. With the c++0x flag turned on, it seems like the templated function is instantiated differently than without the flag. I've been reading through the C++ spec and I don't think the spec is clear on whether the compiler is supposed to find the regular overloaded function first or if it should try to instantiate the template function instead.

 

With all that said, I think the solution to these problems is to:

1) rewrite most of the matrix functionality using templates 

2) add tests that instantiates all the different permutations of the inputs and outputs to the functions.

Any takers? Can anyone on another OS verify that the agrad/matrix test fails? 

make test-unit with CC=g++-4.6 works for me. I can't run the tests under cross-compilation right now, but that appears to be an unrelated problem.

Can't we just eliminate the versions that are defined for doubles?

Almost. The return type should change to be a promoted version of the input arguments so that when 2 double arguments are passed in, the resulting type should be double. In the case of elt_multiply(), we really don't need to have different template functions for a matrix, vector, and row_vector. We should be able to define one templated function to take care of all our cases.

  

I think Ben and Marcus are the closest to this code, but if there aren't any volunteers, this is critical enough for me to pick it up to starting tomorrow.

I can do it, but not having Windows would make it a bit harder to trigger failures such as this.

Ok. If you're not triggering it through Linux, I can start tackling it on the Windows box. By the way, I'm just remoting into a different machine to do it.

[Ben Goodrich]

On Wednesday, November 21, 2012 9:18:37 AM UTC-5, Daniel Lee wrote:

Yeah, sorry about that. I was trying out both to see if we could get it to compile under either set of flags. I'll switch them all to c++0x.

With --std=c++0x, I get a bunch of test failures under g++-4.7. Log attached.

Ben

[Ben Goodrich]

But it is the same with --std=gnu++0x.

Ben

[Daniel Lee]

Interesting. Looks like something weird happens with the parser with those flags. We'll get to those issues after refactoring the matrix functions.

I've created a remote branch called vectorize_matrix, if anyone wants to help with refactoring it. I'll make sure to push/pull often.

 

[Daniel Lee]

Sorry, that's "refactor_matrix".

[Bob Carpenter]

I'll check in a fix for elt_multiply ASAP
and if it works get back to the list. More below.

On 11/20/12 11:01 PM, Ben Goodrich wrote:
> On Tuesday, November 20, 2012 9:07:02 PM UTC-5, Daniel Lee wrote:
> I think the compiler under Windows is actually doing the right thing in this case and the other compilers (clang++
> on Mac, macports g++ 4.6.3 on Mac) are not as strict.

Absolutely. Our code below is wrong.

> There are templated functions in the stan::agrad namespace
> that conflicts with the function definitions in stan::math namespace.
>
> For elt_multiply(), the math/matrix.hpp version's signature looks like:
> vector_d elt_multiply(const vector_d& v1, const vector_d& v2);
>
> and the agrad/matrix.hpp version's signature looks like:
> template <typename T1, typename T2>
> inline Eigen::Matrix<stan::agrad::var,Eigen::Dynamic,1>
> elt_multiply(const Eigen::Matrix<T1,Eigen::Dynamic,1>& v1,
> const Eigen::Matrix<T2,Eigen::Dynamic,1>& v2)

> When T1 and T2 are double, the function is defined twice for double vector inputs. I believe this is the cause of
> the problem in Windows, and it may be the cause of the mysterious bus errors in Mac (but the bus error could be
> something else).

> I was under the impression that the above was valid C++ and that the compiler should select the more specific definition
> of the function, but maybe that is not right.

Functions can only have full specializations. Otherwise,
it's just overloading. Here, there's an extra layer of
namespace difference.

I'm not sure what's supposed to happen if two namespaces
both contain matching functions.

Also, the signature of the second one doesn't really specialize
the first one because of the var return type.

> With all that said, I think the solution to these problems is to:
> 1) rewrite most of the matrix functionality using templates
> 2) add tests that instantiates all the different permutations of the inputs and outputs to the functions.
>
> Any takers? Can anyone on another OS verify that the agrad/matrix test fails?

This will definitely solve the problem.
The return type on the templated versions should use
promotion, and then it can all be done in math/matrix
and we're good to go.

In fact, I think we can just use one templated version
for all of these in math/matrix and be done with it.

> make test-unit with CC=g++-4.6 works for me. I can't run the tests under cross-compilation right now, but that appears
> to be an unrelated problem.
>
> Can't we just eliminate the versions that are defined for doubles?

Yes, if you use promotion for the return types.

> I think Ben and Marcus are the closest to this code, but if there aren't any volunteers, this is critical enough for
> me to pick it up to starting tomorrow.
>
>
> I can do it, but not having Windows would make it a bit harder to trigger failures such as this.

I'm taking a whack at it right now. Shouldn't take
long at all.

- Bob