Skip to content
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 typo in Float32 random number generation #57338

Merged
merged 4 commits into from
Feb 10, 2025
Merged

Conversation

KristofferC
Copy link
Member

introduced in #56144

@KristofferC KristofferC added the backport 1.12 Change should be backported to release-1.12 label Feb 10, 2025
@KristofferC KristofferC requested a review from giordano February 10, 2025 12:33
@KristofferC KristofferC changed the title fix typo in Float32 number generation fix typo in Float32 random number generation Feb 10, 2025
@giordano
Copy link
Contributor

#57337

Comment on lines 1253 to 1255
@testset "Float32 RNG typo" begin
@test length(unique(rand(Float32, 14))) > 10
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extend to other types as well? This won't catch similar issues otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, there are two completely different codepaths for generating random numbers, one for less than 32 numbers and another one for larger vectors. It'd be good to exercise both, especially since the larger one involves llvmcalls which are harder to debug.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test here is pretty much just to check that the concrete issue is fixed. I don't think there is much value in extending the tests more than that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it'd have caught #55997 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why wasn't the test added there? ;). Anyway, general RNG test improvements are nice but doesn't have to be mixed up with this specific bugfix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why wasn't the test added there?

I was thinking of count iszero tests, but those are brittle at best, especially for Float16 (which generates 2048 unique numbers, so the probability of picking exactly 0 is non-negligible) and especially for small vectors (which was the broken case), I could hit test failures fairly easily.

@@ -1250,6 +1250,12 @@ end
@test length(xs) == 3
end

@testset "Float32 RNG typo" begin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@testset "Float32 RNG typo" begin
@testset "Randomness of RNGs" begin

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that oversells it a bit maybe heh.

stdlib/Random/test/runtests.jl Outdated Show resolved Hide resolved
@giordano giordano added the randomness Random number generation and the Random stdlib label Feb 10, 2025
@giordano giordano merged commit dd13878 into master Feb 10, 2025
7 checks passed
@giordano giordano deleted the kc/fix_float32_rand branch February 10, 2025 20:51
KristofferC added a commit that referenced this pull request Feb 11, 2025
introduced in #56144

---------

Co-authored-by: Mosè Giordano <[email protected]>
(cherry picked from commit dd13878)
@KristofferC KristofferC mentioned this pull request Feb 11, 2025
32 tasks
KristofferC added a commit that referenced this pull request Feb 13, 2025
Backported PRs:
- [x] #57142 <!-- Add reference to time_ns in time -->
- [x] #57241 <!-- Handle `waitpid` race condition when `SIGCHLD` is set
to `SIG_IGN` -->
- [x] #57249 <!-- restore non-freebsd-unix fix for profiling -->
- [x] #57211 <!-- Ensure read/readavailable for BufferStream are
threadsafe -->
- [x] #57262 <!-- edit NEWS for v1.12 -->
- [x] #57226 <!-- cfunction: reimplement, as originally planned, for
reliable performance -->
- [x] #57253 <!-- bpart: Fully switch to partitioned semantics -->
- [x] #57273 <!-- fix "Right arrow autocompletes at line end"
implementation -->
- [x] #57280 <!-- dep: Update JuliaSyntax -->
- [x] #57229 <!-- staticdata: Close data race after backedge insertion
-->
- [x] #57298 <!-- Updating binding version to fix MMTk CI -->
- [x] #57248 <!-- improve concurrency safety for `Compiler.finish!` -->
- [x] #57312 <!-- Profile.print: de-focus sleeping frames as gray -->
- [x] #57289 <!-- Make `OncePerX` subtype `Function` -->
- [x] #57310 <!-- Make ptls allocations at least 128 byte aligned -->
- [x] #57311 <!-- Add a warning for auto-import of types -->
- [x] #57338 <!-- fix typo in Float32 random number generation -->
- [x] #57293 <!-- Fix getfield_tfunc when order or boundscheck is Vararg
-->
- [x] #57349 <!-- docs: fix-up world-age handling for META access -->
- [x] #57344 <!-- Add missing type asserts when taking the queue out of
the task struct -->
- [x] #57348 <!-- 🤖 [master] Bump the SparseArrays stdlib from 212981b
to 72c7cac -->
- [x] #55040 <!-- Allow macrocall as function sig -->
- [x] #57299 <!-- Add missing latestworld after parameterized type alias
-->
@KristofferC KristofferC removed the backport 1.12 Change should be backported to release-1.12 label Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
randomness Random number generation and the Random stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rand(Float32, n) generates two values of the same number over and over when n is < 16
4 participants