-
-
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
faster rand!(::MersenneTwister, ::Array{T}) for IntTypes and Float16/32 #8958
Conversation
Should we have some more tests for all the new stuff? |
# this is obviously satisfied on the 32 low bits side, and on the high side, the entropy comes | ||
# from bits 33:52 of A128[i] and then from bits 27:32 (which are discarded on the low side) | ||
# this is similar for the 64 high bits of u | ||
A128[i] = mask128(u, T) |
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.
It would be great to get a few more eyes on this.
@rfourquet The refactoring in this PR is perfect and can be merged right away. The other stuff I would like some eyeballs on. In general, what would help is if you can make smaller PRs that are focussed on one particular feature, so that it is easy to go with the merges. |
OK, will try! (I admit I didn't expect that a refactoring without added functionality would be welcome). |
The only test I can think of is running Bigcrush on this to ensure that we're still providing the same level of randomness as before. |
We should at the bare minimum have tests that ensure that all the code paths are executed, and that no exceptions are raised. It is really sad to get a We can potentially also check return type and range. |
I have just tried BigCrush on the scalar and array versions and they still pass all tests but LinearComp which they consistently fails (which is well known for the MT). |
@rfourquet Refactoring that makes the code more readable or clear in any way is always acceptable. Also, we generally do not have enough tests for all the random stuff - the existing tests are in no way sufficient. So, as suggested, we should try to have more tests to exercise all code paths and whatever we can test of RNGs in the test framework. @andreasnoack Does your BigCrush test suite run tests for all the float and int precisions? |
I added some tests which should exercise most code paths for added |
faster rand!(::MersenneTwister, ::Array{T}) for IntTypes and Float16/32
I finally learnt how to use RNGTest, so I could check that BigCrush tests pass for all int precisions. |
I think your methods is fine. Why is not okay that the tests pass? |
As the |
I define I don't think we should modify our Mersenne-Twister to pass the |
Ah thanks! |
It would certainly be interesting to see how to make Float64 pass BigCrush, and what it means for performance and APIs. |
This PR tries to waste less random bits for array filling, and also uses
dsfmt_fill_array_close1_open2
function on non-Float64
arrays.Edit: better timings using this function:
The results are similar for unsigned types (and also a 3x speedup for
rand!(::BitArray)
).(I would be interested if someone finds very different speedups).