Skip to content

Conversation

@newpavlov
Copy link
Member

@newpavlov newpavlov commented Nov 17, 2025

@newpavlov
Copy link
Member Author

newpavlov commented Nov 17, 2025

Value reproducibility is broken only in rand_isaac since previously it was reusing u64 halves during generation of u32, while the new utility functions just generate u64 and truncate it to 32 bits.

We could either tolerate the value breakage and potentially reduced performance, or we should modify the utility functions to restore the old behavior.

@dhardy
Copy link
Member

dhardy commented Nov 17, 2025

Our reproducibility rules allow value-breaking changes in new minor versions. Further, I checked the biggest reverse-dependencies of rand_isaac and none of them actually use Isaac64Rng (outside of documentation). So I think a value-breaking change can be permitted here (but must be documented).

@dhardy
Copy link
Member

dhardy commented Nov 17, 2025

There are a bunch of stylistic changes here. Since I'm not 100% sure on the helper-function approach yet (and to make this cleaner), would you mind separating out the style/quality improvements (jj split -ir or git add -p) and pushing as a new PR?

@newpavlov
Copy link
Member Author

Our reproducibility rules allow value-breaking changes in new minor versions.

The biggest question is whether we want ~2x slower performance for next_u32 implemented for RNGs based on [u64; N]. Adding tracking of halves results in a bit less straightforward code and additional masking operations in next_u64/fill_bytes for erasing the half tracking bit, but it should not be too bad. I will try to implement it a bit later.

would you mind separating out the style/quality improvements and pushing as a new PR?

I would prefer to keep the minor refactoring changes as part of this PR for now. If you decide against the new helper functions, I could create a separate PR later.

@dhardy
Copy link
Member

dhardy commented Nov 17, 2025

~2x slower performance

This sounds like a big issue...

for RNGs based on [u64; N].

But this is just Isaac64Rng? Does anyone actually use it?


I'm considering some changes to rand_core which would require revisions to rust-random/rand_core#24 and this PR.

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