randfloat() for Float32/64 plus tests #73
Conversation
randfloat for float32/64
Xorshift import bug removed
Mk/randfloat
test/runtests.jl
Outdated
| end | ||
| end | ||
|
|
||
| include("randfloat.jl") |
There was a problem hiding this comment.
Can you move this line of include to line 3 to be consistent?
There was a problem hiding this comment.
Sure, but that's not the error message, right?
There was a problem hiding this comment.
No it's not the error message. I guess the build error is from the changed Menifest.toml. Maybe a Pkg.update() would fix it?
There was a problem hiding this comment.
Yes, the manifest got updated when I included the StatsBase depency for the histogram test in test/randfloat.jl I'll try to clean that.
src/randfloat.jl
Outdated
| @@ -0,0 +1,73 @@ | |||
| const Xoroshiro128P = Xorshifts.Xoroshiro128Plus() # use as default RNG for randfloat | |||
There was a problem hiding this comment.
IMO, the default behavior of randfloat(::Type) should be consistent as those functions in stdlib - using the global MT generator as default. Although it will be much slower, it can correctly reproduce results if users want to, by manually using Random.seed! to set the global state. What do you think?
There was a problem hiding this comment.
It's a good point, but does RandomNumbers.jl have to be consistent with stdlib? I guess most users (including me) would be happy to just do using RandomNumbers; randfloat(1000) and get something that's faster and higher statistical quality then rand(1000)...?
There was a problem hiding this comment.
Hmmm, it's also a good point. But if we want to do so, we should better override all default behaviors from stdlib, like rand, randn, etc., i.e., we need to change the global RNG. I don't know if it should be done or if it is even possible.
There was a problem hiding this comment.
what about using julia's default in randfloat but defining a randfloatx which uses Xoroshiro128Plus?
There was a problem hiding this comment.
That sounds still a little weird to me. Please just use the global MT generator for now. I will probably refactor those functions to redefine a default global RNG sometime.
|
Sorry, how do you use |
|
I believe the failure in 1.5 will be fixed with sunoru@235e931, but |
|
Ah, yes, there's a subtle thing. You need to use And you are right I need to merge the other PR first... I don't know why I missed that for so long :( |
?-branching also for float32
|
Sorry, that's what I currently have no? import Random: AbstractRNG, GLOBAL_RNG
function randfloat(rng::AbstractRNG,::Type{Float32})
...
randfloat(::Type{T}=Float64) where T = randfloat(GLOBAL_RNG,T) |
|
I think it should be more explicit like function randfloat(rng::Random.AbstractRNG,::Type{Float32}) |
with Random.AbstractRNG
|
There's also a warning in the testing log showing the import is ignored: |
|
Okay sorry, I missed that! Let me remove that line then. |
Mk/randfloat
increase tolerance for test
|
Finally 🎉 |
|
Nice 🎉 |
2 participants
No description provided.