Skip to content

Let prim functions return expressions #2190

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

Merged
merged 37 commits into from
Dec 8, 2020

Conversation

t4c1
Copy link
Contributor

@t4c1 t4c1 commented Nov 12, 2020

Summary

I went over all prim functions and modified them to return Eigen expressions, where it makes sense to do so.

I also added a new helper function to_forwarding_ref, which is used, where we need to store equivalent of to_ref in a holder. It works the same as to_ref, except it does perfect forwarding when output does not need to be converted to plain type. In other words it can return rvalue reference to input if the input does not need to be converted.

Improved make_holder, so that holder is not used if the given functor returns plain type or if all arguments are of types, which Eigen passes around by value in expressions.

Tests

All changes should be covered by expression testing framework. I am hoping nothing goes up in flame.

Side Effects

Like ... this whole PR? We never had exposed functions returning expressions.

Release notes

Prim functions now return expressions where that makes sense.

Checklist

  • Math issue Generalize matrix function signatures #1470

  • Copyright holder: Tadej Ciglarič

    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
Copy link
Collaborator

Weird include error on poisson posted below

In file included from test/prob/poisson/poisson_00000_generated_ffd_test.cpp:3:
In file included from ./test/prob/test_fixture_distr.hpp:4:
In file included from ./stan/math/mix.hpp:6:
In file included from ./stan/math/prim.hpp:12:
In file included from ./stan/math/prim/prob.hpp:276:
In file included from ./stan/math/prim/prob/poisson_log.hpp:5:
./stan/math/prim/prob/poisson_lpmf.hpp:68:28: error: call to function 'sum' that is neither visible in the template definition nor found by argument-dependent lookup
  T_partials_return logp = sum(multiply_log(n_val, lambda_val));
                           ^
./stan/math/prim/prob/poisson_lpmf.hpp:85:10: note: in instantiation of function template specialization 'stan::math::poisson_lpmf<false, std::vector<int, std::allocator<int> >, stan::math::fvar<stan::math::fvar<double> > >' requested here
  return poisson_lpmf<false>(n, lambda);
         ^
./stan/math/prim/prob/poisson_log.hpp:23:10: note: in instantiation of function template specialization 'stan::math::poisson_lpmf<std::vector<int, std::allocator<int> >, stan::math::fvar<stan::math::fvar<double> > >' requested here
  return poisson_lpmf<T_n, T_rate>(n, lambda);
         ^
./test/prob/poisson/poisson_test.hpp:49:24: note: in instantiation of function template specialization 'stan::math::poisson_log<std::vector<int, std::allocator<int> >, stan::math::fvar<stan::math::fvar<double> > >' requested here
    return stan::math::poisson_log(n, lambda);
                       ^
./test/prob/test_fixture_distr.hpp:77:26: note: in instantiation of function template specialization 'AgradDistributionsPoisson::log_prob<std::vector<int, std::allocator<int> >, stan::math::fvar<stan::math::fvar<double> >, empty, empty, empty, empty>' requested here
      TestClass.template log_prob<T0, T1, T2, T3, T4, T5>(p0, p1, p2, p3, p4,
                         ^
./test/prob/test_fixture_distr.hpp:822:9: note: in instantiation of member function 'AgradDistributionTestFixture<boost::mpl::vector<AgradDistributionsPoisson, boost::mpl::vector<std::vector<int, std::allocator<int> >, stan::math::fvar<stan::math::fvar<double> >, empty, empty, empty, empty, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na> >::call_all_versions' requested here
  this->call_all_versions();
        ^
lib/benchmark_1.5.1/googletest/googletest/include/gtest/internal/gtest-internal.h:472:44: note: in instantiation of member function 'gtest_suite_AgradDistributionTestFixture_::CallAllVersions<boost::mpl::vector<AgradDistributionsPoisson, boost::mpl::vector<std::vector<int, std::allocator<int> >, stan::math::fvar<stan::math::fvar<double> >, empty, empty, empty, empty, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na> >::TestBody' requested here
  Test* CreateTest() override { return new TestClass; }
                                           ^
lib/benchmark_1.5.1/googletest/googletest/include/gtest/internal/gtest-internal.h:730:13: note: in instantiation of member function 'testing::internal::TestFactoryImpl<gtest_suite_AgradDistributionTestFixture_::CallAllVersions<boost::mpl::vector<AgradDistributionsPoisson, boost::mpl::vector<std::vector<int, std::allocator<int> >, stan::math::fvar<stan::math::fvar<double> >, empty, empty, empty, empty, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na> > >::CreateTest' requested here
        new TestFactoryImpl<TestClass>);
            ^
lib/benchmark_1.5.1/googletest/googletest/include/gtest/internal/gtest-internal.h:789:50: note: in instantiation of member function 'testing::internal::TypeParameterizedTest<AgradDistributionTestFixture, testing::internal::TemplateSel<CallAllVersions>, testing::internal::Types<boost::mpl::vector<AgradDistributionsPoisson, boost::mpl::vector<std::vector<int, std::allocator<int> >, stan::math::fvar<stan::math::fvar<double> >, empty, empty, empty, empty, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>> >::Register' requested here
    TypeParameterizedTest<Fixture, Head, Types>::Register(
                                                 ^
test/prob/poisson/poisson_00000_generated_ffd_test.cpp:32:1: note: in instantiation of member function 'testing::internal::TypeParameterizedTestSuite<AgradDistributionTestFixture, testing::internal::Templates<CallAllVersions, ValidValues, InvalidValues, Propto, FiniteDiff, Function, RepeatAsVector, AsScalarsVsAsVector, Length0Vector>, testing::internal::Types<boost::mpl::vector<AgradDistributionsPoisson, boost::mpl::vector<std::vector<int, std::allocator<int> >, stan::math::fvar<stan::math::fvar<double> >, empty, empty, empty, empty, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>> >::Register' requested here
INSTANTIATE_TYPED_TEST_SUITE_P(AgradDistributionsPoisson_ffd_3, AgradDistributionTestFixture, AgradDistributionsPoisson_ffd_3);
^
lib/benchmark_1.5.1/googletest/googletest/include/gtest/gtest-typed-test.h:317:11: note: expanded from macro 'INSTANTIATE_TYPED_TEST_SUITE_P'
          Register(GTEST_STRINGIFY_(Prefix),                                \
          ^
./stan/math/fwd/fun/sum.hpp:43:24: note: 'sum' should be declared prior to the call site or in an associated namespace of one of its arguments
inline value_type_t<T> sum(const T& m) {                   ^

It looks like this is a clang specific error, but calling stan::math::sum() explicitly ala

  T_partials_return logp = stan::math::sum(multiply_log(n_val, lambda_val));

also gives an error that it can't find the correct sum function (it want's the fvar<> eigen version) gist). Though looking at the candidates it's not even finding the fvar version so I suspect this is something to do with a nasty include order. I think this is happening because prim/prob.hpp is being loaded too soon and I think I found the footgun in rev/fun/multiply.hpp which does

#include <stan/math/prim.hpp>

and that should be

// Or whatever functions it specifically needs but not /prob.hpp!
#include <stan/math/prim/fun.hpp>

I'm pretty sure I did that so my b. I fixed it over here, though I think just doing the above could work.

https://github.com/stan-dev/math/compare/fix/include-stuff-for-eigen-exprs

@t4c1
Copy link
Contributor Author

t4c1 commented Nov 20, 2020

Thanks, that seems to work

Copy link
Member

@bbbales2 bbbales2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bunch o' comments and questions. Nothing big. Code looks good.

template <typename T, require_t<std::is_rvalue_reference<T&&>>* = nullptr,
std::enable_if_t<
static_cast<bool>(Eigen::internal::traits<std::decay_t<T>>::Flags&
Eigen::NestByRefBit)>* = nullptr>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is a NestByRefBit? This should probably go in the docs. I didn't find it searching Google

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eigen is stuck in C++03, in which rvalue references and perfect forwarding do not exist. So when constructing expressions, they can not know which arguments are rvalues and might go out of scope before the expression is evaled. Instead they decide which types to store by reference and which by value depending on the argument type. Lightweight objects, such as operations do not set NestByRefBit and are stored by value. Heavy objects, such as matrices set NestByRefBit and are stored by reference.

@bbbales2
Copy link
Member

bbbales2 commented Dec 7, 2020

@t4c1 two more questions

@stan-buildbot
Copy link
Contributor


Name Old Result New Result Ratio Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan 3.7 3.44 1.08 7.02% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan 0.02 0.02 0.99 -0.81% slower
eight_schools/eight_schools.stan 0.11 0.12 0.95 -5.16% slower
gp_regr/gp_regr.stan 0.17 0.15 1.1 9.47% faster
irt_2pl/irt_2pl.stan 5.8 5.81 1.0 -0.13% slower
performance.compilation 87.22 86.11 1.01 1.27% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan 8.43 8.41 1.0 0.14% faster
pkpd/one_comp_mm_elim_abs.stan 29.76 30.05 0.99 -0.95% slower
sir/sir.stan 129.57 124.76 1.04 3.72% faster
gp_regr/gen_gp_data.stan 0.05 0.04 1.06 5.25% faster
low_dim_gauss_mix/low_dim_gauss_mix.stan 2.93 2.94 1.0 -0.31% slower
pkpd/sim_one_comp_mm_elim_abs.stan 0.4 0.4 1.02 2.13% faster
arK/arK.stan 1.76 1.77 1.0 -0.24% slower
arma/arma.stan 0.74 0.58 1.27 21.12% faster
garch/garch.stan 0.61 0.6 1.02 1.66% faster
Mean result: 1.03477853458

Jenkins Console Log
Blue Ocean
Commit hash: 8dfd5b0


Machine information ProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010

CPU:
Intel(R) Xeon(R) CPU E5-1680 v2 @ 3.00GHz

G++:
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

Clang:
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

@bbbales2 bbbales2 merged commit 6b2fd28 into stan-dev:develop Dec 8, 2020
@bbbales2
Copy link
Member

bbbales2 commented Dec 8, 2020

Alright, expressions are tested and should work so I merged

But @rok-cesnovar @SteveBronder @andrjohns @wds15 if you see something weird, say something. This is a pretty big new feature!

@rok-cesnovar rok-cesnovar deleted the return_expressions2 branch December 8, 2020 13:55
hsbadr added a commit to hsbadr/math that referenced this pull request Dec 9, 2020
…sions2"

This reverts commit 6b2fd28, reversing
changes made to 0d706ef.
@hsbadr
Copy link
Member

hsbadr commented Dec 9, 2020

@t4c1 @bbbales2 @SteveBronder Could be irrelevant (as I'm using unsupported development combination), but I wanted to report it anyway. I know that rstan isn't up to date with stan and math, but this PR breaks the development version for me. Reverting it fixes the model compilation errors, even with the current development version of math. Template functions are rejected because at least one template argument could not be deduced. Here's an example when compiling EpiNow2 (note that I get similar errors when building rstanarm too):

stanExports_simulate_secondary.h(1594): error: no instance of function template "model_simulate_secondary_namespace::report_rng" matches the argument list
            argument types are: (Eigen::VectorBlock<const Eigen::Matrix<double, -1, 1, 0, -1, 1>, -1>, const std::vector<double, std::allocator<double>>, const int, boost::random::ecuyer1988, std::ostream *)
                              report_rng(tail(secondary, (all_dates ? t : h )), get_base1(rep_phi, i, "rep_phi", 1), model_type, base_rng__, pstream__),
                              ^
stanExports_simulate_secondary.h(965): note: this candidate was rejected because at least one template argument could not be deduced
  report_rng(const Eigen::Matrix<T0__, Eigen::Dynamic, 1>& reports,
  ^
          detected during:
            instantiation of "void model_simulate_secondary_namespace::model_simulate_secondary::write_array(RNG &, std::vector<double, std::allocator<double>> &, std::vector<int, std::allocator<int>> &, std::vector<double, std::allocator<double>> &, bool, bool, std::ostream *) const [with RNG=boost::random::ecuyer1988]" at line 1625
            instantiation of "void model_simulate_secondary_namespace::model_simulate_secondary::write_array(RNG &, Eigen::Matrix<double, -1, 1, 0, -1, 1> &, Eigen::Matrix<double, -1, 1, 0, -1, 1> &, bool, bool, std::ostream *) const [with RNG=boost::random::ecuyer1988]" at line 138 of "/mnt/local_drive/badr/r/lib/R/library/StanHeaders/include/src/stan/model/model_base_crtp.hpp"
            instantiation of "void stan::model::model_base_crtp<M>::write_array(boost::random::ecuyer1988 &, Eigen::VectorXd &, Eigen::VectorXd &, bool, bool, std::ostream *) const [with M=model_simulate_secondary_namespace::model_simulate_secondary]"

@rok-cesnovar
Copy link
Member

rok-cesnovar commented Dec 9, 2020

Thanks for the report. Is this using the old stan to c++ compiler? Did you use what rstan uses but just swapped out the math?

@bbbales2
Copy link
Member

bbbales2 commented Dec 9, 2020

Yeah thanks for checking this. For clarification, is this breaking the current rstan develop branch? Or a custom rstan branch with a manually updated math?

@hsbadr
Copy link
Member

hsbadr commented Dec 9, 2020

This breaks a custom rstan development version that fixes some issues with R-Devel , uses the development version of math, RcppEigen, and RcppParallel – linked to the system (Intel OneAPI) TBB. Everything is tested and working properly, when this PR is reverted.

PS: I'd like to push my changes in PRs but need time to assure backward compatibility.

@rok-cesnovar
Copy link
Member

The problem here is that this math version will only work properly for all cases with the latest stanc3 version. That will be available to rstan in the next release and hopefully in the next few weeks on Github. Before that happens you need to revert this PR on your fork.

@hsbadr
Copy link
Member

hsbadr commented Dec 9, 2020

Sounds good. Thanks @rok-cesnovar!

@rok-cesnovar
Copy link
Member

The problem is that user defined functions need to be generated in such a way that they are able to support input Eigen expressions. And stanc2, which is whar cran rstan uses, does not have that support.

@hsbadr
Copy link
Member

hsbadr commented Dec 9, 2020

I see. Looking forward to seeing rstan supports stanc3. The current development version on GitHub does not.

@hsbadr
Copy link
Member

hsbadr commented Dec 14, 2020

@rok-cesnovar I confirm that modifying rstan::stanc to generate C++ code using stanc3 fixed the problem.

See hsbadr/rstan@114552e.

@rok-cesnovar
Copy link
Member

That is great to hear! Thanks.

@hsbadr
Copy link
Member

hsbadr commented Dec 16, 2020

That will be available to rstan in the next release and hopefully in the next few weeks on Github.

@rok-cesnovar Where can I follow the active development of stanc3 support in rstan?

@rok-cesnovar
Copy link
Member

I believe its this: https://github.com/stan-dev/rstan/tree/try_2.25

and its currently blocked by stan-dev/stanc3#762

@hsbadr
Copy link
Member

hsbadr commented Dec 16, 2020

Thanks @rok-cesnovar! That helps.

hsbadr added a commit to hsbadr/math that referenced this pull request Feb 7, 2021
Partially revert stan-dev#2190.

Signed-off-by: Hamada S. Badr <[email protected]>
hsbadr added a commit to hsbadr/math that referenced this pull request Feb 7, 2021
Introduce `USE_STANC3` macro to branch changes specific to Stan 2.26
that cannot be supported by the older version of `stanc` transpiler.

Partially reverts stan-dev#2190.

Signed-off-by: Hamada S. Badr <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants