-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
randn! cannot fill in float32 arrays. Need a more general randn(T, ...) implementation. #9836
Comments
Cc: @rfourquet |
At least the doc for |
I would guess |
|
The missing
|
Just wanted to check in on this and see if there's and ETA for this. I'd really like to use the official version in some code. Thanks for all the hard work! |
I'm sorry i wanted to tackle this but i have been out of office for the past few weeks and my laptop got broken. If no one beats me at this, i think i will be able to fix this within 2 weeks. |
What would be the right way to implement the |
Converting might give a result with a slightly wrong distribution.
|
Cc: @dmbates @simonbyrne |
We should be able to implement I don't really see the point of implementing |
The current algorithm should work for |
Just checking on this. And on a related issue, are randg and randchi2 expected to make a return? |
@the-moliver For equivalents of http://distributionsjl.readthedocs.org/en/latest/univariate.html#list-of-distributions |
@andreasnoack IIRC, you are the author of the current randn Julia code. Would it be good enough if we just create Float32 tables for the ziggurat? |
@andreasnoack Pinging again. Is it safe to implement this one by converting the existing tables to 32-bit? |
Sorry, I missed the last ping. It's been a while since I worked on the ziggurat tables, but I think we'd have to recompute them. The code to generate the |
Yes Marsaglia's original code was for single precision. |
I just ran into this one as well — I wanted |
@andreasnoack Would you be able to generate the tables for single precision? Or can we just round the existing tables? |
From the discussion so far, I can not tell if enabling a native implementation for
What about |
Is there any progress on this issue? Even if it's only by truncating the Float64 values to Float32. I checked the machine code that llvm generates for double-to-single conversions and it's a single operation. It won't massively increase the processing time for generating Float32 values, and the downstream gains are considerable (e.g., when calculating the exponential of normally-distributed values, I'm already gaining more by doing that in Float32 than the conversion costs). I understand a proper single precision implementation could in principle be faster, but in absence of someone having the time to recalculate the tables for single precision, this is a low-cost solution to have randn support the same types as rand. Someone mentioned above that rounding errors gave problems for rand(Float16), because rounded values could fall outside the support, but surely this can't be the case for the normal distribution as its support is infinite? I just submitted a request in the Distributions package to implement support for Float32 in all distributions, which is relatively easy there but would be even easier if randn had the same interface as rand. |
I don't see how converting could give the wrong distribution for |
Great! I just raised the issue because I was fascinated with the rand conversion bug. |
@rfourquet Would be good to have a converting version for now and an open issue about a native Float32 implementation. I think it only needs new tables, and should be easy to get. |
Perhaps given the various constants and bit manipulations, we may have to replicate the code for the float32 version. |
@stevengj I started implementing the |
looks good to me, except that it will promote the result to Now that I look at it, it's not clear that there is an unambiguous/accepted extension of |
I'm not a huge fan of To put it another way, I would expect |
I would be ok with a different name. Not sure |
Then we might want to consider if complex Gaussian variates are really necessary to support in base. They are rather rare (e.g. I don't think Matlab or Numpy have them) and also very simple to create from real Gaussian variates. |
@simonbyrne, I disagree that "it's a fundamentally different distribution". The definition is exactly the same as for reals, just taken over a different number field. Leaving out the complex case seems like an odd omission, considering that supporting it is trivial, and I've often been annoyed by it. |
I would contend that the fact that it's a different number field makes it a different distribution. That said, I can certainly see the appeal/convenience of having something like this in Base, so I withdraw my previous objection (pending documentation), and would be content to keep the pedantry for Distributions.jl |
Perhaps lending credence to their intuitiveness, I independently implemented the same Regarding concern about overloading |
@fiatflux, a pull request would be welcome. |
…ng#14811) * implement randn!(::Array{Float32}) etc. (fix JuliaLang#9836) * re-enable tests for Char in rand API (disabled in 84349f2)
a = float32(randn(10,10));
rand!(a) works fine
randn!(a) fails
The text was updated successfully, but these errors were encountered: