-
Notifications
You must be signed in to change notification settings - Fork 0
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 #164
Comments
Comment by bgoodri I almost have Stan Math converted in a safe-ish way (On a Mac, you have to put
The remaining occurrences of the "word"
|
Comment by bob-carpenter 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 |
Comment by syclik 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.
|
Comment by bgoodri Okay, I can trigger this outside of Stan by trying to compile
I get:
|
Comment by bgoodri Upon further review, the problem seems to be although inserting
allows the rows and / or columns of a dynamically sized
is hard-coded for |
Comment by bob-carpenter Yes, it sounds like the template types should remain |
Comment by bgoodri This is almost working now. Specifically,
So, it looks like if you call a function with an |
Comment by bob-carpenter Yes, those were put in to avoid ambiguity with promoting to 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
|
Comment by bgoodri You mean signatures for both int and int64_t inputs? On Wed, Nov 1, 2017 at 3:49 PM, Bob Carpenter notifications@github.com
|
Comment by bob-carpenter 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
|
Comment by bgoodri 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 On Wed, Nov 1, 2017 at 4:17 PM, Bob Carpenter notifications@github.com
|
Comment by bgoodri Also, there are issues converting an
I think this has to do with the constructor not knowing how to promote:
|
Comment by bgoodri I think I fixed the |
Comment by bob-carpenter 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:
|
Comment by bob-carpenter 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. |
Issue by bob-carpenter
Monday Aug 14, 2017 at 12:34 GMT
Originally opened as stan-dev/stan#2381
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: