Use perfect forwarding for functions that use `apply__` functions by SteveBronder · Pull Request #3215 · stan-dev/math · GitHub

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.

template <typename T1, typename T2, require_any_container_t<T1, T2>* = nullptr>
inline auto gamma_p(const T1& a, const T2& b) {
  return apply_scalar_binary(
      [](const auto& c, const auto& d) { return gamma_p(c, d); }, a, b);
}

Calling this function with an Eigen expression that has a temporary in it would not give apply_scalar_binary and the Holder inside of apply_scalar_binary enough information to know that the Holder class should own any of the input arguments. As an example we can look at a simplified version of the code used in poisson_lccdf.hpp.

auto log_Pi = log(gamma_p(n_val + 1.0, lambda_val)));
double log_sum = sum(log_Pi);

gamma_p uses apply_scalar_binary and log uses apply_scalar_unary. We need to make sure the inputs and results of the gamma_p function do not fall out of scope by the time we go through log and then assign to log_Pi. Before this PR it would be possible for the expression n_val + 1.0 to fall out of scope as well as the result of gamma_p to go out of scope from log after log_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 the Holder 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
- 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

Overall big fan of the changes! Just a couple of general queries before a full review

Suggested change

      
                  std::forward<T>(x), [&](const auto& alpha) {
          
                  std::forward<T>(x), [](auto&& alpha) {

The apply_vector_unary functors themselves should probably also perfect-forwarding, since they'll be passing their inputs to apply_* functions as well.

Also probably best to remove the reference-capture default while we're here

The apply_vector_unary functors themselves should probably also perfect-forwarding, since they'll be passing their inputs to apply_* functions as well.

Can you clarify? I'm not seeing in this function how much forwarding can be done. I do the perfect forwarding in the actual code for apply_vector_unary etc if that is was you mean.

Also probably best to remove the reference-capture default while we're here

Agree

Suggested change

      
              static inline auto apply(const T2& x) {
          
                return F::fun(x);
          
              }
          
              static inline auto apply(T2&& x) {
          
                return F::fun(std::forward<T2>(x));
          
              }

Should this also be forwarding? As the downstream calls are forwarding their arguments to apply_scalar_unary<*>::apply()

Not something you necessarily need to change in this PR, but it might be a bit cleaner to move the std::decay_t<T> handling into apply_vector_unary itself

Suggested change

      
              plain_type_t<T_x> tanh_x = tanh(x);
          
              plain_type_t<T_x> tanh_x = tanh(std::forward<T_x>(x));

Looks like the the std::forward<T>(x) is missing from this function, and there are other container overloads in the file which probably need forwarding added as well

So for functions that just operate on scalars I don't think we need to worry about forwarding as much since those functions will immediately evaluate

I think mu also needs to be forwarded here since it's passed to to_ref below

Suggested change

      
                  divide(subtract(std::forward<T>(y), std::forward<M>(mu_ref)),
          
                         std::forward<S>(sigma_ref)));
          
                  divide(subtract(std::forward<T>(y), std::forward<decltype(mu_ref)>(mu_ref)),
          
                         std::forward<decltype(sigma_ref)>(sigma_ref)));

Should these also be forwarded in the offset_multiplier_free call below?

Suggested change

      
              plain_type_t<T> log_inv_logit_x = log_inv_logit(x);
          
              plain_type_t<T> log_inv_logit_x = log_inv_logit(std::forward<T>(x));

Re #3208, this branch fixes the issue in test/prob/poisson/poisson_ccdf_log_00000_generated_v_test, but I'm still getting ASAN failures in test/prob/loglogistic/loglogistic_cdf_00001_generated_ffv_test on line 116 of loglogistic_cdf.hpp

math/stan/math/prim/prob/loglogistic_cdf.hpp

Lines 115 to 116 in f7ccc01

    
               = -multiply_log(alpha_div_y_pow_beta, alpha_div_y) * prod_all_sq; 
        
           partials<2>(ops_partials) = beta_deriv * cdf_div_elt;

Could be related to #3147?

…arried through them

…d-apply

I do think we should weigh the odds that merging something of this scope during a release window would introduce more issues than it resolves (especially because the issues seem to have been present in the code for a couple years)

I agree this is a large PR. Once I found the bug in one place I realized it was kind of systematic to all of our functions that can take in an expression and then output an expression. Let's talk irl. I agree this is risky before a release, but also think the bug is kind of large and can also happen in user space

Not denying it can happen to users, but as far as we know it hasn’t in the couple years it has been present. So, would it be that bad to hold the fix a couple months for the next release

Anyway, especially if @andrjohns thinks he can review it, I’m not opposed to merging, just wanted to ask in the name of due diligence

…::vector

Yes I agree that it feels risky. But this is a very odd bug and imo I don't feel great about having this in a release when we know it is there.

Not denying it can happen to users, but as far as we know it hasn’t in the couple years it has been present. So, would it be that bad to hold the fix a couple months for the next release

Anyway, especially if @andrjohns thinks he can review it, I’m not opposed to merging, just wanted to ask in the name of due diligence

Definitely happy to review! But given that the first 2.37 RC was already a month ago and we'd want another RC with pretty decent testing after this many files were touched, I'd just worry that it would push out the broader release schedule. Although I don't have a good sense of how severe/urgent the issue is, so I'm happy to go either way

But given that the first 2.37 RC was already a month ago and we'd want another RC with pretty decent testing after this many files were touched

The changes to laplace are going to require (at least) one more release candidate anyway. We got a bit caught out with the level of feedback and some travel by various developers which has stretched out the release

…ize https://godbolt.org/z/cn7dKsMWr

Definitely happy to review! But given that the first 2.37 RC was already a month ago and we'd want another RC with pretty decent testing after this many files were touched, I'd just worry that it would push out the broader release schedule. Although I don't have a good sense of how severe/urgent the issue is, so I'm happy to go either way

I'd list the issue as severe. I don't think it is so sever we need a 1800 line PR to fix it though. This is mostly to figure out which pieces need changed. My goal here is to pinpoint the underlying issue and check and check which things need changed. Right now I think there are two main issues

Anything that uses the apply family of functions and can work over eigen matrices needs to use perfect forwarding
Anything that returns an expression needs to use perfect forwarding and return a Holder class.

This will touch a lot, but not as much as in this PR hopefully. To be able to replicate this issue I think we need to change the expression tests so that they generate use temp matrices. This is actually not that bad of a change. Right now it just generates them as a separate line like the following.

auto matrix4 = stan::test::make_arg<Eigen::Matrix<double, Eigen::Dynamic, Eigen::Dynamic>>(0.4, 1);
auto matrix5 = stan::test::make_arg<Eigen::Matrix<double, Eigen::Dynamic, Eigen::Dynamic>>(0.4, 1);
auto result6 = stan::math::eval(stan::math::mdivide_right_spd(matrix0_expr2,matrix1_expr3));
auto result7 = stan::math::eval(stan::math::mdivide_right_spd(matrix4,matrix5));

But we just need to inline matrix4/5 so that they are temporaries

auto result6 = stan::math::eval(stan::math::mdivide_right_spd(matrix0_expr2,matrix1_expr3));
auto result7 = stan::math::eval(stan::math::mdivide_right_spd(
stan::test::make_arg<Eigen::Matrix<double, Eigen::Dynamic, Eigen::Dynamic>>(0.4, 1),
  stan::test::make_arg<Eigen::Matrix<double, Eigen::Dynamic, Eigen::Dynamic>>(0.4, 1)));

Sounds good to me! I'll hold off on any reviews then until you've got 'final' PR

I think I have something smaller that fixes this. Not tiny but not as big as this

SteveBronder and others added 2 commits June 13, 2025 16:33

perfect forward container math overloads

4eb5681

use perfect forwarding for functions that use apply

81bd2d0

SteveBronder marked this pull request as ready for review June 30, 2025 21:11

yashikno and others added 7 commits June 30, 2025 17:12

Merge commit 'bdd5b3ed4e54666590f5eb6fb6e822f1099da01b' into HEAD

561f772

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

e40bd1e

cleanup apply_scalar_unary

3c9462e

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

5dd0f62

update sum_to_zero

589895d

Merge commit '09542d0beef76139c8d3df580535cae80cfb0191' into HEAD

6301f3e

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

deec65a

andrjohns requested changes Jul 1, 2025

View reviewed changes

SteveBronder added 2 commits July 1, 2025 00:17

fix mu and sigma ref

8a824c9

update offset_multiplier_free requires

5fffe24

WardBrian mentioned this pull request Jul 1, 2025

Release CmdStan 2.37 stan-dev/cmdstan#1324

Closed

26 tasks

SteveBronder and others added 7 commits July 1, 2025 15:36

update functions with holder

80dbe6c

make all recursive vector functions pf so that value information is c…

50052d9

…arried through them

Merge remote-tracking branch 'origin/develop' into fix/perfect-forwar…

939ed72

…d-apply

update

e63c866

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

67a15d5

fix functions that return expressions

ebb2dbd

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

92e4076

fix symmetrize

3e921d4

SteveBronder and others added 4 commits July 2, 2025 14:48

update logic for offset_multiply to direct to correct version for std…

43a2e0b

…::vector

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

2b9d261

fix categorical_logit test to use plain type for rep_matrix

1144241

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

3c42f90

SteveBronder and others added 2 commits July 2, 2025 17:09

