Elementwise pow functions not compiling at the C++ level #661

rok-cesnovar · 2020-08-03T09:30:26Z

Unfortunately, this is a bug in the release version which will most likely warrant a hotfix release.

This test model https://github.com/stan-dev/stanc3/blob/master/test/integration/good/function-signatures/math/matrix/elementwise_pow.stan compiles to C++, but the generated C++ does not compile with:

examples/bugs/bug.hpp:3464:41: note:   ‘std::vector<std::vector<std::vector<Eigen::Matrix<double, 1, -1> > > >’ is not derived from ‘const Eigen::ArrayBase<Derived>’
 3464 |         pow(p_row_vector_array_3d, d_int),
      |                                         ^
In file included from stan/lib/stan_math/lib/eigen_3.3.7/Eigen/Core:533,
                 from stan/lib/stan_math/lib/eigen_3.3.7/Eigen/Dense:1,
                 from stan/lib/stan_math/stan/math/prim/fun/Eigen.hpp:22,
                 from stan/lib/stan_math/stan/math/rev.hpp:4,
                 from stan/lib/stan_math/stan/math.hpp:19,
                 from stan/src/stan/model/model_header.hpp:4:
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/GlobalFunctions.h:161:3: note: candidate: ‘template<class Scalar, class Derived> typename Eigen::internal::enable_if<((! Eigen::internal::is_same<typename Derived::Scalar, Scalar>::value) && Eigen::internal::has_ReturnType<Eigen::ScalarBinaryOpTraits<Scalar, typename Derived::Scalar, Eigen::internal::scalar_pow_op<Scalar, typename Derived::Scalar> > >::value), const Eigen::CwiseBinaryOp<Eigen::internal::scalar_pow_op<Scalar, typename Eigen::internal::traits<_Rhs>::Scalar>, const typename Eigen::internal::plain_constant_type<Derived, Scalar>::type, const Derived> >::type Eigen::pow(const Scalar&, const Eigen::ArrayBase<ExponentDerived>&)’
  161 |   pow(const Scalar& x, const Eigen::ArrayBase<Derived>& exponents)
      |   ^~~
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/GlobalFunctions.h:161:3: note:   template argument deduction/substitution failed:
examples/bugs/bug.hpp:3464:41: note:   mismatched types ‘const Eigen::ArrayBase<ExponentDerived>’ and ‘const int’
 3464 |         pow(p_row_vector_array_3d, d_int),
      |                                         ^
In file included from stan/lib/stan_math/lib/eigen_3.3.7/Eigen/Core:533,
                 from stan/lib/stan_math/lib/eigen_3.3.7/Eigen/Dense:1,
                 from stan/lib/stan_math/stan/math/prim/fun/Eigen.hpp:22,
                 from stan/lib/stan_math/stan/math/rev.hpp:4,
                 from stan/lib/stan_math/stan/math.hpp:19,
                 from stan/src/stan/model/model_header.hpp:4:
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/GlobalFunctions.h:169:3: note: candidate: ‘template<class Derived> const Eigen::CwiseBinaryOp<Eigen::internal::scalar_pow_op<typename Derived::Scalar, typename Eigen::internal::traits<T>::Scalar>, const typename Eigen::internal::plain_constant_type<Derived, typename Derived::Scalar>::type, const Derived> Eigen::pow(const typename Derived::Scalar&, const Eigen::ArrayBase<Derived>&)’
  169 |   pow(const typename Derived::Scalar& x, const Eigen::ArrayBase<Derived>& exponents)
      |   ^~~
stan/lib/stan_math/lib/eigen_3.3.7/Eigen/src/Core/GlobalFunctions.h:169:3: note:   template argument deduction/substitution failed:
examples/bugs/bug.hpp:3464:41: note:   mismatched types ‘const Eigen::ArrayBase<Derived>’ and ‘const int’
 3464 |         pow(p_row_vector_array_3d, d_int),
      |                                         ^
examples/bugs/bug.hpp:3468:42: error: no matching function for call to ‘pow(const std::vector<std::vector<std::vector<Eigen::Matrix<double, 1, -1> > > >&, double&)’
 3468 |         pow(d_row_vector_array_3d, p_real),
      |                                          ^
In file included from stan/lib/stan_math/lib/eigen_3.3.7/Eigen/Core:96,
                 from stan/lib/stan_math/lib/eigen_3.3.7/Eigen/Dense:1,
                 from stan/lib/stan_math/stan/math/prim/fun/Eigen.hpp:22,
                 from stan/lib/stan_math/stan/math/rev.hpp:4,
                 from stan/lib/stan_math/stan/math.hpp:19,
                 from stan/src/stan/model/model_header.hpp:4:
/usr/include/c++/9/complex:1889:5: note: candidate: ‘template<class _Tp, class _Up> std::complex<typename __gnu_cxx::__promote_2<_Tp, _Up>::__type> std::pow(const std::complex<_Tp>&, const std::complex<_Up>&)’

The issue is the using std::pow in the generated code model code. Removing that fixes it, but need to double-check that does not corrupt anything else.

The text was updated successfully, but these errors were encountered:

bbbales2 · 2020-08-03T14:16:22Z

Do you understand why removing tusing std::pow fixes the problem?

If we don't have that, then presumably this comes back.

bbbales2 · 2020-08-03T14:53:04Z

I'm not sure yet exactly why:

using std::pow;
using namespace stan::math;

causes problems now. My suspicion is something with how templates come in to ADL that I wasn't understanding in the previous discussion (https://discourse.mc-stan.org/t/using-std-pow-inside-stan-math-namespace/13975/6).

The things that compile are:

using std::pow;
using stan::math::pow;
using namespace stan::math;

in the generated code, or moving all the using declarations (everything except using namespace stan::math) outside the model namespace in the generated code or inserting

using std::pow

inside the stan::math namespace in stan/math/prim/fun/pow.hpp (in which case using std::pow in the model generated code still causes an error but it can be removed now).

All three of those should solve both problems. I'm going to have to think about ADL and why this is failing in the first place and why these are fixes cuz I don't really understand.

bbbales2 · 2020-08-03T14:54:03Z

All three of those should solve both problems

What am I saying lol I don't know that anymore.

rok-cesnovar · 2020-08-03T14:55:35Z

There are two ways of fixing this:

add a using stan::math::pow;
remove using std::pow;

I think this line here: https://github.com/stan-dev/math/blob/develop/stan/math/prim/fun/pow.hpp#L44
fixes the ambiguity issue.

Just to be safe I am running a test locally that will compile all 850+ models in the tests here, including https://github.com/stan-dev/stanc3/blob/master/test/integration/good/function-signatures/math/functions/pow.stan

Gonna take awhile and already uncovered other stuff we need to fix.

bbbales2 · 2020-08-03T18:09:25Z

I did some more looking. It looks like the older error was platform dependent. It didn't always trigger in Linux.

I think the issue was only the C99 pow (definitions here) + the Stan pows were available for some compiler configurations, and so something like pow(int, int) or pow(double, int) failed because there's ambiguity (we can get to pow(double, double) or pow(var, double) in one cast).

The trick was to expose the later pows with using statements.

Gonna take awhile and already uncovered other stuff we need to fix.

What other issues are coming up?

I think the issue now is that we are defining specializations for pow inside stan::math which include arguments which themselves are not defined in stan::math.

Originally (before #1809), we had code like:

namespace model {
  using namespace stan::math;
  double d_double = 0.0;
  int d_int = 0;
  var d_var = 0.0;

  pow(d_int, d_double); // call this statement 'A'
  pow(d_var, d_double); // call this statement 'B'
}

This caused problems sometimes because the pow signature required by A was ambiguous (pow(int, double) wasn't defined, and the compiler could cast to pow(double, double) or pow(var, double) or something).

We added using std::pow to the code to get:

namespace model {
  using std::pow;
  using namespace stan::math;
  double d_double = 0.0;
  int d_int = 0;
  var d_var = 0.0;

  pow(d_int, d_double); // call this statement 'A'
  pow(d_var, d_double); // call this statement 'B'
}

Now what happens is the definitions of std::pow are included inside the model namespace as if they are defined there, and there is an unambiguous definition for pow(int, double).

The weirdness is happening with statement 'B'.

Statement 'B' works not because the using namespace stan::math, but because pow(d_var, d_double) includes a var argument. In ADL, functions defined in the namespace immediately enclosing the definition of classes of the arguments to a function call are added to the list of functions to consider.

Because of this, all the stan::math::pow functions are considered alongside the std::pow definitions and the correct one gets picked.

Now the code is this:

namespace model {
  using std::pow;
  using namespace stan::math;
  double d_double = 0.0;
  int d_int = 0;
  var d_var = 0.0;
  std::vector<double> d_vector = { 0.0 };

  pow(d_int, d_double); // call this statement 'A'
  pow(d_var, d_double); // call this statement 'B'
  pow(d_vector, d_double); // call this statement 'c'
}

'A' works because using std::pow is using declared in the same namespace.

'B' works because there is a var argument, so stan::math::pow and std::pow are both considered by ADL and the correct function gets picked.

'C' fails because using std::pow gives it a list of functions to consider in the current namespace, and it looks no farther. using namespace stan::math imports stan::math::pow (and all the other functions) at the namespace level that contains both the current namespace (model) and the stan::math namespace. So this is file scope.

There's another weird fix for this -- if you remove the model namespace in the generated code of a Stan model, the code will also compile. In this case using std::pow puts the std::pow definition at file scope. That's also where the using namespace stan::math puts the definitions, so it works.

I still think adding using std::pow inside the stan::math namespace in stan/math/prim/fun/pow.hpp is the best move but it's not perfect since we're declaring pow signatures for std types.

rok-cesnovar · 2020-08-03T18:32:03Z

It would be great if we had a solution that only required touching stanc3 to hotfix this. It makes the hotfix release really simple. Would including both using std::pow and stan::math::pow work if I understand correctly what you are saying?

I think the longterm solution would be to just define pow(double, double), pow(int, int) and the rest in Math. Like we do for unary functions: https://github.com/stan-dev/math/blob/develop/stan/math/prim/fun/acos.hpp

What other issues are coming up?

Two issues came up. #662 is a longstanding issue, I checked 2.23 and it had the same issue.

The other one is that the linspaced_* utility functions should be data only (they work with doubles only in math). I have a fix ready for this, looking at #662 now.

bob-carpenter · 2020-08-03T18:36:15Z

I think the longterm solution would be to just define pow(double, double), pow(int, int) and the rest in Math.

If you look at the long history, we've gone back and forth on this by either hardcoding std::foo(...) in the generated code where necessary vs. defining stan::math::foo(...) in the math lib. The problem with the latter is that too eager includes and usings can mess things up. It all feels very brittle and always has.

rok-cesnovar · 2020-08-03T18:51:49Z

That is true yeah. Someone adding a global using statement can mess things in a bunch of other files (we should not encourage that but can happen). The problem is that we will always have some stan::math::foo(...) functions that shadow std::foo(...) because we want to differ from std for some of them.

We could list the exceptions and hardcode std:: in code-gen for the rest. Not sure what would be ideal :/. Testing does not help catch everything here, as these include are very OS-dependent.

bbbales2 · 2020-08-03T19:23:33Z

Would including both using std::pow and stan::math::pow work

I think so.

For all pows, they get to see all the std and all the stan::math code in that case.

the longterm solution would be to just define pow(double, double), pow(int, int) and the rest in Math

using std::pow inside stan::math does this and the added bonus is there are no ambiguities between std::pow(double, double) and stan::math::pow(double, double) then (they're the same thing and the compiler knows it).

we want to differ from std for some of them.

This is surprising to me. Do you know an example?

rok-cesnovar · 2020-08-03T19:29:36Z

using std::pow inside stan::math

Yes, that sounds like a good call.

This is surprising to me. Do you know an example?

I think its exclusively edge cases (NaN and Inf and such). Example https://github.com/stan-dev/math/blob/develop/stan/math/prim/fun/atanh.hpp

rok-cesnovar mentioned this issue Aug 4, 2020

Bugfix/elementwise pow and empty matrices #664

Merged

rok-cesnovar closed this as completed in #664 Aug 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Elementwise pow functions not compiling at the C++ level #661

Elementwise pow functions not compiling at the C++ level #661

rok-cesnovar commented Aug 3, 2020 •

edited

bbbales2 commented Aug 3, 2020

bbbales2 commented Aug 3, 2020

bbbales2 commented Aug 3, 2020

rok-cesnovar commented Aug 3, 2020

bbbales2 commented Aug 3, 2020

rok-cesnovar commented Aug 3, 2020

bob-carpenter commented Aug 3, 2020

rok-cesnovar commented Aug 3, 2020

bbbales2 commented Aug 3, 2020

rok-cesnovar commented Aug 3, 2020

Elementwise pow functions not compiling at the C++ level #661

Elementwise pow functions not compiling at the C++ level #661

Comments

rok-cesnovar commented Aug 3, 2020 • edited

bbbales2 commented Aug 3, 2020

bbbales2 commented Aug 3, 2020

bbbales2 commented Aug 3, 2020

rok-cesnovar commented Aug 3, 2020

bbbales2 commented Aug 3, 2020

rok-cesnovar commented Aug 3, 2020

bob-carpenter commented Aug 3, 2020

rok-cesnovar commented Aug 3, 2020

bbbales2 commented Aug 3, 2020

rok-cesnovar commented Aug 3, 2020

rok-cesnovar commented Aug 3, 2020 •

edited