Using std::pow inside stan::math namespace

Developers
21

This seems related: https://github.com/stan-dev/math/pull/1684 . Just adding it here for whenever we discuss this again in the future.

Edit: Also relevant: https://github.com/stan-dev/math/pull/1765

1 Like
22

It’s a couple months till the next release so probably time to talk through this issue if it’s going to be resolved in this window.

The question here is:

  • How should stan::math handle implementations of functions for which there are versions in the std namespace?

I think the two workable options are:

  1. If a function is available via ADL in std, it should not be available in stan::math to avoid ambiguities in lookup.

  2. If a function is available via ADL in std, it should be included in stan::math via a using-declaration so that even though the function is available in both places, there is no ambiguity in the lookup.

1 is what we do now. The implication is that any time we use a stan::math function that is also defined in std, we need to add using-declarations for the std functions (for Stan this means adding using statements to the generated model code).

2 is how C++ handles ambiguity between C math functions that are also defined in C++.

The downside to 1 is there will presumably be weird errors if someone isn’t adding the right set of using statements to their code. In this case we should document what using statements they may need based on which functions in Stan have this ambiguity.

The downside to 2 is that using things from one namespace in another is discouraged in C++ pretty generically. If we go with 2 we should figure out what problems might come up and document them (Using std::pow inside stan::math namespace) (or if they’re bad enough 1 is the only choice).

I guess out two options on what to do are:

  1. Pick a policy, figure out how to implement it, and write it down somewhere.

  2. Punt on this decision (I think the stuff referenced in this thread has all been handled one way or another).

Of the technical solutions, I like 2 if we were starting from scratch. Given that we’re closer to implementing 1 and it seems like the code is all working, I think I lean more to 1 just for simplicity.

I don’t think it would take a huge amount of time to verify the 1 implementation. It basically requires checking which functions in stan::math exist in std, writing that list down somewhere with an explanation of what goes wrong if the right usings are not used.

Adding @syclik

23

@bbbales2, I’m a little confused from your question. I think some of the terminology used is not used properly and that’s making it unclear in spots… this particular issue depends on us really being clear on what things mean. I’m particular, the use of “specialization” is throwing me off.

Do you have a concrete example? That might help ground this.

Also, just so we’re calibrated, we should revisit argument dependent lookup and unqualified name lookup as a refresher.

No matter what we do, we should document the principles on the Math wiki so it can be found.

24

@syclik this was the issue: https://github.com/stan-dev/math/issues/1809 that spawned this discussion.

You’re right I’m using the word specialization wrong.

25

I’ll take a look at the issue.

Mind clarifying what you mean in the question? Or is it going to be clear in the issue?

26

The difference were talking about here is the difference between solving the problem with this:

This is the fix to the issue going by solution 1: [WIP] Add `using std::pow` to model code generator (Issue #1809 in math) by bbbales2 · Pull Request #2906 · stan-dev/stan · GitHub

This is the fix to the issue going by solution 2: Added arithmetic version of pow (Fixes: #1809) by bbbales2 · Pull Request #1810 · stan-dev/math · GitHub

Notice the first is a change to stan-dev/stan (this is patching the stanc2 compiler – a similar change would be in stanc3). The second change is in Math.

Or is it going to be clear in the issue?

I think start with the issue and ask if you don’t see.

This all started with code that was failing on windows but passing on Linux and so our first solutions with some #ifdefs and such. It’s definitely an ADL thing.

27

Specifically the problem started because:

  1. stan offered a version of std::pow which worked with int arguments with one promotion (via vars and complex types I believe)

  2. C offered a version of std::pow which worked with int arguments with one promotion (promoting to double)

  3. C++ offered a version of std::pow that worked with int arguments because the specification allows for this (https://en.cppreference.com/w/cpp/numeric/math/pow).

And somewhere in a test model we did something like std::pow(int, int) and there were ambiguities.

28

Wouldnt

template<typename T1, typename T2,
         typename = require_arithmetic_t<T1>,
         typename = require_arithmetic_t<T2>>
inline double pow(T1 x, T2 y) {
  using std::pow;
  return std::pow(x, y);
}

work in this case also? I am just not sure why this is returning double, it should probably return the return_type_t<T1,T2>?

This is how we deal with all the other functions, for example https://github.com/stan-dev/math/blob/develop/stan/math/prim/fun/log.hpp#L29

29

That’s an explicit using statement where you need it, which is in line with solution 1.

Is that pow function somewhere in stan::math? That return type double does look wrong.

30

That is from your link above: https://github.com/stan-dev/math/pull/1810/commits/bb9d624ac98865cdd2ead9867f1a08ddc82d9414

Yes, solution 1 is what we should go with then, to stay consistent.

31

Oh lol. Guilty. I’m not sure if it was intentional, but it looks like the c++ pow promotes ints to doubles: std::pow, std::powf, std::powl - cppreference.com

32

Oh yeah, that makes sense.

33

@rok_cesnovar, the version you posted doesn’t seem right for yet another reason. I’d expect the body to look like this:

using std::pow;
return pow(x, y);

rather than having both the using statement and then qualifying it.

@bbbales2, thanks for editing the post above. I think the question you posted is the one we want to answer:

I think we are currently take a different policy for the C++ numerics library vs Boost or Eigen. I don’t think we want to consolidate that, but right now, I think we treat things differently.

Just so we’re on the same page, I’m going to try to write things down that I think are factual. If they aren’t, mind letting me know?

(for brevity, I’ll refer to the Stan Math library as “Math”)

  • Math implements a lot of the math functions that’s defined in the C++ numerics library inside the stan::math namespace for Math’s types: stan::math::var, stan::math::fvar<T> (where T can be double or stan::math::var).
  • Math does not implement math functions defined in the C++ numerics library for primitive types in the stan::math namespace except where there are places where we can’t compile when using a mix of double and int. (This should be rare.)
  • We should not be extending the std namespace. That includes adding additional declarations to existing C++ numeric functions. (Extending the namespace std - cppreference.com) This means where we have problems with compiling double and int in arguments, we need to fix outside of the std namespace. There are a few places where we’ve extended the std namespace and that should have been out of necessity.

There’s no philosophical reason we can’t have all of the C++ numerics library defined in the stan::math namespace, even for primitives. It really comes down to whether you want to do be able to do what you highlighted:

#include <stan/math.hpp>

int main() {
  using namespace stan::math;
  pow(1.0, 1);
  return 0;
}

or if that should just not compile because there’s no using std::pow. (This one is actually a little more complicated because I think there’s a ::pow defined sometimes.)

Let’s say we took option 2 and brought in all the definitions into stan::math. If we did that, then the generated code could actually qualify every call to be stan::math instead of having usings and unqualified calls. That’s kinda nice… it’d be really easy to swap code with primitives and Math variables. I do think we’ll have to think through what happens if someone writes code that has using namespace std; before using namespace stan::math or vice versa. Or if they mix in a using namespace boost::math; with std and stan::math.

I’m going to guess that there’s actually a different issue if you’re having trouble with std::complex. When we looked at it in depth a couple years ago, we found that there were qualified calls to C++ numerics functions in the std namespace in the implementation of functions that took std::complex<T> which should have been unqualified. And this will break no matter what because the value_type inside std::complex is now in the stan::math namespace. And we really need to call the stan::math function since we’re not supposed to extend the std namespace with additional definitions. (Using std::complex for any type that’s not float, double, or long double is unspecified.)

If I’m thinking about it now, I’d want to bring in the primitive definitions of the C++ numerics functions into the stan::math namespace. We’ve been matching the error handling of the C++ numerics function in std and it is a little weird to not have the primitive version in the same namespace. This will reduce the ambiguity for the developers as to what implementation we’re matching our error handling to. And this will allow us to change the primitive definition if we wanted to do so.

If we do this, I think we have to actually bump up the major version since this may cause some really weird behavior dependent on the using statement order.

I know… this wasn’t an answer. Thoughts?

2 Likes
34

Yes definitely.

35

You know I hadn’t thought about Boost or Eigen like std here, but that’s a good point.

With solution 1, the expected way to using Stan is some variant of:

using std::whatever1;
using std::whatever2;
...
using stan::math;

With solution 2, nobody needs to use using statements at all. stan::math has everything built in.

Are there ordering rules here?

Also 2 gets away from the need for using statements, so if something breaks in a weird way it’s not as bad as a situation where using is required and something breaks. Still not great though.

Are there implications for external types? Is stan::math prim supposed to allow for other user-defined types than what we define (or already exist in C++)?

1 Like
36

One is correct. I couldn’t follow the second bullet point. Three is also correct.

I’m a little confused because defining stan::math::pow(...) isn’t extending the standard library. It’s overloading the function pow.

This all took a sharp turn for Stan with C++11, which defines integer forms of these functions. It was really the integer forms that led us to define our own primitive versions of functions, because they were ambiguous with promotion to double and to autodiff types like stan::math::var.

So I like solution (2) if we can make it work and I like having your example of just bringing in stan::math, also bringing in relevant standard library functions under the stan::mathnamespace. Similarly, we’d just be able to call pow(1.0, 1) in all of our own functions. Getting those includes right has sometimes been painful and it’s always verbose.

std::complex is a bit trickier because the standard library implementations vary between clang++ and g++ in terms of which functions are there for disambiguation.

For ordering, we’d just include the standard functions before any of the Stan code.

1 Like
37

Sorry about that… for the second bullet point, I was just trying to say we don’t write functions in the stan::math namespace for all primitive arguments for functions that are in the std namespace. For example, we wouldn’t have double stan::math::pow(double, int).

I’m looking at doc like Namespaces - cppreference.com. I’ll quote the part that gives me a little pause:

Notes
The using-directive using namespace std; at any namespace scope introduces every name from the namespace std into the global namespace (since the global namespace is the nearest namespace that contains both std and any user-declared namespace), which may lead to undesirable name collisions. This, and other using directives are generally considered bad practice at file scope of a header file.

I think we’re all in agreement that functions in stan::math should also be defined for primitives makes sense. We can implement most of them with using statements. I’m sure as time goes on, we may want to have different functions there.

38

The using-declaration (using std::pow;) and the using-direction (using namespace std;) behave quite a bit differently. We definitely don’t want to use the second.

39

And by “use” here I mean “use in the Stan library.”

using namespace std; is presumably something someone using the Stan library might have in their code, and that’s something different.

1 Like