Skip to content
This repository has been archived by the owner on Jul 3, 2022. It is now read-only.

from_seed should not panic #4

Closed
dhardy opened this issue Sep 7, 2018 · 5 comments
Closed

from_seed should not panic #4

dhardy opened this issue Sep 7, 2018 · 5 comments

Comments

@dhardy
Copy link

dhardy commented Sep 7, 2018

assert!(seed != [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],

from_seed does not have error handling, and we decided that it shouldn't panic doc.

Compare with this implementation.

@vks vks closed this as completed in e27cf08 Sep 11, 2018
@vks
Copy link
Owner

vks commented Sep 11, 2018

I fixed and also made sure that the xoshiro generators are not initialized with all zeros in 0.4.

@dhardy
Copy link
Author

dhardy commented Sep 12, 2018

Okay. That's slightly obscure (i.e. might not be obvious to others what happens) but does the job.

@vks
Copy link
Owner

vks commented Sep 12, 2018

I'm not sure what you mean. What would be a less obscure alternative?

@dhardy
Copy link
Author

dhardy commented Sep 12, 2018

Choose a substitute seed directly instead of referring to another function which essentially just picks a different seed for you. Mr Dicker had fun with hex constants here ("odd biases", "bad seed") but it doesn't really matter what.

Potentially the compiler can constantise the function result anyway; I just thought it odd that you're essentially just picking another seed value but not admitting it.

@vks
Copy link
Owner

vks commented Sep 12, 2018

I just thought it odd that you're essentially just picking another seed value but not admitting it.

I'm confused, this is exactly what I'm saying in the docs:

    /// Create a new `Xoshiro256StarStar`.  If `seed` is entirely 0, it will be
    /// mapped to a different seed.

There are two reasons I didn't pick magic constants:

  1. I don't want to come up with a constant that has too many zeros and is potentially not optimal. Using SplitMix64 instead seems like a more conservative option, it's the officially recommended way to seed Vigna's generators.
  2. The size of the seeds is different for the different generators, so the implementation with a constant would be less uniform. (This could be easily fixed by a wrapper function that throws away superfluous bits though.)

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

No branches or pull requests

2 participants