-
Notifications
You must be signed in to change notification settings - Fork 7
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
Fix implementation of rand
#66
Conversation
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #66 +/- ##
==========================================
+ Coverage 95.85% 96.03% +0.18%
==========================================
Files 8 8
Lines 434 429 -5
==========================================
- Hits 416 412 -4
+ Misses 18 17 -1
☔ View full report in Codecov by Sentry. |
@rfourquet Can you confirm that the change from GLOBAL_RNG to default_rng() in this PR is appropriate? |
@rikhuijzer Do you have time and interest in looking over this? |
I would suggest to cleanup the symbol loading a bit with these changes: diff --git a/test/methods.jl b/test/methods.jl
index 83215a2..30516fb 100644
--- a/test/methods.jl
+++ b/test/methods.jl
@@ -1,16 +1,17 @@
module TestUnivariateFiniteMethods
-using Test
-using CategoricalDistributions
-using CategoricalArrays
+import CategoricalDistributions: classes, ERR_NAN_FOUND
import Distributions
-using StableRNGs
import Random
-rng = StableRNG(123)
+
+using CategoricalArrays
+using CategoricalDistributions
+using Random: default_rng
using ScientificTypes
-import Random.default_rng
+using StableRNGs
+using Test
-import CategoricalDistributions: classes, ERR_NAN_FOUND
+rng = StableRNG(123)
v = categorical(collect("asqfasqffqsaaaa"), ordered=true)
V = categorical(collect("asqfasqffqsaaaa"))
@@ -314,7 +315,7 @@ if VERSION >= v"1.7"
@test [rand(d) for i in 1:30] == samples
Random.seed!(123)
- samples = rand(Random.default_rng(), d, 3, 5)
+ samples = rand(default_rng(), d, 3, 5)
Random.seed!(123)
@test samples == rand(d, 3, 5)
end To apply these changes, you can copy them into a file $ git apply mypatch.patch |
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.
Apart from that, LGTM
Closes #65.
This PR also changes the behaviour of
rand(d, args...)
to usedefault_rng
in place ofGLOBAL_RNG
, mimicking changes made to Random in Julia 1.7.edit
No, rather we now address #65 by implementing
Random.Sampler(rng, ::UnivariateDistribution)
andRandom.rand(rng, sampler)
instead directly overloading each variant ofrand([rng, ], ::UnivariateDistribution, args...)
. This has the effect of automatically usingdefault_rng
as the default rng, instead ofGLOBAL_RNG
, because that how Random overloadsBase.rand
.