search

stan-dev / math

The Stan Math Library is a C++ template library for automatic differentiation of any order using forward, reverse, and mixed modes. It includes a range of built-in functions for probabilistic modeling, linear algebra, and equation solving.
https://mc-stan.org
BSD 3-Clause "New" or "Revised" License
750 stars 189 forks source link

Backporting Eigen code changes #2653

Open andrjohns opened 2 years ago

andrjohns commented 2 years ago

Description

In order to fix the compile errors with multiplying/solving CWiseUnaryViews (which originally necessitated the val_op and adj_op member introductions), I've submitted patches to Eigen's code which are needed for fixing our implementations.

For reference, the Eigen PR is here: https://gitlab.com/libeigen/eigen/-/merge_requests/792 And a Math PR that shows tests passing with the code changes is here: https://github.com/stan-dev/math/pull/2604

However, I think we'll need to agree on a method on backporting these changes until they're in an Eigen release. While the simplest method would be to modify the Eigen headers included in the Math library directly, this would break compatibility with rstan, which links against Eigen headers in an external package.

To work around this, I'm proposing that we include the Eigen headers with necessary modifications in the Math library itself (I'm proposing creating a /prim/plugins folder). The existing header guards for these would prevent the source (unmodified) versions from being included when run via rstan.

I have a branch with this proposed structure and backported Eigen changes here: https://github.com/stan-dev/math/compare/develop...andrjohns:feature/backport-view-stride?expand=1

Thoughts?

Tagging @hsbadr, @bgoodri, and @SteveBronder given their involvement in the rstan and Eigen future states

Current Version:

v4.2.1

bgoodri commented 2 years ago

That is more or less what rstan has done with BH when stuff like this happens.

On Wed, Jan 12, 2022 at 11:34 AM Andrew Johnson @.***> wrote:

Description

In order to fix the compile errors with multiplying/solving CWiseUnaryViews (which originally necessitated the val_op and adj_op member introductions), I've submitted patches to Eigen's code which are needed for fixing our implementations.

For reference, the Eigen PR is here: https://gitlab.com/libeigen/eigen/-/merge_requests/792 And a Math PR that shows tests passing with the code changes is here:

2604 https://github.com/stan-dev/math/pull/2604

However, I think we'll need to agree on a method on backporting these changes until they're in an Eigen release. While the simplest method would be to modify the Eigen headers included in the Math library directly, this would break compatibility with rstan, which links against Eigen headers in an external package.

To work around this, I'm proposing that we include the Eigen headers with necessary modifications in the Math library itself (I'm proposing creating a /prim/plugins folder). The existing header guards for these would prevent the source (unmodified) versions from being included when run via rstan.

I have a branch with this proposed structure and backported Eigen changes here: https://github.com/stan-dev/math/compare/develop...andrjohns:feature/backport-view-stride?expand=1

Thoughts?

Tagging @hsbadr https://github.com/hsbadr, @bgoodri https://github.com/bgoodri, and @SteveBronder https://github.com/SteveBronder given their involvement in the rstan and Eigen future states Current Version:

v4.2.1

— Reply to this email directly, view it on GitHub https://github.com/stan-dev/math/issues/2653, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZ2XKSFGLWIGG4DWJG6OXDUVWUQLANCNFSM5LZNZOAA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

andrjohns commented 2 years ago

Nice to know I'm on the right track! Any chance you could point me towards some of the rstan source where this has been implemented? That way I can make things as compatible as possible

bgoodri commented 2 years ago

We don't have any of that stuff at the moment because everything in the latest BH works as far as we know of. But over the years, we sometimes put modified .hpp files into rstan/rstan/rstan/inst/include/boost_not_in_BH, which you can see at

https://github.com/stan-dev/rstan/search?q=boost_not_in_BH

andrjohns commented 2 years ago

Perfect, thanks