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 XorShiftRng::extract_seed #119

Closed
wants to merge 1 commit into from

Conversation

tomaka
Copy link

@tomaka tomaka commented Oct 10, 2016

Useful for serializing the PRNG and reload it later.

Copy link
Contributor

@jacwah jacwah left a comment

Choose a reason for hiding this comment

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

I'm not sure seed is the correct term here. As far as I know, the seed is the inital state, not the current one. Maybe extract_state would be a better name.

Generally, I think a way to serialize the state of all RNGs is important. Perhaps we should explore ways to do this in a consistent way across all algorithms. Serde could maybe be useful.

However, the issue with serialization is that it breaks encapsulation. As soon as we merge something like this PR, we can no longer change struct fields without breaking user's code.

@tomaka
Copy link
Author

tomaka commented Jan 21, 2017

Serde could maybe be useful.

Serde would be way overkill and create a semver hell.
This just about adding one function per struct that returns an array.

@alexcrichton alexcrichton added the F-new-int Functionality: new, within Rand label Jun 14, 2017
@dagit
Copy link

dagit commented Jul 13, 2017

I found this PR while reading this thread: https://www.reddit.com/r/roguelikedev/comments/6mk4k4/serializing_the_prng/

There the OP wants to serialize the PRNG state in a game.

I agree that the name should be changed to extract_state. I would love to see a similar function for the other PRNGs too, but for the ones that reseed from external entropy sources that may require additional API changes. Specially to deal with the case where the goal is to get the exact same sequence given some PRNG state (common in simulations).

@dhardy
Copy link
Member

dhardy commented Oct 30, 2017

Serialization should be provided by #189, which I think is the main motivation here.

@tomaka
Copy link
Author

tomaka commented Oct 30, 2017

Do we really need a PR that adds 874 lines of code just to give access to four u32s? :-/

@dhardy
Copy link
Member

dhardy commented Oct 30, 2017

It's not to give us four u32s, it's to allow full serialisation and deserialisation of all types. Your code isn't anywhere near so comprehensive (and can't be along this approach, because most PRNGs don't let you directly set their internal state).

Besides, once serde support for wrapping types and generalisation over constants allows proper handling of large arrays, it should be possible to reduce all of this to #[derive(Serialize, Deserialize)].

@dhardy
Copy link
Member

dhardy commented Feb 8, 2018

We have Serde support now, and it doesn't require 874 lines of code.

@dhardy dhardy closed this Feb 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-new-int Functionality: new, within Rand
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants