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

revealing current seed as a public read-only property #44

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

a-voronov
Copy link

@a-voronov a-voronov commented Oct 19, 2017

Sometimes it might be helpful to be able to log seed that was used to create generator.
So we can reproduce same random sequence using that seed we know.

For example it can be logged for test suit session whose data is generated randomly.
And if some tests fail, we can fix them and re-run test suit using that seed to verify it on the same 'random' data.

@@ -31,6 +31,9 @@ public protocol Seedable {
/// The seed type.
associatedtype Seed

/// Current seed value.
var seed: Seed { get }
Copy link
Owner

Choose a reason for hiding this comment

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

Requiring Seedable to have a seed property is a breaking change since types outside of this project can conform to this protocol and would have to update their code accordingly.

public typealias Seed = (UInt64, UInt64)

/// The seed value.
public var seed: Seed {
Copy link
Owner

Choose a reason for hiding this comment

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

It would be better if _state was made public and mutable as state instead. See my comment about Seedable.

public typealias Seed = (UInt32, UInt32, UInt32, UInt32)

/// The seed value.
public var seed: Seed {
Copy link
Owner

Choose a reason for hiding this comment

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

x, y, z, and y can probably be made into a public mutable state variable which can be exposed instead.

@@ -38,6 +37,11 @@ public struct XorshiftStar: RandomBytesGenerator, Seedable, SeedableFromRandomGe
UInt64, UInt64, UInt64, UInt64
)

/// The seed value.
public var seed: Seed {
Copy link
Owner

Choose a reason for hiding this comment

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

Similar to with Xoroshiro, should probably change _state to be public and mutable instead.

/// The seed type.
public typealias Seed = [UInt32]

/// The seed value
public var seed: Seed {
Copy link
Owner

@nvzqz nvzqz Oct 21, 2017

Choose a reason for hiding this comment

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

Unless ChaCha(seed: rng.seed).seed == rng.seed, this should be removed since it may not be valid to consider this as a seed. Please add a test to ensure this invariant is met.

@nvzqz
Copy link
Owner

nvzqz commented Oct 21, 2017

Side note: this makes me wish Swift had more of Rust's abilities regarding protocols/traits.

How I'd implement Seedable in Rust:

pub trait Seedable<S> {
    fn from_seed(seed: S) -> Self where Self: Sized;

    fn reseed(&mut self, seed: S);
}

Then there would be no need for SeedableFromSequence since we would have a blanket implementation. This would require specialization, which Swift has out-of-the-box.

impl<S: IntoIterator<Item=u64>> Seedable<S> for ChaCha {
    default fn from_seed(seed: S) -> ChaCha {
        for s in seed.into_iter() {
            /* ... */
        }
    }

    default fn reseed(&mut self, seed: S) {
        for s in seed.into_iter() {
            /* ... */
        }
    }
}

impl<S> Seedable<S> for ChaCha
where
    S: for<'a> IntoIterator<Item=&'a u64>
{
    default fn from_seed(seed: S) -> ChaCha {
        // Dereference the integer
        let iter = seed.map(|&s| s);
        ChaCha::from_seed(iter)
    }

    default fn reseed(&mut self, seed: S) {
        // Dereference the integer
        let iter = seed.map(|&s| s);
        self.reseed(iter)
    }
}

Specialization would allow for:

impl Seedable<u64> for ChaCha {
    fn from_seed(seed: u64) -> ChaCha {
        ChaCha::from_seed(&[seed])
    }

    fn reseed(&mut self, seed: u64) {
        self.reseed(&[seed])
    }
}

Edit: apparently because of problems with specialization, this implementation wouldn't be currently possible in Rust either 😢

https://play.rust-lang.org/?gist=409dba7bc6162833285c8437815bc178&version=nightly

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.

None yet

2 participants