Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mdivide_right_tri copies a lot #706

Closed
wds15 opened this issue Jan 2, 2018 · 4 comments
Closed

mdivide_right_tri copies a lot #706

wds15 opened this issue Jan 2, 2018 · 4 comments
Milestone

Comments

@wds15
Copy link
Contributor

wds15 commented Jan 2, 2018

Summary:

The current mdivide_right_tri functions does lots of copying.

Description:

Currently, the mdivide_right_tri function does a lot of copying as it calls the mdivide_left_tri function with each of its inputs being transposed. As each input gets copied this creates a lot of overhead.

So

row_vector b;
matrix A; //A is lower triangle

mdivide_right_tri(b, A) = b * A;

currently this is mapped to (A' * b')' and evaluated by creating a copy of A and b. We should avoid the copying.

Reproducible Steps:

Please report steps to reproduce the issue. If it's not possible to reproduce, please include a description of how you discovered the issue.

If you have a reproducible example, please include it.

Current Output:

The current output. Knowing what is the current behavior is useful.

Expected Output:

Just run the test ./test/unit//math/prim/mat/fun/mdivide_right_tri_test

Additional Information:

Provide any additional information here.

Current Version:

v2.17.0

@wds15
Copy link
Contributor Author

wds15 commented Jan 2, 2018

uhm... after digging into this I found another variant (and test for it) in our fwd directories (./test/unit/math/fwd/mat/fun/mdivide_right_tri_test). That one is not as easy to fix as for the prim case. Lot's of things are weird (for me)

  • most of the fwd mode tests do not compile because of odd Eigen casting errors (things like YOU_MIXED_DIFFERENT_NUMERIC_TYPES__YOU_NEED_TO_USE_THE_CAST_METHOD_OF_MATRIXBASE_TO_CAST_NUMERIC_TYPES_EXPLICITLY)
  • when disabling most tests, there is one left which fails with an assertion Assertion failed: (other.rows() == 1 || other.cols() == 1), function resizeLike, file lib/eigen_3.3.3/Eigen/src/Core/PlainObjectBase.h, line 369

If someone has an idea here, that would be great. The function can be important and the old version does lots of copying as it does not take advantage of clever Eigen views for the problem.

@bob-carpenter
Copy link
Contributor

Reducing copies would be great. Transposes are much worse than copies as one of the matrices is going to be copied without memory locality.

For the first case, you can't perform operations that mix different scalar types. So no matrix/vector multiplication of var and double or fvar<double> and double. Instead, you can call Stan's functions, which do admit mixing double and autodiff types (but not mixing different types of autodiff types).

The second test is testing for something to be shaped like a row vector or column vector at runtime.

@wds15
Copy link
Contributor Author

wds15 commented Jan 3, 2018

What I really do not understand is that the promote magic works for the mdivide_left_tri function, but it fails when you insert some transponse calls at the right positions whenever you go to fwd. Just compare these:

To me it looks like either the promote magic failing for some reason or some obscurities from Eigen are making this fail.

Now, I have to say that at least for internal code one can work around this issue whenever one has many repeated evaluations of mdivide_right_tri with the same matrix. That is, one can just transpose the matrix a single time and then call mdivide_left_tri<Eigen::Upper>. However, this function is not exposed and hence not a solution to users.

@bob-carpenter
Copy link
Contributor

bob-carpenter commented Jan 3, 2018 via email

@seantalts seantalts added this to the 2.18.0 milestone Jul 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants