[WIP] Update to Eigen 3.4

[SteveBronder]

Summary

Updates Eigen to 3.4-rc1 to test and report any issues downstream.

Tests

No new tests

Side Effects

See the release page for the updates to 3.4

https://eigen.tuxfamily.org/index.php?title=3.4

Release notes

None

Checklist

• [x] Math issue #2474

• [x] Copyright holder: Steve Bronder

  The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses: - Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause) - Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

• [x] the basic tests are passing

  • unit tests pass (to run, use: ./runTests.py test/unit)
  • header checks pass, (make test-headers)
  • dependencies checks pass, (make test-math-dependencies)
  • docs build, (make doxygen)
  • code passes the built in C++ standards checks (make cpplint)

• [x] the code is written in idiomatic C++ and changes are documented in the doxygen

• [x] the new changes are tested

[SteveBronder]

@rok-cesnovar can we bump up to just testing on Rtools 4.0? tmk the old-rel of Cran now uses version 4.0 for checking so that seems fine.

[SteveBronder]

@rok-cesnovar do you have an idea of what the scheme would be to update our testing to Rtools40? I tried changing the download link just to the Rtools 40 download in the PR (here) though that seems to just keep downloading?

[rok-cesnovar]

Oh sorry, I missed the message last time....

It turns out I was complicating things here unnecessarily. Never went back to simplify it though. Just add the setup-r action that will instal the release version of R (which also installs Rtools 4.0).

- uses: r-lib/actions/setup-r@v1
   with:
     r-version: 'release'

and then add the paths with:

- name: Set path for RTools 4.0
  if: runner.os == 'Windows'
  run: echo "C:/rtools40/usr/bin;C:/rtools40/mingw64/bin" | Out-File -Append -FilePath $env:GITHUB_PATH -Encoding utf8

[SteveBronder]

Awesome thank you!

[SteveBronder]

@bgoodri these tests should hopefully pass and it should be ready to look over. The stan code related changes are in https://github.com/stan-dev/math/pull/2583/commits/a195451ed03757168164eb666c10f70954d0eb5b though reading it over I think everything should be pretty compatable for 2.26?

I couldn't figure it out but the specialization for fwd mode's mdivide_right() is failing so I just commented out the code for now (though we should just delete it before we merge if we are going to delete it). If you want to take a look at those you can uncomment the code in stan/math/fwd/fun/mdivide_right.hpp and run the following test with CXXFLAGS+=-DSTAN_TEST_PRINT_MATRIX_FAILURE in your make/local to see the failing hessian and hessian gradient.

./runTests.py -j24 test/unit/math/ -f mix/fun/mdivide_right_test

[SteveBronder]

@bgoodri we may hav to hold off on this for a lil bit till the Eigen bug below is sorted out since it effects a lot of our stuff (and is the error we are seeing in the tests)

https://gitlab.com/libeigen/eigen/-/issues/2391

[serban-nicusor-toptal]

This Jenkins error is on me @SteveBronder looks like open mpi updated not properly last night when I've added clang-format. Will rebuild the image and restart the job sorry for the inconvenience!

[SteveBronder]

Np and ty!

[andrjohns]

@serban-nicusor-toptal would you be able to have a look at the CI for this PR when you have a sec? It looks like the gelman-group-linux machine is having issues

[serban-nicusor-toptal]

Checking.

Edit: Moved that stage to Docker, I think some tests were failing that's why we let it on the gelman machine but now I've tested it and it works fine. I will soon create a PR so we can propagate this change on develop too.

[andrjohns]

It looks like the current 3.4 release has a bug with handling -inf arguments to exp, which has since been resolved in the development branch.

Using the testing code:

#include <Eigen/Core>
#include <limits>
#include <iostream>

int main() {
    double y = -std::numeric_limits<double>::infinity();
    Eigen::VectorXd y_vec(2);
    y_vec << y, y;

    std::cout << y_vec.array().exp() << std::endl;
}

The results under 3.4 (godbolt):

Program returned: 0
5.55553e-309
5.55553e-309

And under the current trunk (godbolt):

Program returned: 0
0
0

It looks to upgrade to 3.4 we'll have to disable the use of Eigen's SIMD-vectorised exp until the following release of Eigen

[bgoodri]

Is 5e-309 really a problem?

On Fri, Mar 25, 2022 at 12:44 PM Andrew Johnson @.***> wrote:

It looks like the current 3.4 release has a bug with handling -inf arguments to exp, which has since been resolved in the development branch.

Using the testing code:

#include <Eigen/Core> #include #include int main() { double y = -std::numeric_limits::infinity(); Eigen::VectorXd y_vec(2); y_vec << y, y;

std::cout << y_vec.array().exp() << std::endl;

}

The results under 3.4 (godbolt https://godbolt.org/z/3xqz84f43):

Program returned: 0 5.55553e-309 5.55553e-309

And under the current trunk (godbolt https://godbolt.org/z/M5zWTW6z9):

Program returned: 0 0 0

It looks to upgrade to 3.4 we'll have to disable the use of Eigen's SIMD-vectorised exp until the following release of Eigen

— Reply to this email directly, view it on GitHub https://github.com/stan-dev/math/pull/2583#issuecomment-1079207984, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZ2XKSWHW7TRWOAZYPX7R3VBXUN7ANCNFSM5EIOCC7A . You are receiving this because you were mentioned.Message ID: @.***>

[andrjohns]

It's what's causing the current distribution test failures, since the log cdf of the gumbel with Inf inputs is no longer zero.

So we could add a tolerance to the test if its not much of an issue

[bgoodri]

I think temporarily changing the unit test would be preferable to disabling SIMD for something that is correct for all numerical intents and purposes.

On Fri, Mar 25, 2022 at 12:50 PM Andrew Johnson @.***> wrote:

It's what's causing the current distribution test failures, since the log cdf of the gumbel with Inf inputs is no longer zero.

So we could add a tolerance to the test if its not much of an issue

— Reply to this email directly, view it on GitHub https://github.com/stan-dev/math/pull/2583#issuecomment-1079212840, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZ2XKSYS7R3L7DHXQKKEC3VBXVEXANCNFSM5EIOCC7A . You are receiving this because you were mentioned.Message ID: @.***>

[andrjohns]

Sounds good to me

[SteveBronder]

Just an fyi when this passes I think what we need to do is rebase this to have the eigen 3.4 include in one commit and the actual stan changes in another commit so it's a bit easier to look over. Or alt we could have one PR just adding Eigen 3.4, one PR to switch to 3.4, and another to delete 3.39. Though I think the rebasing thing will not be too bad

[andrjohns]

Yeah agreed. Probably easiest to do the Stan Math changes first, so we can make sure they're backwards compatible-ish before the Eigen changes go in

[hsbadr]

FYI. I've tested this successfully with the experimental version RStan.

PS: It conflicts with #2641.

[andrjohns]

FYI. I've tested this successfully with the experimental version RStan.

PS: It conflicts with #2641.

Great news! Yeah both PRs partially overlap in the same changes, so I'll just resolve the conflicts with whichever is merged first

[SteveBronder]

Excellent this is working! @bgoodri actually instead of breaking up the commits how do you feel about using the view github gives us below? Along with the lhs tab dropdown I was able to look over the changes pretty easily. The big thing imo is making sure mdivide_left and right are good

https://github.com/stan-dev/math/pull/2583/files?file-filters%5B%5D=.hpp&show-viewed-files=true

[andrjohns]

I've narrowed down the mdivide failures to something related to vector vs row_vector differences, since the tests all pass if vectors are used instead

[SteveBronder]

I can take a look at this next week again to try to sort that out. Yes I noticed something about the vector thing being weird as well. I can take a crack again at this next week

[roualdes]

Thanks, y'all, for getting this going. In the last week, I've missed out on two new features that Eigen 3.4 brings.

[andrjohns]

I've traced the mdivide failures to the finite_diff_grad_hessian_auto function in the autodiff testing framework. The gradients calculated by the autodiff framework are consistent between a row_vector(2) and a matrix (1, 2) inputs, but they differ when calculated by finite-differencing.

I'll keep chasing this down and update with any findings. Almost there!

[andrjohns]

@SteveBronder I've narrowed things down to the hessian function, but I can't figure out why the behaviour is different.

I've put together a minimal reproduction of the part of the testing framework where the discrepancy occurs, would you be able to have a look when you get a minute?

#include <test/unit/math/test_ad.hpp>
#include <vector>
#include <iostream>

template <typename T>
auto calc_hessian(const T& x1) {
   auto f = [](const auto& x, const auto& y) {
    return stan::math::mdivide_right(x, y);
  };

  Eigen::MatrixXd x2(2, 2);
  x2 << 1, 0, 0, 1;

  auto g = [&](const auto& v) {
    auto ds = stan::test::to_deserializer(v);
    auto x1ds = ds.read(x1);
    auto x2ds = ds.read(x2);
    return stan::test::serialize_return(stan::test::internal::eval(f(x1ds, x2ds)))[1];
  };

  // internal::expect_ad_helper(tols, f, g12, serialize_args(x1, x2), x1, x2);
  auto x = stan::test::serialize_args(x1, x2);

  // test_grad_hessian(tols, g, x, gx);

  double fx = 0;
  int d = x.size();
  std::vector<Eigen::MatrixXd> grad_hess_fx(d);
  Eigen::VectorXd x_temp(x);
  Eigen::VectorXd grad_auto(d);
  Eigen::MatrixXd hess_auto(d, d);

  x_temp(5) = x(5) + 2 * stan::math::finite_diff_stepsize(x(5));
  stan::math::hessian(g, x_temp, fx, grad_auto, hess_auto);

  return hess_auto;
};

TEST(MathFunctions, abs) {
  Eigen::MatrixXd mat_x1(1, 2);
  mat_x1 << 1, 1;

  Eigen::RowVectorXd rv_x1(2);
  rv_x1 << 1, 1;

  Eigen::MatrixXd mat_hessian = calc_hessian(mat_x1);
  Eigen::MatrixXd rv_hessian = calc_hessian(rv_x1);

  EXPECT_MATRIX_EQ(mat_hessian, rv_hessian);
}

[hsbadr]

@andrjohns After you've merged develop in 02b85557330c7d20382038e43ccc0f9d3061c5db, this branch now has the two versions of eigen and builds v3.3.9. https://github.com/stan-dev/math/blob/24f3f763b4f05adcaf8cca65b24de6c6bb5da2fa/make/libraries#L12

[andrjohns]

Ah good catch, thanks. I knew that merge was too painless to be true

[SteveBronder]

@andrjohns ty for the minimal example! Yes I will try to look at this on Monday

[andrjohns]

@SteveBronder this is now passing all tests (including mdivide_), but the 2pl_irt model from the performance tests is failing. When I run locally it looks like the values are consistent between develop and this PR, but I'm not sure if I'm doing something wrong. Would you be able to run the test on your machine when you get a minute? Thanks!

[SteveBronder]

Tysm for sorting out the above error, very exciting!!!

I'm looking at irt_2pl and it doesn't really seem to do much related to the changes here. I'll run it locally to see whatsup rn