-
-
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
WIP: Use single call to dsfmt_gv_genrand_uint32() for small ranges #5578
Conversation
This is probably not going to be a good place to give up type stability. |
I could not see any time difference in repeated calls to rand(int64(1:10)), rand(int64(1:10), 10000) on the other hand is two times faster than before here. But still worth to make everything type stable if this idea is interesting at all. |
I have changed the procedure a bit and rebased. The overall performance of seperate calls is slightly better than before due to less calls to rand(Uint32) for small ranges: Master:
PR:
Note that these are hard to compare, because of the rejection part of the algorithm. |
|
||
function rand{T<:Integer}(g::RandIntGen{T,Uint64}) | ||
local x::Uint64 | ||
if g.k >> 32 == 0 |
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.
The indentation of this function seems to be a little bit messy. Would you please clean up following julia's four-space convention?
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.
Sorry, some hidden tabs removed.
What's the performance of |
I get
|
Good. +1 for merging this. |
I'll test on 32 bit. |
You don't return a different type. The point is that you only need 32 bits of entropy to return an |
if g.k >> 32 == 0 | ||
x = convert(Uint64,rand(Uint32)) | ||
while x >= g.u | ||
x = convert(Uint64,rand(Uint32)) |
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.
These convert calls are unnecessary since x
is a typed variable.
@mschauer, can you clarify about the type stability? Does this actually introduce any? |
No, I was maybe too generous with rebasing, this does not apply anymore. |
Only a very early version of this was type-unstable. No longer relevant. This seems like a good change. |
I found a bug in the constructors and need a bit more time testing this. A problem is not straight forward to test because the results only can differ if rejections happen, which is unfrequent. I will add some real tests too. |
…y reflectors. Add Float16 and BigFloat to tests and test promotion. Cleaup promotion in LinAlg. Avoid promotion in mutating ! functions. Make Symmetric, Hermitian and QRs immutable. Make thin a keyword argument in svdfact. Remove cond argument in sqrtm.
I will take 5585 into account and submit a new pull request.
|
Closed in favor of 5741 |
This is just an idea regarding #5549,
this way one looses the type stability in RandIntGen, but gains faster number generation, when length(r) <= typemax(Uint32) and only one call to dsfmt_gv_genrand_uint32 to obtain entropy is needed.