-
Notifications
You must be signed in to change notification settings - Fork 31
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
Add MPSMatrixRandom
#321
base: main
Are you sure you want to change the base?
Add MPSMatrixRandom
#321
Conversation
The quality of random numbers produced by GPUArrays' RNG is probably lower though, so I would consider just using the one from MPS. |
4fa8052
to
b203523
Compare
A few comments.
|
So you're just reinterpreting Float32-generated random numbers as integers? That doesn't seem valid. Naively I would try to map the generated [0, 1) numbers to the integer's range, but that may still result in a biased output. I'm no RNG expert, but you could try it out and comparing a histogram to
What is the problem? Doesn't it have a way to seed the generator (e.g. https://developer.apple.com/documentation/metalperformanceshaders/mpsmatrixrandomphilox/3242872-initwithdevice?language=objc)?
I would just fall back to the GPUArrays RNG in those cases. CUDA.jl similarly tries to CURAND when possible, and falls back to the native generator in other cases. |
The default rng generator generates random UInt32 values so it's reinterpreting those as whatever Integer value was determined. Just took a look at histograms for all Integer datatypes and they all seem to be equivalent to the CPU versions. They also seem to be much better than the GPUArrays.
I'm not sure how to tie it in with I've marked this as a draft because the seeding test is currently marked as broken and I don't want to forget to fix it. |
4e7abb1
to
e5657f7
Compare
That's not surprising :-) FYI, if you really want to make sure the RNG is good, you can use https://github.com/JuliaRandom/RNGTest.jl. CUDA.jl's native RNG passes those, but it took a while to get there. |
40e640e
to
9f55467
Compare
This is now ready for final review. It should not be squashed when merging because there are a few general improvements that are not directly related to the main focus of this PR. I made sure to have all the commits make sense on their own. |
9f55467
to
16da0c0
Compare
Added some API functions that I missed. I won't squash anymore until it's been reviewed to make things less confusing. |
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.
Looking good!
I've fixed the easy to fix comments. I'll go over the rest over the weekend when I have more time. |
d76ec2e
to
5a995b2
Compare
0b5f039
to
d48c7da
Compare
9b07eaa
to
84911a9
Compare
test/random.jl
Outdated
@@ -11,6 +11,7 @@ const OOPLACE_TUPLES = [[(Metal.rand, rand, T) for T in RAND_TYPES]; | |||
@testset "random" begin | |||
# in-place | |||
@testset "in-place" begin | |||
Metal.seed!(123) |
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.
Why does the global RNG need seeding if you're using an explicit RNG object below?
See #329 |
16a57f4
to
c621e85
Compare
# Seed the default generators to work around value of 0 being | ||
# randomly generated in the size 1 Int8 Array test in 1.11 | ||
Metal.seed!(123) |
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.
Wait, what? Can you elaborate?
6b291e1
to
cef60f4
Compare
That's a GPUCompiler issue, unrelated to this PR. |
fb42b7c
to
22b02ad
Compare
# Even though `append_copy`` seems to work with any size or offset values, the documentation at | ||
# https://developer.apple.com/documentation/metal/mtlblitcommandencoder/1400767-copyfrombuffer?language=objc | ||
# mentions that both must be multiples of 4 bytes in MacOS so error when they are not |
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.
We should rather add a warning or assertion to append_copy
directly.
00ff88b
to
d61f381
Compare
0ce4936
to
8b1f274
Compare
32057c1
to
b43c268
Compare
b43c268
to
c992bac
Compare
Adds https://developer.apple.com/documentation/metalperformanceshaders/mpsmatrixrandom?language=objc and the other classes that start with "MPSMatrixRandom".
Code to integrate with
Random.jl
functions is present but commented out because it seems to be slower than the currentrand!
functionality from GPUArrays.