Skip to content

Conversation

ironhaven
Copy link
Contributor

Currently SmallRng implements SeedableRng but it does not specify a seed_from_u64 method so SmallRng uses the default (pcg32) trait implementation. Because of this the Xorshiro's seed_from_u64 method using Splitmix64 is effectively dead code. Splitmix64 is recommended for seeding Xorshiro by opinionated people so this is definitely a bug.

The documentation for seed_from_u64 says changing the algorithm is a "value-breaking change" but on the other hand the current implementation is a unintended bug so might not be protected from breaking changes.

Should this wait for the next breaking version or be merged immediately?

@dhardy
Copy link
Member

dhardy commented Nov 22, 2021

Splitmix64 is recommended for seeding Xorshiro by opinionated people so this is definitely a bug.

There is discussion on this in #1038. I'm not really convinced it's worth introducing a breaking change for this. In any case, we make zero guarantees about compatibility of SmallRng with other Xoshiro implementations; use the rand_xoshiro crate if you want that.

Should this wait for the next breaking version or be merged immediately?

This is a value-breaking change. We haven't merged breaking changes yet but probably will soon.

@dhardy dhardy added the B-value Breakage: changes output values label Nov 22, 2021
@vks
Copy link
Collaborator

vks commented Nov 22, 2021

I agree it's a bug, because SmallRng::seed_from_u64 was supposed to be using SplitMix (see #1038).

@dhardy
Copy link
Member

dhardy commented Jan 10, 2022

@vks do you think it is reasonable to increase the tolerance of your normal distribution sparkline test here?

@vks
Copy link
Collaborator

vks commented Jan 10, 2022

@dhardy Sure, it seems the test failed only 0.4 % of the tolerance. I should probably calculate the probability of failing with the current tolerance to determine a better threshold.

@vks
Copy link
Collaborator

vks commented Feb 22, 2022

This still needs an update to the changelog and a fix for the tolerance of the test.

@ironhaven
Copy link
Contributor Author

I modified the test by feeding all the numbers in the range 0..100 to the seed_from_u64 and these seeds failed at least one expected error bucket.

1 2 3 8 10 12 14 15 25 38 44 46 48 49 54 63 66 73 81 82 87 93

This means that 22/100 random seeds failed the unit test with error more than 3 standard deviations from expected.
If I allow 4 standard deviations, only a single seed fails 87
I get similar results if I use from_entropy or switch back to the pcg32 implementation.

The other solution would ditch the truly obvious seed of 1 and pick a fair random number like 4 to move on.

@vks
Copy link
Collaborator

vks commented Feb 24, 2022

This means that 22/100 random seeds failed the unit test with error more than 3 standard deviations from expected.

Thanks for investigating this! That's a much higher failure rate than what I would fundamentally expect for the 3 standard deviations threshold. I'll need to take a closer look at the error distribution.

@vks vks mentioned this pull request Mar 29, 2022
@vks
Copy link
Collaborator

vks commented Mar 30, 2022

This only needs updating the changelog for 0.9, documenting the value-breaking change to seed_from_u64.

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

I check all merged PRs when writing changelogs anyway (we always miss some), so I can fix that then).

@dhardy dhardy merged commit 0aca902 into rust-random:master Nov 14, 2022
benjamin-lieser pushed a commit to benjamin-lieser/rand that referenced this pull request Feb 5, 2025
* Forward inner seed_from_u64 implmentation for SmallRng

* increase tolerance of sparkline tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-value Breakage: changes output values
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants