Skip to content

Conversation

jpf91
Copy link
Contributor

@jpf91 jpf91 commented Jun 12, 2012

Adds a new seed overload as discussed here: http://forum.dlang.org/thread/jr0luj$1ctj$1@digitalmars.com

@dnadlinger
Copy link
Contributor

Maybe add the number of elements required to the error message and documentation?

@dnadlinger
Copy link
Contributor

Also, you might want to add a note to the single integer overload, describing its limitations.

@jpf91
Copy link
Contributor Author

jpf91 commented Jun 12, 2012

The the number of elements required is not fixed. It's 624 for Mt19937 but it seems it can be any value for MersenneTwisterEngine. But I added a basic description to the docs.

@@ -559,8 +559,11 @@ Parameter for the generator.

/**
Seeds a MersenneTwisterEngine object.
Note:
This seed function gives 2^32 starting points. To allow the RNG to be started in any one of its
2^19,937 internal states use the seed overload taking an InputRange.
Copy link
Contributor

Choose a reason for hiding this comment

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

2^19937 (-1) is only the period for Mt19937, not for MersenneTwisterEngine in general, right?

@jpf91
Copy link
Contributor Author

jpf91 commented Jun 13, 2012

Yes, you're right. Removed the 2^19937 note.

*/
void seed(T)(T range) if(isInputRange!T && is(Unqual!(ElementType!T) == UIntType))
{
int j;
Copy link
Member

Choose a reason for hiding this comment

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

j is used for indexing mt and comparing against n (which is a size_t), so it should be size_t, not int.

@jpf91
Copy link
Contributor Author

jpf91 commented Jun 17, 2012

@jmdavis OK, implemented your suggested changes. rndGen is also using the new seed overload now. However, I had to slightly change isSeedable as well. Without that change, it's not possible to use isSeedable for the new seed overload.

@jpf91
Copy link
Contributor Author

jpf91 commented Jun 17, 2012

Yes I should probably do that. Although adding the guarantee about Rng.front was wrong in the first place, but I didn't know that RNGs could be seeded with other types (I implemented isSeedable).

@jpf91
Copy link
Contributor Author

jpf91 commented Jun 17, 2012

map doesn't work in CTFE yet. I think we can just leave that as is, as long as rndGen uses the new seed method and we document the problems with the old seed overload.

@jmdavis
Copy link
Member

jmdavis commented Jun 17, 2012

If there's really no relation between the type of front and the type that's used to seed the range, then there should be no check for whether the type of front and the seed type are the same. I objected, because it was checking that, and you changed it so that it wasn't. Now, it's half-and-half. In one case it checks, and the other doesn't, which seems a bit off to me. So, if you're sure that there's not really any relation between front and the seed type (and the more I think about it, the more it seems like there wouldn't be), then isSeedable shouldn't be checking the type of front at all.

@jpf91
Copy link
Contributor Author

jpf91 commented Jun 17, 2012

OK, I removed the check again. It doesn't really make sense and user code should always check isRandomRNG(Type, frontType) and isSeedable(SeedType).

jmdavis added a commit that referenced this pull request Jun 17, 2012
Add seed(InputRange) overload to MersenneTwisterEngine
@jmdavis jmdavis merged commit d6ffd58 into dlang:master Jun 17, 2012
@jmdavis
Copy link
Member

jmdavis commented Jun 17, 2012

Merged.

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