[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

• Math issue Update to Eigen 3.4 #2474

• 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/)

• 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)

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

• 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 a195451 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 <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 <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 <#2583 (comment)>, 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 <#2583 (comment)>, 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 02b8555, this branch now has the two versions of eigen and builds v3.3.9.

math/make/libraries

Line 12 in 24f3f76

EIGEN ?= $(MATH)lib/eigen_3.3.9

[andrjohns]

@SteveBronder I've reverted the patch commit I made, so (assuming that only the irt_2pl model fails again) this should be ready for review & merge

[andrjohns]

Now that the feature freeze is over, should we look at getting this merged? @rok-cesnovar or @SteveBronder , not sure who would be the best choice for reviewer here

[RajalakshmiSR]

Is there any target release version(stan) for this eigen 3.4 update?

[WardBrian]

@andrjohns did you ever change the performance tests model? If so, if you merge in develop and it passes I think we can get this approved and merged

[andrjohns]

@andrjohns did you ever change the performance tests model? If so, if you merge in develop and it passes I think we can get this approved and merged

In this case, changing the performance-tests model is unlikely to make this pass - since it's due to minor numerical differences in the vectorised handling of the log() function. In this instance, the changes will fail the first merge but will pass in the subsequent (since the vectorised operations will be the same).

Alternatively, if we want this to pass before we merge, we can also disable vectorised operations in the performance tests (this line).

Thoughts/preferences?

[WardBrian]

I think we will also want to update the result stored in https://github.com/stan-dev/performance-tests-cmdstan/blob/master/golds/stat_comp_benchmarks_benchmarks_irt_2pl_irt_2pl.gold, no?

[WardBrian]

@andrjohns thoughts on the golds question above?

[WardBrian]

I looked over the few changed files outside of the lib/ folder and they all seem reasonable.

I think we can do whatever is required to make the performance tests update themselves after this

[WardBrian]

@serban-nicusor-toptal After merging in master a test is failing to compile on Windows due to out-of-memory, have we handled things like this in the past?

[rok-cesnovar]

We split the tests into multiple files. Though this is the first time I am seeing this happening since we switched to Rtools4+ on Windows.

[serban-nicusor-toptal]

I'm afraid Rok solution is the right one, else we would need to ask Dylan to make the Windows executors with more RAM.

[WardBrian]

@andrjohns @SteveBronder I believe this should be ready to go. Earlier in this thread you mentioned wanting to rebase all the stan changes into one commit and Eigen changes into another, would you still like to do this?

[WardBrian]

Discussed with @SteveBronder and decided rather than rebasing we can just squash this PR

[RajalakshmiSR]

What is the expected stan-math release for this PR?

[rok-cesnovar]

The next release will include it, so 4.6.0 (part of Stan 2.32 release). The release should be official in the middle of April: https://discourse.mc-stan.org/t/planning-the-2-32-release/30641