Add perfect forwarding and constexpr to reverse mode functions #3092
Conversation
|
Oh clang-format made a wild choice for formatting of long |
|
I'm generally in favour of move-semantics, but are we ready to start enforcing a c++17 minimum with |
|
The message we have says
I think it’s fine? People can always use older versions, and basic C++17 support is very old by now |
|
Ah I didn't pay attention to the actual warning, well that makes sense to me then! I'll give this a proper review tomorrow |
|
@andrjohns I need to figure out the weird clang-format stuff before this is ready for review. It's breaking type traits over to newlines for some reason |
|
No wokkas, feel free to ping me when it's sorted |
…nstexpr' into feature/pf-funcs-constexpr
|
@andrjohns Alrighty I think this is ready for review! |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
There was a problem hiding this comment.
Just a few requests (some of which apply to a bunch of changes though!).
Quick clarification question, you've marked a bunch of functions as inline now - any particular motivation there?
Also, with several changes of return types to arena<T> (from just T), is to avoid a copy on return or something else?
|
|
||
| template <typename... Types> | ||
| inline constexpr bool is_constant_v | ||
| = std::conjunction<is_constant<Types>...>::value; |
There was a problem hiding this comment.
| = std::conjunction<is_constant<Types>...>::value; | |
| = math::conjunction<is_constant<Types>...>::value; |
For consistency
There was a problem hiding this comment.
Also same comment about redundancy - is there a situation where is_constant_all_v would behave differently to is_constant_v here?
There was a problem hiding this comment.
Actually I think we should start using the std version! We just used the stan math version because we didn't have c++17 available previously
Also same comment about redundancy - is there a situation where is_constant_all_v would behave differently to is_constant_v here?
Since I made is_constant_all_v to accept multiple types there should be no difference. I'm going to put up another PR where throughout the math library I change is_constant_all with is_constant_v. Which should clean up a lot.
| @@ -28,6 +28,10 @@ struct is_stan_scalar | |||
| is_fvar<std::decay_t<T>>, std::is_arithmetic<std::decay_t<T>>, | |||
| is_complex<std::decay_t<T>>>::value> {}; | |||
|
|
|||
| template <typename... Types> | |||
| inline constexpr bool is_stan_scalar_v | |||
| = std::conjunction<is_stan_scalar<Types>...>::value; | |||
There was a problem hiding this comment.
| = std::conjunction<is_stan_scalar<Types>...>::value; | |
| = math::conjunction<is_stan_scalar<Types>...>::value; |
| template <typename... Types> | ||
| inline constexpr bool is_autodiff_v | ||
| = math::conjunction<is_autodiff<Types>...>::value; | ||
|
|
||
| template <typename... Types> | ||
| inline constexpr bool is_autodiffable_v | ||
| = math::conjunction<is_autodiff<scalar_type_t<Types>>...>::value; | ||
|
|
There was a problem hiding this comment.
These two typedefs seem to test the same thing. Also, shouldn't this be is_autodiff_all_v?
There was a problem hiding this comment.
These need better names. is_autodiff_v<> looks to see is the type is a fvar or var and fails otherwise. is_autodiffable_v looks into the scalar type of the type to see if it is autodiffable, so things like eigen matrices and vectors of var or fvar types would be true for is_autodiffable_v.
I left is_autodiff alone to not mess with the other functions that use it (mostly functions that use it in a requires). Maybe the current is_autodiff should be named is_autodiff_scalar?
| if (!is_constant<T1>::value && !is_constant<T2>::value) { | ||
| arena_t<promote_scalar_t<var, T1>> arena_A = A; | ||
| arena_t<promote_scalar_t<var, T2>> arena_B = B; | ||
| if constexpr (is_autodiffable_v<T1, T2>) { |
There was a problem hiding this comment.
Should this be !is_constant_all_v<T1, T2> for consistency with the original definition?
There was a problem hiding this comment.
(Applies to the other changes as well)
There was a problem hiding this comment.
Maybe I'm confused on definitions. If something is not constant then shouldn't it be autodiffable (i.e. var and fvar?)
stan/math/rev/fun/atan2.hpp
Outdated
| arena_t<VarMat> arena_b = b; | ||
| if constexpr (is_autodiffable_v<Scalar, | ||
| VarMat> && is_autodiffable_v<VarMat>) { | ||
| auto atan2_val = atan2(a.val(), arena_b.val()); |
There was a problem hiding this comment.
Is this one of those situations where using auto with arena_matrix could be problematic?
There was a problem hiding this comment.
How so? Also I looked through these functions and massively simplified them in the latest commit
| @@ -31,8 +31,7 @@ namespace math { | |||
| template <typename Mat1, typename Mat2, | |||
| require_all_eigen_t<Mat1, Mat2>* = nullptr, | |||
| require_any_eigen_vt<is_var, Mat1, Mat2>* = nullptr> | |||
| inline Eigen::Matrix<return_type_t<Mat1, Mat2>, 1, Mat1::ColsAtCompileTime> | |||
| columns_dot_product(const Mat1& v1, const Mat2& v2) { | |||
| inline auto columns_dot_product(const Mat1& v1, const Mat2& v2) { | |||
There was a problem hiding this comment.
| inline auto columns_dot_product(const Mat1& v1, const Mat2& v2) { | |
| inline auto columns_dot_product(Mat1&& v1, Mat2&& v2) { |
There was a problem hiding this comment.
We don't use perfect forwarding in this function so idt it is necessary
stan/math/rev/fun/cumulative_sum.hpp
Outdated
| @@ -32,7 +32,7 @@ inline auto cumulative_sum(const EigVec& x) { | |||
| using return_t = return_var_matrix_t<EigVec>; | |||
| arena_t<return_t> res = cumulative_sum(x_arena.val()).eval(); | |||
| if (unlikely(x.size() == 0)) { | |||
| return return_t(res); | |||
| return arena_t<return_t>(res); | |||
There was a problem hiding this comment.
| return arena_t<return_t>(res); | |
| return res; |
stan/math/rev/fun/fma.hpp
Outdated
| } | ||
| if (!is_constant<T3>::value) { | ||
| forward_as<T3_var>(arena_z).adj() += ret.adj().sum(); | ||
| if constexpr (is_matrix_v<T1, T2, T3>) { |
There was a problem hiding this comment.
This many if/else if statements (nested as well), feels a bit yucky. Not for this PR, but would be good to simplify in the future using as_array_or_scalar
There was a problem hiding this comment.
Yeah this was gross. I worked on this a bit and simplified it quite a lot!
stan/math/rev/fun/fma.hpp
Outdated
| arena_t<T1> arena_x = std::forward<T1>(x); | ||
| arena_t<T2> arena_y = std::forward<T2>(y); | ||
| arena_t<T3> arena_z = std::forward<T3>(z); | ||
| if constexpr (is_matrix_v<T1> && is_matrix_v<T2>) { |
There was a problem hiding this comment.
Do we have an is_all_matrix_v for this?
There was a problem hiding this comment.
is_matrix_v is overloaded for multiple types so fixed this.
| using T2_t = std::decay_t<T2>; | ||
| arena_t<Eigen::Matrix<double, T2_t::RowsAtCompileTime, | ||
| T2_t::ColsAtCompileTime>> |
There was a problem hiding this comment.
| using T2_t = std::decay_t<T2>; | |
| arena_t<Eigen::Matrix<double, T2_t::RowsAtCompileTime, | |
| T2_t::ColsAtCompileTime>> | |
| arena_t<promote_scalar_t<double, T2>> |
or
| using T2_t = std::decay_t<T2>; | |
| arena_t<Eigen::Matrix<double, T2_t::RowsAtCompileTime, | |
| T2_t::ColsAtCompileTime>> | |
| arena_t<decltype(res.adj())> |
Since arena_t<> uses plain_type_t internally (if I'm understanding that right)
All functions in stan math should be inline so that we are allowed to have multiple definitions across translation units.
This is so we avoid a copy on the return. It's one of the bigger changes we agreed to with the bump to 5.0 |
…nstexpr' into feature/pf-funcs-constexpr
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
|
fyi I'm going to do just the constraints I've done so far. It's a lot of code so I'm going to hold off and fix those in another PR |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
|
@andrjohns this is ready for review! |
|
@SteveBronder are you hoping to get this in before the release? I noticed the makefile changes would also be useful for #3110 |
|
Let me give this one hard look over and then I think it's ready to merge |
Summary
This is a pretty decently sized PR that does a few things
if constexprand remove uses ofpromote_scalar_tandforward_as. This gets rid of a lot of overhead metaprogramming we had to do with c++14. Usingif constexpralso lead to a lot of other little nice cleanups with redundant codeTests
All current tests pass
Side Effects
Release notes
Add perfect forwarding to reverse mode autodiff functions
Checklist
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
./runTests.py test/unit)make test-headers)make test-math-dependencies)make doxygen)make cpplint)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested