-
-
Notifications
You must be signed in to change notification settings - Fork 368
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
Widen int to 64-bit signed integer in generated code #2381
Comments
I almost have Stan Math converted in a safe-ish way (On a Mac, you have to put
The remaining occurrences of the "word"
|
Wow, that's awesome. I think we should be able to patch this up pretty easily. Not sure how to verify that the tests do what we want, but I suppose we should just trust our unit and integration tests at this point. @syclik ---you've done a lot of this before---any ideas? We'll have to cleanup the residuals---assigning an I'm pretty sure Eigen uses |
Yeah, we should trust our tests on this one.
This sort of thing takes a lot of time and energy. The only real suggestion I have is if it's possible to do it in chunks, commit to your branch often. Most of the effort will have to be after you've made changes and really getting to the edge cases. Test often. Write and add new tests if it helps. I use every roll I know how to use: diff3, emacs, GitHub's diff, git status. I tend to review every change at least a handful of times before submitting a pull request--an even then I miss things, but at that point you have help from the rest of the developers.
… On Oct 22, 2017, at 7:49 PM, Bob Carpenter ***@***.***> wrote:
Wow, that's awesome. I think we should be able to patch this up pretty easily. Not sure how to verify that the tests do what we want, but I suppose we should just trust our unit and integration tests at this point. @syclik ---you've done a lot of this before---any ideas?
We'll have to cleanup the residuals---assigning an int64_t to an int will be just fine by C++, but we want to eliminate all of that. So I'm surprised Eigen is balking at all here.
I'm pretty sure Eigen uses ptrdiff_t for indexing by default, but that's one of those platform-dependent types, as opposed to int64_t (to which it resolves on most modern platforms). We might have some badly behaved uses of Eigen that the default won't catch---can't we define that in the makefile?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Okay, I can trigger this outside of Stan by trying to compile
I get:
|
Upon further review, the problem seems to be although inserting
allows the rows and / or columns of a dynamically sized
is hard-coded for |
Yes, it sounds like the template types should remain |
This is almost working now. Specifically,
So, it looks like if you call a function with an |
Yes, those were put in to avoid ambiguity with promoting to `double` or promoting
to an `autodiff` type.
We could probably also put in definitions that restricted to arithmetic types using an enable_if, but then I don't think that'll help resolution ambiguity.
The only solution I see is to also have signatures for `int64_t`. It seems ridiculous, but I don't see a way around it so that everything will resolve.
… On Nov 1, 2017, at 1:57 PM, bgoodri ***@***.***> wrote:
This is almost working now. Specifically, make test-headers works. There are some test failures under ``test/unit/math/fwdandtest/unit/math/prim/scal/fun` pertaining to promotion. Such as
./stan/math/prim/scal/fun/log_modified_bessel_first_kind.hpp:241:22: error: call of overloaded ‘log1p(const int&)’ is ambiguous
lgam += log1p(v);
~~~~~^~~
In file included from ./stan/math/prim/scal/fun/log1m.hpp:4:0,
from ./stan/math/prim/scal/fun/binary_log_loss.hpp:4,
from ./stan/math/prim/scal.hpp:67,
from test/unit/math/prim/scal/prob/von_mises_log_test.cpp:1:
./stan/math/prim/scal/fun/log1p.hpp:27:19: note: candidate: double stan::math::log1p(double)
inline double log1p(double x) {
^~~~~
./stan/math/prim/scal/fun/log1p.hpp:40:19: note: candidate: double stan::math::log1p(int64_t)
inline double log1p(int64_t x) {
^~~~~
So, it looks like if you call a function with an int it does not know whether to promote it to a double or an int64_t. This can be resolved by deleting the function signature for inline double log1p(int64_t x) (and a bunch of similar specializations of functions that are defined in cmath), but as I recall, we recently put those in to avoid ambiguous behavior.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
You mean signatures for both int and int64_t inputs?
On Wed, Nov 1, 2017 at 3:49 PM, Bob Carpenter <notifications@github.com>
wrote:
… Yes, those were put in to avoid ambiguity with promoting to `double` or
promoting
to an `autodiff` type.
We could probably also put in definitions that restricted to arithmetic
types using an enable_if, but then I don't think that'll help resolution
ambiguity.
The only solution I see is to also have signatures for `int64_t`. It seems
ridiculous, but I don't see a way around it so that everything will resolve.
> On Nov 1, 2017, at 1:57 PM, bgoodri ***@***.***> wrote:
>
> This is almost working now. Specifically, make test-headers works. There
are some test failures under ``test/unit/math/fwdandtest/unit/math/prim/scal/fun`
pertaining to promotion. Such as
>
> ./stan/math/prim/scal/fun/log_modified_bessel_first_kind.hpp:241:22:
error: call of overloaded ‘log1p(const int&)’ is ambiguous
> lgam += log1p(v);
> ~~~~~^~~
> In file included from ./stan/math/prim/scal/fun/log1m.hpp:4:0,
> from ./stan/math/prim/scal/fun/binary_log_loss.hpp:4,
> from ./stan/math/prim/scal.hpp:67,
> from test/unit/math/prim/scal/prob/von_mises_log_test.cpp:1:
> ./stan/math/prim/scal/fun/log1p.hpp:27:19: note: candidate: double
stan::math::log1p(double)
> inline double log1p(double x) {
> ^~~~~
> ./stan/math/prim/scal/fun/log1p.hpp:40:19: note: candidate: double
stan::math::log1p(int64_t)
> inline double log1p(int64_t x) {
> ^~~~~
>
> So, it looks like if you call a function with an int it does not know
whether to promote it to a double or an int64_t. This can be resolved by
deleting the function signature for inline double log1p(int64_t x) (and a
bunch of similar specializations of functions that are defined in cmath),
but as I recall, we recently put those in to avoid ambiguous behavior.
>
> —
> You are receiving this because you were assigned.
> Reply to this email directly, view it on GitHub, or mute the thread.
>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2381 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADOrqn8miqeD0UjAly7AR2svPNe365ijks5syMstgaJpZM4O2Ty4>
.
|
Yes. That'll remove the ambiguity because there's a most specific version that will match. I'm not sure if doing that with an enable_if on arithmetic types would work, but it might.
The problem with ambiguity is not just with `var` (to which the arithmetic types may be promoted), but also with built-ins when they're brought into scope. I think we've limited the latter, but I'm not 100% sure.
… On Nov 1, 2017, at 3:52 PM, bgoodri ***@***.***> wrote:
You mean signatures for both int and int64_t inputs?
|
Would it suffice to roll back the
inline double foo(int64_t x)
signatures to
inline double foo(int x)
? If x is an int64_t, then the compiler should know to promote that to a
double because it can't demote that to an int.
On Wed, Nov 1, 2017 at 4:17 PM, Bob Carpenter <notifications@github.com>
wrote:
… Yes. That'll remove the ambiguity because there's a most specific version
that will match. I'm not sure if doing that with an enable_if on arithmetic
types would work, but it might.
The problem with ambiguity is not just with `var` (to which the arithmetic
types may be promoted), but also with built-ins when they're brought into
scope. I think we've limited the latter, but I'm not 100% sure.
> On Nov 1, 2017, at 3:52 PM, bgoodri ***@***.***> wrote:
>
> You mean signatures for both int and int64_t inputs?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2381 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADOrqgRWVmuNMuW6bmrLN4LpfN7SUCTLks5syNG8gaJpZM4O2Ty4>
.
|
Also, there are issues converting an
I think this has to do with the constructor not knowing how to promote:
|
I think I fixed the |
If `x` is an `int64_t`, then the compiler should know to promote that to a double because it can't demote that to an int. With C++, you can go both ways:
|
I think I fixed the ad_promotable thing.That'd be great. We need to do better doc and edge cases on all of these---@seantalts and @bbbales2 have been cleaning some of them up. |
This is a code-gen issue that I am not sure is still relevant with stanc3 anymore, but feel like it is not. Please reopen and we can move it if you feel otherwise. |
Summary:
Replace all the uses of
int
in the generated code withlong
(see below).Description:
The current restriction to integers is hobbling our ability to do things like negative binomial or even Poisson random variates and to represent some inputs.
Care will have to be taken to make sure 32-bit architectures use 64-bit integers here. I'm not sure where we're at with Windows on this issue. If all our potential platforms do not treat
long
as 64 bits, we'll need to use 64-bit specific types. In C++11, we can useint_64_t
, but we could also use the Boost equivalent, though that's seriously clunky if we use it everywhere compared to the language standard.Additional Information:
All of the integer argument functions in in
stan-dev/math
should also be updated to acceptlong
inputs.Current Version:
v2.16.0
The text was updated successfully, but these errors were encountered: