-
-
Notifications
You must be signed in to change notification settings - Fork 195
Use Perfect Forwarding in all functions that use apply
family of functors
#3221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
@SteveBronder anything I can do to help this over the line? |
Right now it is just making sure the tests pass. I think I got it so we should be good! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gonna leave the proper review to @andrjohns, just two things that stood out
stan/math/prim/core/complex_base.hpp
Outdated
@@ -17,6 +17,7 @@ namespace math { | |||
template <typename ValueType> | |||
class complex_base { | |||
public: | |||
auto __rep() const { return *this; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't figure out from searching what this would do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our complex<var>
and complex<fvar<T>>
impls still call std library functions sometimes. In those functions, at least for gcc, sometimes they request this __rep()
function. The __rep()
in their code just returns a copy of this
like the one here.
But let me double check if this is still needed
Math pipeline looks good, upstream has some (I believe legitimate) failures. All seem to blame
|
Yes I missed one |
Yep still all good! @SteveBronder should I do a first pass or do you have more changes? |
I have just a little fix I need to make for the constraints but am afk ATM. Everything else is ready for review! |
inline auto positive_constrain(T&& x, S& lp) { | ||
auto&& x_ref = to_ref(std::forward<T>(x)); | ||
lp += sum(x_ref); | ||
return exp(std::forward<decltype(x_ref)>(x)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return exp(std::forward<decltype(x_ref)>(x)); | |
return exp(std::forward<decltype(x_ref)>(x_ref)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bunch of minor requests/queries, the main thing I'm not too sure about is the new specification for the apply_scalar_unary
functions (with the is_arithmetic_v<>
conditional) - what's necessitating that?
stan/math/prim/fun/abs.hpp
Outdated
if constexpr (std::is_arithmetic_v<std::decay_t<T>>) { | ||
return std::abs(x); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary or just a testing thing? The earlier scalar overload should capture the arithmetic case right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think I can remove these
return apply_vector_unary<Container>::apply( | ||
x, [](const auto& v) { return v.array().abs(); }); | ||
std::forward<Container>(x), [](auto&& v) { return v.array().abs(); }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR is a bit inconsistent on whether the apply_vector_unary
functor should be (auto&& v)
or (const auto& v)
, can you standardise on one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just fixed all these up
stan/math/prim/fun/inc_beta.hpp
Outdated
a, b, c); | ||
inline auto inc_beta(T1&& a, T2&& b, T3&& c) { | ||
return apply_scalar_ternary( | ||
[](auto&& d, auto&& e, auto&& f) { return inc_beta(d, e, f); }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the functor also be forwarding the d, e, f
arguments to inc_beta
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes good catch
stan/math/prim/fun/inv_inc_beta.hpp
Outdated
return inv_inc_beta(d, e, f); | ||
}, | ||
a, b, c); | ||
[](auto&& d, auto&& e, auto&& f) { return inv_inc_beta(d, e, f); }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same query around forwarding the inner arguments here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
stan/math/prim/fun/inv_logit.hpp
Outdated
if (a < LOG_EPSILON) { | ||
return exp_a; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be dropped imo, change is unrelated to the PR (and saves some operations)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -191,7 +205,7 @@ struct apply_vector_unary< | |||
= plain_type_t<decltype(apply_vector_unary<T_vt>::apply(x[0], f))>; | |||
std::vector<T_return> result(x.size()); | |||
std::transform(x.begin(), x.end(), result.begin(), [&f](auto&& xx) { | |||
return apply_vector_unary<T_vt>::apply_no_holder(xx, f); | |||
return apply_vector_unary<T_vt>::apply(xx, f); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return apply_vector_unary<T_vt>::apply(xx, f); | |
return apply_vector_unary<T_vt>::apply(std::forward<decltype(xx)>(xx), f); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std transform does not know whether x
is a temporary so it will never do forwarding here.
static inline auto apply_no_holder(const T& x, const F& f) { | ||
return f(x).matrix().derived(); | ||
static inline auto apply_no_holder(T2& x, F&& f) { | ||
return std::forward<F>(f)(std::forward<T2>(x)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no longer returning the plain type now that the .derived()
call is removed, should this at least be evaluating first since make_holder
isn't used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anywhere these functions are being used now?
typename F, typename... Args, | ||
require_not_plain_type_t<std::invoke_result_t<F, Args&&...>>* = nullptr> | ||
template <typename F, typename... Args, | ||
require_not_plain_type_t<std::invoke_result_t<F, Args&&...>>*> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these still well-formed without the nullptr
default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions are forward declared at the top of the file. There we set the default of nullptr
stan/math/rev/fun/norm1.hpp
Outdated
arena_v.adj().array() += res.adj() * sign(arena_v.val().array()); | ||
template <typename Container, | ||
require_eigen_vector_vt<is_var, Container>* = nullptr> | ||
inline var norm1(const Container& x) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both the norm1
and norm2
here have been updated but are still passing by const ref
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -119,9 +119,9 @@ TEST(mathMixMatFun, ub_stdvec_constrain_inf) { | |||
// matrix[], real | |||
TEST(mathMixMatFun, ub_stdvec_mat_mat_constrain) { | |||
Eigen::MatrixXd A_inner(2, 3); | |||
A_inner << 5.0, 2.0, 4.0, -2.0, 0.0, 0.005; | |||
A_inner << 5.0, 2.0, 4.0, -2.0, 1.0, 2.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these values need to be changed for the test to pass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On my desktop machine I was getting weird floating point errors when the values were small. But on my mac I did not get any fp errors. But these tests were failing on my desktop computer before this 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: |
test/prob/beta_neg_binomial/beta_neg_binomial_cdf_00000_generated_vv_test segfaulted on the latest run it looks like |
…less they are an rvalue
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: |
I'm running the distribution tests on this branch with all the tests enabled + |
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: |
Looks like that full run passed 👍🏻 @andrjohns are you able to review again? Thanks! |
Summary
This fixes #3208 by using perfect forwarding for all functions that use our underlying apply family of functions for calling functions on containers and containers and containers. The issue was that the
Holder
class, when used in the apply functors, did not have enough type information to know which arguments it should take ownership of.Consider the following function, where all types are passed in via constant reference.
Calling this function with an Eigen expression that has a temporary in it would not give
apply_scalar_binary
and theHolder
inside ofapply_scalar_binary
enough information to know that theHolder
class should own any of the input arguments. As an example we can look at a simplified version of the code used inpoisson_lccdf.hpp
.gamma_p
usesapply_scalar_binary
andlog
usesapply_scalar_unary
. We need to make sure the inputs and results of thegamma_p
function do not fall out of scope by the time we go throughlog
and then assign tolog_Pi
. Before this PR it would be possible for the expressionn_val + 1.0
to fall out of scope as well as the result ofgamma_p
to go out of scope fromlog
afterlog_Pi
is assigned.To combat this we now use perfect forwarding for all of the functions that use our internal
apply
family of functors. This should allow theHolder
used internally by the apply functors to know which types need to be owned by it to make sure things do not fall out of scope.Tests
There is no new tests for this. Since it is an isue on gcc I do wonder how we should test this in our CI/CD?
Side Effects
I'd like to think of some test we can write so that, in the future, developers do not accidentally write functions that use the apply family of functors that do not use perfect forwarding.
Release notes
Adds perfect forwarding to all functions that use the apply family of functors.
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