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 trait ReproducibleRng #1570

Closed
wants to merge 2 commits into from
Closed

Add trait ReproducibleRng #1570

wants to merge 2 commits into from

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Jan 29, 2025

Summary

pub trait ReproducibleRng: SeedableRng {}

Motivation

Clarify that SeedableRng does not imply reproducibility and what RNGs do have this property.

Details

I'm somewhat loathe to do this since in part, reproducibility is a special property that StdRng and SmallRng opt out of. On the other hand, this is a property not implied by the API or the semver model.

So does this form of formal documentation make sense?

Obviously this PR is incomplete: the ChaCha and Pcg RNGs in this repo should implement the trait, and also StepRng and most of the rngs repo.

@newpavlov
Copy link
Member

Personally, I think we do not need this trait and that it's sufficient to explain the reproducibility properties in docs.

@dhardy
Copy link
Member Author

dhardy commented Jan 29, 2025

Personally I don't either. I'm also not wildly keen on adding it.

It does help clarify what SeedableRng does not offer, but simply adding docs there may be enough.

@benjamin-lieser
Copy link
Member

My first thought was also that I do not like this. But on second thought the cost is very low, because it increases the API only by a very small amount and it has a good chance that everyone who cares about reproducibility will do the right thing without reading the docs too closely.

So I am in favor, but no strong opinion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants