-
-
Notifications
You must be signed in to change notification settings - Fork 185
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
Added arithmetic version of pow (Fixes: #1809) #1810
Conversation
@bob-carpenter are both of these candidates through implicit conversion?
|
@bob-carpenter Looks like it's blowing up on a Mac in the downstream tests (I'm guessing it's a Mac cause of the use of /usr/local/Cellar which is a brew thing): https://jenkins.mc-stan.org/blue/organizations/jenkins/Stan/detail/downstream_tests/1438/pipeline I swapped in explicitly the only overloads we need that C99 doesn't provide and to see what happens (
So in this case it's not finding the C99 pow definitions. I think this is because there's a difference in what:
and
mean. Because Rosenbrock uses the first, it works, and the tests use the second, they fail. If I modify the tests to use Check the difference in rule 6 & 7 here: https://en.cppreference.com/w/cpp/language/namespace
Okay so backing up a bit, what we want is:
I think what we need to do is inside
This gets rid of any ambiguities between Stan and std::math and in the case where we're operating in the stan::math namespace we'll hit the std::pow definition with ADL before we find the C version so there won't be ambiguities there. Edit: Aah, I don't think this explanation is quite right. If we have the code:
Then the symbols for stan::math get absorbed as if they are in the nearest enclosing namespace that contains stan::math and rosenbrock_model_namespace (which is the global namespace). I'm not sure why then there wouldn't be ambiguities between the C++ pow and the C pow. Presumably there's a rule somewhere for this though. |
I switched the implementation to:
And added a bunch of tests that I think should help catch problems during the math tests. I'm not sure I got everything though honestly. Can discuss further later if these tests pass. |
From @SteveBronder
Yes. It requires promoting an |
From @bbbales2
We absolutely cannot have a namespace-level using statement. |
We would like this to work idiomatically using argument-dependent lookup, no matter what
That means the solution may need to be having |
Well I get that this is given out as advice, but why? I think this is the behavior we want. |
Modulo we kinda define the behavior we want cause we can just change the code generation. |
I acutally meant more in the sense of wanting to work with argument-dependent lookup (ADL). That's the way to ensure that generic written code will work with Stan types dropped in. Let me repeat. The pattern we need to work to support an idiomatic ADL is:
In words, an idiomatic usage has a But, that's not going to work for us in models. The reason is that the following using statement in our models will mess us up.
With both using statements, as in
we have the problem that the then we ran into was that mingw32 didn't implement To summaize, I see two options:
From a math library client perspective, (2) is far superior. But that's going to be a huge pain for us on the model side. I think we should target getting there eventually. That would also allow us to clean up all those The ideal long-term goal for generated model code would be completely generic in that users could drop in their own types as long as they supported the basic C++ built-in arithmetic and standard library functions. That'd also require us to implement all Stan functions generically down to basic arithmetic; as is, we have lots of functions with a |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Looks like a Jenkins problem:
I restarted the performance tests. |
@serban-nicusor-toptal : No idea what's going wrong, but it appears to be a Jenkins test config issue (or general test config issue). After restarting, it failed in exactly the same place. This is confusing, too, because we got the email with results of the performance tests, but there's not a simple file with "logistic" in the name among them. At least it's progress in that I could find the error in the performance tests on my own. |
Hey, it looks like the machine that ran the tests went down. |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Thanks! Looks like it worked. I'll review and merge. |
Since it's working, I made a post about the using declaration: https://discourse.mc-stan.org/t/using-std-pow-inside-stan-math-namespace/13975 like we discussed. So we can get comments there hopefully and get this merged by the weekend if nobody finds big problems. |
So looking at all the candidate functions ^
/usr/include/x86_64-linux-gnu/bits/mathcalls.h:153:17: note: candidate function
__MATHCALL_VEC (pow,, (_Mdouble_ __x, _Mdouble_ __y));
^ ^ That's the one we want right? What if for all the ones here we just change the signatures to use SFINAE to kick them out? So for instance the first below here would just be template <typename Complex, typename Arith,
require_complex_t<Complex>* = nullptr,
require_arithmetic_t<Arith>* = nullptr>
inline std::complex<var> pow(const Complex& x, Arith y) { I think that avoids namespace issues and preserves just using the standard lookup rules
|
So basically this signature won't allow for promotions from (double, int)? |
idt int to double is the issue is it? It looks like because
Though 2 feels gross. I think the one I have above would work |
I'd have to try this out, I think. I mean, it's true we don't want doubles/ints promoting to vars through argument calls like that for efficiency, but that doesn't mean we aren't leaning on it somewhere I'm not thinking about. By (double, int) I meant the first argument is double, second is int. |
I'm not sure where this would be true and I'm like 90% sure it's actually where the error comes from. @bob-carpenter thoughts? I could be missing something here |
I think just by doing the above thing the double double version is the only possible candidate |
The discussion's now half on Discourse and half here, so I'll follow up here to comments posted on GitHub. I believe @bbbales2 figured out the right solution to this problem---have a As I keep trying to emphasize, the pure type traits version isn't workable in general because it's not specific enough and won't get chosen over library versions in cases where we need it. We might be able to come up with some hybrid solution that type traits out some of them and has other more specialized versions. We want to allow |
Should be tested here: https://github.com/stan-dev/math/pull/1810/files#diff-acb4663890da3d3806032f1554bea099R37 |
Can you show me an example where this would fail to do what we want using type traits?
I think there's some confusion on my end here. Yes i agree we want to allow promotion, but if someone does double x = 1.0;
int y = 2;
auto v = exp(x, y); I don't see why we would ever want to allow that If I can get a version of this working with type traits would that be an acceptable answer? |
The solution over here is probably the simpler stopgap to getting this to work if we don't want I'm happy with just putting Getting rid of implicit casts in pow might work, but I don't think we'd want to do this generally just cuz it means writing tons and tons of functions. |
Actually nvm I think there is more manutia then I thought. Messing around I had some weird with the namespaces that idk how to work with atm |
There's a I would suggest you try it for yourself. Maybe there's something I'm missing here. |
When is overpromotion going to be a problem? Each of the autodiff files should be including the primitive version, so I don't even see how it'll be a problem if we have the instantiations covered properly. We've decided that end users should be using our top-level includes, which take care of this all at once. Math library developers need to understand C++ include order. |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
@rok-cesnovar, @SteveBronder, @bob-carpenter, anyone want to get this? This removes the mingw |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There wa a stray space fix in opencl that probably shouldn't be in this PR.
Otherwise this is fine, but how do we stage the testing?
@@ -49,7 +49,7 @@ class load_ | |||
* Creates a deep copy of this expression. | |||
* @return copy of \c *this | |||
*/ | |||
inline load_<T&> deep_copy() const & { return load_<T&>(a_); } | |||
inline load_<T&> deep_copy() const& { return load_<T&>(a_); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a stray change that shouldn't be in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was the autoformatter, but I dunno why it looked at this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the commit: d539b58#diff-e92a842ba1a127d7587a635913cb4ce3 . Since this is the autoformatter I'm not going to bother with trying to revert it. Presumably it'd just put it back again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is related to #1822
The styling on develop was forced by an older version of clang-format. So one PR would eventually have to fix it.
The stan and stanc3 code is already in (stan-dev/stan#2906 (comment), stan-dev/stanc3#492). This is the last one. Presumably the stan stuff didn't get tested on Windows with mingw so it passed (got tested with g++). |
Stanc3 had failures on master since the complex stuff was added. Adding using std::pow fixed that so I think we can proceed here. |
@bob-carpenter this needs merged so that there aren't ambiguities between |
This should fix #1809
Checklist
Math issue Ambiguities with between C/C++/stan pow implementations #1809
Copyright holder: Columbia University
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
./runTests.py test/unit
)make test-headers
)make test-math-dependencies
)make doxygen
)make cpplint
)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested