Using std::pow inside stan::math namespace

Developers
1

@Bob_Carpenter and I were trying to iron out issues with std::pow that popped up with the introduction of the stan versions of the standard library complex functions into stan::math.

We have a solution we’d like other people to take a look at (it works – but we want to know if it is good C++ or whatever).
@syclik, @ChrisChiasson, @stevebronder, @rok_cesnovar, @mcol, @bgoodri or anyone else please have a look.

We were having problems resolving ambiguities between the stan implementations of pow, the standard C++ definitions of pow, and the C definitions of pow.

The solution we have now uses a using declaration statement to bring the std::pow implementations into the stan::math namespace. stan/math/prim/fun/pow.hpp now contains:

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

This solved the problems we were having defining custom specializations of pow in stan::math and trying to be careful about what std stuff we were using to avoid ambiguities.

Basically our question is, is what we’re doing here okay? Is there some obvious reason we wouldn’t do this? It’s standard to not do using namespace things (using-directives) in namespaces, but what about using declarations? (the differences are described here in 5 and 6 here: https://en.cppreference.com/w/cpp/language/namespace)

We’d like to resolve this in a few days, otherwise we need to revert the complex-funs pull request so we aren’t clogging up the development pipeline with our failing tests. Ideally we could have a video call tomorrow, Thursday, or Friday but comments here are good too.

Here’s a little more information about what needs to work.

There are eight use cases, and all these calls should work for a and b each being any of double, int, var, fvar<double>, fvar<var>, fvar<fvar<double>>, fvar<fvar<var>>.

Use case A:

using namespace stan::math;
using namespace std;
pow(a, b)

Use case B:

using namespace stan::math;
using std::pow;
pow(a, b);

Use case C:

using stan::math::pow;
using namespace std;
pow(a, b);

Use case D:

using stan::math::pow;
using std::pow:
pow(a, b);

Use case E:

using namespace stan::math;
pow(a, b);

Use case F:

using namespace std;
pow(a, b);

Use case G:

using stan::math::pow;
pow(a, b);

Use case H:

using std::pow;
pow(a, b);

Our four implementation options are:

  1. No arithmetic pow implementation in stan::math

  2. Arithmetic pow implementation in stan::math. No templating, only overloads.

  3. Templated arithmetic pow implementation in stan::math

  4. using std::pow in stan::math

The first implementation fails for case E and G (no arithmetic implementations in stan::math)

The second implementation fails for at least A and E (ambiguities between pow(double, double) implementations in C++, the C libraries, and stan::math).

The third implementation fails for at least A as well (ambiguities between stan::math and std::pow for arithmetic types).

The fourth implementation works everywhere (as far as we know).

2

Not that is obvious enough for me to think of. @ChrisChiasson: Did we encounter and overcome this issue in the original PR for complex numbers?

3

While we generally try to avoid having global using statements we don’t need to behave like what you are doing here is something completely new. We had global using statements for std::sqrt and std::log in the Math library up until this last version of the Math library for instance. See cleanup constants.hpp · stan-dev/math@58e801d · GitHub (These were here for at least two math versions I didnt go further back than that).

So if this is the easiest and most clean solution to this issue I think we go for it. But we maybe make an issue in the Math library if we find some miraculous solution or things go away at a later point in time.

1 Like
4

using declarations (namespace or individual names) inside header files that change what namespace a function is pulled from are generally only used inside functions, for the simple reason that if it were instead done at file scope (and therefore at translation unit scope), any header files included after the declaration (and therefore hierarchically higher ) have the meaning of their unprefixed symbols possibly changed, without any corresponding change in the source code itself. In other words, it changes the meaning of code based on the semi-random order in which you write your includes (see example below). You may think this is easy to avoid, but I assure you it is not. My opinion: put the using declarations as close to where they are used as possible: inside function bodies. Also, the other functions rok mentioned should be candidates in the future for being rewritten to not do this.

example:
—some_deep_file.h—

using foo::a;

—top_level_file_1.h—

include "some_high_level_file_that_eventually_inludes_some_deep_file.h"
include "some_other_high_level_file.h" // meaning of "a()" is modified

—top_level_file_2.h—

include "some_other_high_level_file.h" //meaning of "a()" is normal
include "some_high_level_file_that_eventually_inludes_some_deep_file.h"

BTW, this is the kind of thing that C++ modules is meant to address.

4 Likes
5

Google style guide also bans them. Abseil has a blogpost about this

1 Like
6

Yeah, this seems like it’s true (difference in call_pow1 and call_pow2: Compiler Explorer)

But the rules change with argument dependent lookup, and we are leaning on a lot of use-before-defining behavior (a function in prim can call something in rev even though rev is declared after).

Like, when you call a function with a class argument, then the namespace attached to that class is searched (Argument-dependent lookup - cppreference.com). At least, I got this working with a templated function. Class argument to non-template function didn’t work.

This is a use-before-declaring situation: Compiler Explorer

And if you pull the var definition out of that namespace it breaks.

But I guess it’s still funky cause ADL doesn’t call the right pow there (it’ll call the C pow for the double).

This non-template version breaks: Compiler Explorer .

I think it’s because ADL rules change for templates (this still works): Compiler Explorer , but I don’t know.

So I agree it seems like something complicated is going on here.

From the Google Style guide:

or using-declarations (which we ban in header files except when the imported names are part of the interface exposed by the header file in question)

std::pow as the arithmetic implementations of stan::math::pow is what we want to expose, so technically seems fine, so we can squeeze through that with an exception lol.

The gnu standard libraries depend on this behavior. Search for ‘::pow’ in here (the cmath from my laptop): cmath.hpp (47.3 KB) . Bob and I were looking at this yesterday. We think it’s how they avoid ambiguities between the C pow and the C++ pow.

I guess this commentary doesn’t clarify much.

The general resistance makes me feel like, yeah, we probably shouldn’t do this. But I’m not totally convinced we aren’t leaning on similar functionality already lol. stan::math has a lot going on, but maybe that’s an argument to avoid yet-another-trick as well.

I’m kindof into using using std::pow as a stopgap and talking through this in the context of generated code for the next release, cause I feel like our problems stem from thinking about that.

It’s either that or find another stopgap. We’d need to find that stopgap quickly too, or we’ll need to take complex-funs out for the release, and I really don’t want Bob or anyone to have to maintain the complex-funs things separately from stan::math in a way that we can merge it whenever we figure this out. That was a bear for Bob to get in.

7

Right. But I think we want to exploit that as a feature. As Ben showed me, it’s how the standard library in C++ deals with compatibility with the C99 library. They have using statements that bring the simpler definitions into the std:: namespace.

That’s generally recommended as good practice. But in the approach @bbbales2 is suggesting, I don’t see how they could be placed inside function bodies—the point’s to bring the standard library functions into the stan::math namespace.

Don’t give up yet. My reaction was the same as everyone else’s until I understood exactly what was going on with the using namespace stan::math in our generated model code and with using std::pow at stan::math namespace scope.

Yes. We want to implement the same solution for all the standard library functions like this.

8

Actually I forgot. I think we have another stopgap cause we can just do the using std::log inside the generated code.

It doesn’t seem like using std::pow inside the Stan namespace is quite the slam dunk I’d hoped, and putting it in the generated code is in line with what everyone is saying (and I think fixes the problem). I’ll put a pull request up to test this.

9

Oof, those race conditions.

10

Now this thread makes sense to me

1 Like
11

BTW, if this is done, putting it after all the includes is the way to go. That way the math library operates without having the meaning of its internal symbols changed.

12

The point is to put it first precisely so that the overloads are available both internally and externally. This is what the standard library code does for all the C99 symbols they want to import into the std:: namespace. To be more specific:

namespace stan {
namespace math {
using std::pow;  // after this, stan::math::pow has all std::pow definitions available

// now extend the basics for other types
inline var pow(const var& x, const var& y) { ... };
...
}}

At least that’s how the standard library code’s doing this in either clang or gcc (can’t remember which @bbbales2 was showing me).

13

What I mean is that if the stanc3 compiler has generated a model (I don’t actually work with it, so I don’t know what it does):
I presume it generates something like:

include <some file from stan-math>
include <other file from stan-math>
include <etc>

int main(){
 //generated calculations here
}

In this case I was suggesting that the using statements, which I presume are necessary for the “generated calculations” part, should go after the includes and the before main.

This takes care of the use case Ben needed to solve, where the functions in the generated code could not find the standard namespace or whatever other namespace.

Putting the using statements before the includes will alter the way the system compiles stan-math when it is generating models, such that the results of the unit tests in stan math may no longer match the “same” function calls done in a model.

If the idea is instead to “fix” the stan-math unit tests themselves by putting using statements in the headers, then I stand by my original recommendation of putting the using statements in the functions for the reasons stated previously.

14

Hoooray! It’s decision day!

I think the least invasive thing that solves this problem is adding using stan::pow to the generated code (this pull: https://github.com/stan-dev/stan/pull/2906).

But that only handles the stanc2 compiler, I’ll also need help figuring out how to do that in the stanc3 compiler.

That also means removing the mingw thing from Math as well.

We need to talk through the interaction between the std and stan::math namespaces and how we use this all in code gen whichever thing we do, and this covers us for Stan’s generated code without really doing anything to the math library so that seems nice.

If I hear nothing back by 6PM EST today, we’ll just roll with this.

15

I can handle that one.

1 Like
16

The generated code using std::log is in in ([WIP] Add `using std::pow` to model code generator (Issue #1809 in math) by bbbales2 · Pull Request #2906 · stan-dev/stan · GitHub) and there’s a pull to remove the mingw stuff from stan::math (Added arithmetic version of pow (Fixes: #1809) by bbbales2 · Pull Request #1810 · stan-dev/math · GitHub).

@rok_cesnovar can you add a using std::log into the stanc3 code generator? The top of the .hpp generated by the old compiler looks like:

// Code generated by Stan version 2.22.0                                                                                                                                                                           

#include <stan/model/model_header.hpp>

namespace rosenbrock_model_namespace {

using std::istream;
using std::string;
using std::stringstream;
using std::vector;
using std::pow;
using stan::io::dump;
using stan::math::lgamma;
using stan::model::prob_grad;
using namespace stan::math;

static int current_statement_begin__;

This makes me wonder though – should stanc3 somehow be built into the upstream tests? We’re using stanc2 right now and that seems like we might accidentally miss things.

I made myself a reminder to come back after the release and schedule a meeting to talk about the interaction of std and stan::math.

17

Do you mean std::pow? Just checking, I am a bit confused now :)

Yes it should. But that will be quite a big task. Cmdstan also still uses the old compiler for the interface tests and needs to switch to stanc3.

For now we rely on catching that in the stanc3 tests that try to compile the mother model and a bunch of example models into executables.

But yeah, this needs to become a part of the Math upstream sooner rather than later.

18

PR open: https://github.com/stan-dev/stanc3/pull/492

If someone has time to review please do.
One liner change and no OCaml knowledge needed.

19

Ooof, yes lol.

Thanks! I see @seantalts got it in.

1 Like
20

That’s indeed where they’re going, though it’s a class definition, not a main in the generated code (the main’s in another translation unit). It’s also where cpplint insists the standard library includes go—after all the other includes.

1 Like