matrix_exp_multiply seg faults

[syclik]

Description

matrix_exp_multiply, as written, currently seg faults.

Example

This test current seg faults on my machine. I think it may be memory dependent, but not certain.

#include <stan/math/rev/mat.hpp>
#include <gtest/gtest.h>

TEST(MathRev, matrix_multiply_exp__vd) {
  Eigen::Matrix<stan::math::var, -1, -1> A(5, 5);
  A << -0.96871, 0.398827, 0.241306, 0.741373, 0.108926,
    0.888077, -0.915624, -0.373344, 0.255238, 0.717304,
    -0.0899219, -0.898862, -0.800546, -0.222652, -0.271382,
    0.683227, 0.827031, -0.780702, -0.104228, 0.885106,
    -0.996585, -0.097802, 0.739617, 0.235266, -0.0247717;


  Eigen::Matrix<double, -1, -1> B(5, 5);
  B << -0.96871, 0.398827, 0.241306, 0.741373, 0.108926, 0.888077, -0.915624,
    -0.373344, 0.255238, 0.717304, -0.0899219, -0.898862, -0.800546,
    -0.222652, -0.271382, 0.683227, 0.827031, -0.780702, -0.104228, 0.885106,
    -0.996585, -0.097802, 0.739617, 0.235266, -0.0247717;

  Eigen::Matrix<stan::math::var, -1, -1> result = stan::math::matrix_exp_multiply(A, B);

  std::vector<double> gradients;
  stan::math::print_stack(std::cout);

  std::vector<stan::math::var> vars = stan::math::to_array_1d(A);
  result(0, 0).grad(vars, gradients);

  std::cout << "gradients = " << std::endl;
  for (size_t j = 0; j < gradients.size(); ++j)
    std::cout << gradients[j] << " ";
  std::cout << std::endl;

  FAIL() << "There are no asserts in the tests." << std::endl
         << "This test seg faults with the existing implementation." <<	std::endl;
}

Expected Output

Not seg fault. Return the same result as doing the actions separately.

Additional Information

Thank you, @rok-cesnovar, for pushing the Windows tests to the point where we were able to find this.

@yizhang-cae, can you look at the implementation and help identify why it's seg faulting? I'm going to patch this by using the naive implementation.

There is a new branch: bugfix/issue-1073-matrix_exp_mulitply and the failing test has been included to the test/unit/math/rev/mat/fun/matrix_exp_multiply_test.cpp file. If you run:

$ ./runTests.py test/unit/math/rev/mat/fun/matrix_exp_multiply_test.cpp 

it should fail.

Current Math Version

v2.18.0

[bob-carpenter]

This can't be the permanent test because (a) tests shouldn't write to standard out, and (b) should succeed when the code works. For (b), I don't see a way this test could succeed---if it doesn't segfault, it hits the FAIL() macro.

[syclik]

See the updated branch. I put that original test in there to demonstrate the seg fault. It's been updated already.

[syclik]

@rok-cesnovar, I just verified that this patch gets us past the matrix_exp_multiply problem in Windows. Thank you for helping with that PR... it's been a lot of help. (even though it's been slow; I didn't expect we'd run into a seg fault issue that is trapped by Windows and not Mac or Linux.)

[rok-cesnovar]

No problem, glad too help. Besides this, all the remaining Windows issue were fixed so I think we are clear! Great stuff!

[yizhang-yiz]

Sorry for being late on this. Is the failure compiler/platform dependent? Because on my mac

test/unit/math/rev/mat/fun/matrix_exp_multiply_test --gtest_output="xml:test/unit/math/rev/mat/fun/matrix_exp_multiply_test.xml"
Running main() from lib/gtest_1.8.1/src/gtest_main.cc
[==========] Running 5 tests from 2 test cases.
[----------] Global test environment set-up.
[----------] 1 test from MathRev
[ RUN      ] MathRev.matrix_multiply_exp__vd__segfault
[       OK ] MathRev.matrix_multiply_exp__vd__segfault (0 ms)
[----------] 1 test from MathRev (0 ms total)

[----------] 4 tests from MathMatrix
[ RUN      ] MathMatrix.matrix_exp_multiply_dv
[       OK ] MathMatrix.matrix_exp_multiply_dv (1 ms)
[ RUN      ] MathMatrix.matrix_exp_multiply_vd
[       OK ] MathMatrix.matrix_exp_multiply_vd (4 ms)
[ RUN      ] MathMatrix.matrix_exp_multiply_vv
[       OK ] MathMatrix.matrix_exp_multiply_vv (15 ms)
[ RUN      ] MathMatrix.matrix_exp_multiply_exception
[       OK ] MathMatrix.matrix_exp_multiply_exception (0 ms)
[----------] 4 tests from MathMatrix (20 ms total)

[----------] Global test environment tear-down
[==========] 5 tests from 2 test cases ran. (20 ms total)
[  PASSED  ] 5 tests.

[syclik]

The PR already has a fix on it. Can you reproduce it if you only include the first commit in the PR? And it's memory dependent, so running multiple times may result in it working or not.

…

On Wed, Dec 5, 2018 at 12:51 PM Yi Zhang ***@***.***> wrote: Sorry for being late on this. Is the failure compiler/platform dependent? Because on my mac test/unit/math/rev/mat/fun/matrix_exp_multiply_test --gtest_output="xml:test/unit/math/rev/mat/fun/matrix_exp_multiply_test.xml" Running main() from lib/gtest_1.8.1/src/gtest_main.cc [==========] Running 5 tests from 2 test cases. [----------] Global test environment set-up. [----------] 1 test from MathRev [ RUN ] MathRev.matrix_multiply_exp__vd__segfault [ OK ] MathRev.matrix_multiply_exp__vd__segfault (0 ms) [----------] 1 test from MathRev (0 ms total) [----------] 4 tests from MathMatrix [ RUN ] MathMatrix.matrix_exp_multiply_dv [ OK ] MathMatrix.matrix_exp_multiply_dv (1 ms) [ RUN ] MathMatrix.matrix_exp_multiply_vd [ OK ] MathMatrix.matrix_exp_multiply_vd (4 ms) [ RUN ] MathMatrix.matrix_exp_multiply_vv [ OK ] MathMatrix.matrix_exp_multiply_vv (15 ms) [ RUN ] MathMatrix.matrix_exp_multiply_exception [ OK ] MathMatrix.matrix_exp_multiply_exception (0 ms) [----------] 4 tests from MathMatrix (20 ms total) [----------] Global test environment tear-down [==========] 5 tests from 2 test cases ran. (20 ms total) [ PASSED ] 5 tests. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1073 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZ_F7JZXu7LrM-HwK6F-EDPBqLLGLmAks5u2AeOgaJpZM4ZBCJ2> .

[syclik]

@yizhang-cae: to be more specific, can you check out this commit and see if it fails? 1784c1c. That's the first one that includes the test (and leaves the code alone).

[yizhang-yiz]

You're right. I can reproduce the failure. I'm looking for the cause.

[syclik]

Thanks. I've searched for it for a while and I don't know... if this PR looks ok, can we merge it first so we can keep moving forward with fixing things for Windows? (You should be able to revert the merge that removes everything to start from there in one step.)

…

On Thu, Dec 6, 2018 at 2:57 PM Yi Zhang ***@***.***> wrote: You're right. I can reproduce the failure. I'm looking for the cause. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1073 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZ_F7qig-TnsnJSm4JneiWjhKXRmxMwks5u2XaJgaJpZM4ZBCJ2> .

[yizhang-yiz]

I've added a branch https://github.com/yizhang-cae/math/tree/matrix_multiply_seg_fault
to show that seg fault can be introduced to multiply function if two lines are added. In that branch run

./runTests.py test/unit/math/rev/mat/fun/multiply_failure

will produce a seg fault.

[syclik]

Cool. Thanks for creating that. Maybe it's the creation of an Eigen::Matrix<var, -1, -1> inside the vari implementation that does it? Instead of using random inside, does it also happen if you just set the first value to 0.0?

…

On Fri, Dec 7, 2018 at 1:50 PM Yi Zhang ***@***.***> wrote: I've added a branch https://github.com/yizhang-cae/math/tree/matrix_multiply_seg_fault to show that seg fault can be introduced to multiply function if two lines are added. In that branch run ./runTests.py test/unit/math/rev/mat/fun/multiply_failure will produce a seg fault. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1073 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZ_FwyJ0lNAEKDoPV_eYey7YeaEKPUrks5u2rhXgaJpZM4ZBCJ2> .

[yizhang-yiz]

That's my suspicion, and it won't fail if it's

    Eigen::Matrix<stan::math::var, -1, -1> A_temp(1, 1);
    A_temp << 1.0;

so it's might be related to the assignment.

[bob-carpenter]

On Dec 7, 2018, at 2:11 PM, Daniel Lee ***@***.***> wrote: Cool. Thanks for creating that. Maybe it's the creation of an Eigen::Matrix<var, -1, -1> inside the vari implementation that does it? Instead of using random inside, does it also happen if you just set the first value to 0.0?

There shouldn't be any instances of var being created within a vari class. The var are just pimpl wrappers of vari. Nor should there be any Matrix member variables because their destructors won't get called. Is there a line of code you are looking at?itHub, or mute the thread.

…

[syclik]

It took me a little while to understand Bob's message, but that's absolutely correct. There shouldn't be var instances created within a vari class. I don't understand the part about the Matrix member variables. The Matrix's destructor should get called, right? But not the vars within the Matrix? @bob_carpenter: this example is not realistic. Yi added some unsafe code to a safe implementation of matrix_multiply(). But if you want to see the original thing that crashed, it was here: https://github.com/stan-dev/math/blob/1784c1ce87e3d7982600885932d2cc04e283971c/stan/math/rev/mat/fun/matrix_exp_multiply.hpp On Fri, Dec 7, 2018 at 5:17 PM Bob Carpenter <notifications@github.com> wrote:

…

> On Dec 7, 2018, at 2:11 PM, Daniel Lee ***@***.***> wrote: > > Cool. Thanks for creating that. Maybe it's the creation of an > Eigen::Matrix<var, -1, -1> inside the vari implementation that does it? > Instead of using random inside, does it also happen if you just set the > first value to 0.0? There shouldn't be any instances of var being created within a vari class. The var are just pimpl wrappers of vari. Nor should there be any Matrix member variables because their destructors won't get called. Is there a line of code you are looking at?itHub, or mute the thread. > — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1073 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZ_F3us9uIEunD7vMLi6Lm0zVPk_RNHks5u2uj2gaJpZM4ZBCJ2> .

[bob-carpenter]

That int& should just be an int and you don't need const on primitive args (doesn't change the external api). Eigen's Matrix is an RAII type, so the destructor gets called when it goes out of scope. But that won't happen if the Matrix is a member variable of a vari. Then you have to put it on a special stack to call its destructor explicitly. That is, vari is designed to work without its destructor being called by using the arena memory allocation. If you need destructors called, you have to add it to the special stack.

…

On Dec 10, 2018, at 10:06 AM, Daniel Lee ***@***.***> wrote: It took me a little while to understand Bob's message, but that's absolutely correct. There shouldn't be var instances created within a vari class. I don't understand the part about the Matrix member variables. The Matrix's destructor should get called, right? But not the vars within the Matrix? @bob_carpenter: this example is not realistic. Yi added some unsafe code to a safe implementation of matrix_multiply(). But if you want to see the original thing that crashed, it was here: https://github.com/stan-dev/math/blob/1784c1ce87e3d7982600885932d2cc04e283971c/stan/math/rev/mat/fun/matrix_exp_multiply.hpp On Fri, Dec 7, 2018 at 5:17 PM Bob Carpenter ***@***.***> wrote: > > > > On Dec 7, 2018, at 2:11 PM, Daniel Lee ***@***.***> wrote: > > > > Cool. Thanks for creating that. Maybe it's the creation of an > > Eigen::Matrix<var, -1, -1> inside the vari implementation that does it? > > Instead of using random inside, does it also happen if you just set the > > first value to 0.0? > > There shouldn't be any instances of var being created within a vari class. > The var are just pimpl wrappers of vari. > > Nor should there be any Matrix member variables because their destructors > won't get called. > > Is there a line of code you are looking at?itHub, or mute the thread. > > > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#1073 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AAZ_F3us9uIEunD7vMLi6Lm0zVPk_RNHks5u2uj2gaJpZM4ZBCJ2> > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[rok-cesnovar]

@yizhang-yiz example no longer segfaults (not sure what fixed it but Math lib has been refactored a few times in the last 2 years):

TEST(MathMatrix, multiply_seg_fault_vd) {
  Eigen::Matrix<stan::math::var, -1, -1> A=Eigen::Matrix<stan::math::var, -1, -1>::Random(1, 1);
  Eigen::Matrix<double, -1, -1> B=Eigen::Matrix<double, -1, -1>::Random(1, 1);

  Eigen::Matrix<stan::math::var, -1, -1> result = stan::math::multiply(A, B);

  std::vector<double> gradients;

  std::vector<stan::math::var> vars = stan::math::to_array_1d(A);
  result(0, 0).grad(vars, gradients);
}

Closing.