add back tests for rep_matrix

0f1658d

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

7958b81

SteveBronder and others added 4 commits July 2, 2025 17:25

fix reverse

77646e6

Update reverse.hpp

8dc16ca

update block like functions for expressions

2b52498

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

b033319

SteveBronder and others added 2 commits July 7, 2025 13:37

remove if in mdivide_left_spd as the expressions return the correct s…

1ebbb3e

…ize https://godbolt.org/z/cn7dKsMWr

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

7862a22

update

ea2f224

SteveBronder added 3 commits July 8, 2025 10:37

use to_ref in quad_diag_form

49a5273

use to_ref in quad_diag_form

60065f1

use to_ref in quad_diag_form

54b12e1

SteveBronder closed this Jul 10, 2025

-                        log_softmax_alpha(k).d_ += negative_alpha_m_d_times_softmax_alpha_t_m;
+              inline auto log_softmax(T&& x) {
+                return apply_vector_unary<T>::apply(
+                    std::forward<T>(x), [&](const auto& alpha) {

-                return apply_vector_unary<T>::apply(
-                    y, [K](auto&& v) { return cholesky_corr_constrain(v, K); });
+              inline auto cholesky_corr_constrain(T&& y, int K) {
+                return apply_vector_unary<std::decay_t<T>>::apply(

               template <typename T_x, typename T_lp>
-              inline auto corr_constrain(const T_x& x, T_lp& lp) {
+              inline auto corr_constrain(T_x&& x, T_lp& lp) {
                 plain_type_t<T_x> tanh_x = tanh(x);

               template <typename T, typename L, require_all_stan_scalar_t<T, L>* = nullptr,
                         require_all_not_st_var<T, L>* = nullptr>
-              inline auto lb_constrain(const T& x, const L& lb) {
+              inline auto lb_constrain(T&& x, const L& lb) {

                         require_not_std_vector_t<M>* = nullptr>
-              inline auto offset_multiplier_free(const std::vector<T>& x, const M& mu,
-                                                 const std::vector<S>& sigma) {
+              inline auto offset_multiplier_free(T&& x, const M& mu, S&& sigma) {

	std::forward<T>(x), [&](const auto& alpha) {
	std::forward<T>(x), [](auto&& alpha) {

	plain_type_t<T_x> tanh_x = tanh(x);
	plain_type_t<T_x> tanh_x = tanh(std::forward<T_x>(x));

		divide(subtract(std::forward<T>(y), std::forward<M>(mu_ref)),
		std::forward<S>(sigma_ref)));

		auto&& mu_ref = to_ref(std::forward<M>(mu));
		auto&& sigma_ref = to_ref(std::forward<S>(sigma));

-                lp += log_inv_logit_x + log1m_inv_logit(x);
-                return exp(log_inv_logit_x);
+              inline auto prob_constrain(T&& x, return_type_t<T>& lp) {
+                plain_type_t<T> log_inv_logit_x = log_inv_logit(x);

	plain_type_t<T> log_inv_logit_x = log_inv_logit(x);
	plain_type_t<T> log_inv_logit_x = log_inv_logit(std::forward<T>(x));

Use perfect forwarding for functions that use `apply_*_*` functions by SteveBronder · Pull Request #3215 · stan-dev/math · GitHub

Uh oh!

Uh oh!

Use perfect forwarding for functions that use apply_*_* functions - #3215

Use perfect forwarding for functions that use apply_*_* functions#3215SteveBronder wants to merge 35 commits intodevelopstan-dev/math:developfrom fix/perfect-forward-applystan-dev/math:fix/perfect-forward-applyCopy head branch name to clipboard

Conversation

SteveBronder commented Jun 30, 2025

Summary

Tests

Side Effects

Release notes

Checklist

Uh oh!

andrjohns left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WardBrian commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WardBrian commented Jul 2, 2025

Uh oh!

SteveBronder commented Jul 2, 2025

Uh oh!

WardBrian commented Jul 2, 2025

Uh oh!

SteveBronder commented Jul 2, 2025

Uh oh!

andrjohns commented Jul 7, 2025

Uh oh!

WardBrian commented Jul 7, 2025

Uh oh!

SteveBronder commented Jul 7, 2025

Uh oh!

andrjohns commented Jul 8, 2025

Uh oh!

SteveBronder commented Jul 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Use perfect forwarding for functions that use `apply__` functions by SteveBronder · Pull Request #3215 · stan-dev/math · GitHub

Use perfect forwarding for functions that use apply__ functions - #3215

Use perfect forwarding for functions that use apply__ functions#3215
SteveBronder wants to merge 35 commits into
developfrom
fix/perfect-forward-apply

WardBrian commented Jul 1, 2025 •
edited
Loading