Skip to content

Investigate which int overloads are still needed (exp, pow, ...) #1692

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

Closed
rok-cesnovar opened this issue Feb 10, 2020 · 4 comments · Fixed by #1765
Closed

Investigate which int overloads are still needed (exp, pow, ...) #1692

rok-cesnovar opened this issue Feb 10, 2020 · 4 comments · Fixed by #1765
Milestone

Comments

@rok-cesnovar
Copy link
Member

Description

My lab mate asked me about the reasoning behind having exp(int) defined in the Math library.

Example: inline double exp(int x) { return std::exp(x); } in https://github.com/stan-dev/math/blob/develop/stan/math/prim/fun/exp.hpp#L18

I could not find a reason so I am wondering myself also. Can anyone help me understand the reasoning behind this.

This came up while trying to add a custom function in stan math and using something like exp(3.3) inside the stan::math namespace was being truncated down to exp(3). This was due to a missing using std::exp; that would have been picked if he used cpplint.

Current Version:

v3.1.0

@bob-carpenter
Copy link
Member

That problem hadn't occurred to me!

The reason we have the int signature is to avoid ambiguity with exp(int) when std::exp() and stan::math::exp() are in scope. None of them have integer signatures, so an int either needs to be promoted to double or to var or fvar.

We could get around this by having our math lib version be defined as something like:

template <typename T, typename = std::enable_if_t<std::is_arithmetic<scalar_type_t<T>>::value>>
inline T exp(T x) {
  return std::exp(x);
}

Then it won't try to cast double to int, while still being a better match for exp(int) than std::exp().

@bob-carpenter
Copy link
Member

OK, I just went and looked up the signatures of exp and as of C++11, there's an exp(IntegralType). That must not cause ambiguities with our exp(int), but it probably means we can get rid of our exp(int) definition without problems.

@rok-cesnovar
Copy link
Member Author

while still being a better match for exp(int) than std::exp()

But stan::math::exp still uses std::exp so these would be the same in the end right? So I guess this was to prevent int being promoted to var?

@bob-carpenter
Copy link
Member

bob-carpenter commented Feb 11, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants