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

Add XoshiroSplit type to copy added task RNG state #51271

Closed
wants to merge 2 commits into from

Conversation

nhz2
Copy link
Contributor

@nhz2 nhz2 commented Sep 11, 2023

Fixes #51255 by defining mutable struct XoshiroSplit <: AbstractRNG that is returned by copy(default_rng())

#49110 added an additional state to the task-local RNG. However, before this PR copy(default_rng()) did not include this extra state, causing subtle errors in Test where copy(default_rng()) is assumed to contain the full task-local RNG state.

@nhz2 nhz2 marked this pull request as ready for review September 11, 2023 19:03
@ViralBShah ViralBShah added the randomness Random number generation and the Random stdlib label Sep 11, 2023
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

I think we might as well make this the default behavior for Xoshiro itself, rather than duplicating everything to have an identical XoshiroSplit type. They will be the same size anyways when allocated (6 words), due to alignment.

@nhz2 nhz2 mentioned this pull request Sep 15, 2023
@nhz2
Copy link
Contributor Author

nhz2 commented Sep 15, 2023

I created #51332 to replace this PR

@nhz2 nhz2 closed this Sep 15, 2023
vtjnash pushed a commit that referenced this pull request Sep 15, 2023
This PR adds an optional field to the existing `Xoshiro` struct to be
able to faithfully copy the task-local RNG state.

Fixes #51255
Redo of #51271

Background context: #49110 added an additional state to the task-local
RNG. However, before this PR `copy(default_rng())` did not include this
extra state, causing subtle errors in `Test` where `copy(default_rng())`
is assumed to contain the full task-local RNG state.
NHDaly pushed a commit that referenced this pull request Sep 20, 2023
This PR adds an optional field to the existing `Xoshiro` struct to be
able to faithfully copy the task-local RNG state.

Fixes #51255
Redo of #51271

Background context: #49110 added an additional state to the task-local
RNG. However, before this PR `copy(default_rng())` did not include this
extra state, causing subtle errors in `Test` where `copy(default_rng())`
is assumed to contain the full task-local RNG state.
KristofferC pushed a commit that referenced this pull request Oct 3, 2023
This PR adds an optional field to the existing `Xoshiro` struct to be
able to faithfully copy the task-local RNG state.

Fixes #51255
Redo of #51271

Background context: #49110 added an additional state to the task-local
RNG. However, before this PR `copy(default_rng())` did not include this
extra state, causing subtle errors in `Test` where `copy(default_rng())`
is assumed to contain the full task-local RNG state.

(cherry picked from commit 41b41ab)
nalimilan pushed a commit that referenced this pull request Nov 5, 2023
This PR adds an optional field to the existing `Xoshiro` struct to be
able to faithfully copy the task-local RNG state.

Fixes #51255
Redo of #51271

Background context: #49110 added an additional state to the task-local
RNG. However, before this PR `copy(default_rng())` did not include this
extra state, causing subtle errors in `Test` where `copy(default_rng())`
is assumed to contain the full task-local RNG state.

(cherry picked from commit 41b41ab)
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.

Test: Child tasks have identical RNG streams after a testset in 1.10.0-beta2
3 participants