Fix issue 5849 - Add a dice range type to std.random #1702
Conversation
| && isImplicitlyConvertible!(ElementType!InRange, StorageType)) | ||
| in | ||
| { | ||
| enforce(!proportions.empty, "Proportions cannot be empty"); |
There was a problem hiding this comment.
Preconditions should always assert or throw an Error, not Exception.
|
Quick question: How does your Is there any "initialization overhead"? If not, would it be reasonable to implement |
|
They're both O(n). Both must sum up the entire array. The only difference is that diceRange will binary search the result after summing everything up. So while dice is ~(n + n/2) time, diceRange is ~(n + log n) time (which really doesn't matter that much). Tests: For proportions that are appropriate for dice ( For abnormally high proportions ( So, indeed, it could be appropriate to implement dice in terms of DiceRange from a performance perspective. However, dice and diceRange have incompatible argument orders (diceRange puts proportions first in order to allow UFCS chaining), so it would be slightly more complicated than what you've shown for that reason. In addition, diceRange's output will be different than dice when given integrals. diceRange will match data types internally to avoid the loss of precision that can occur with floating point types. This will drastically change the output because it will then use the integral version of uniform instead of the floating point version. However, it's possible to alleviate that concern by directly instantiating DiceRange. Another problem is that dice does not enforce that its rnd parameter is a uniform RNG. So either we'd have to change the template constraint for dice or DiceRange/diceRange to make them compatible. I don't think anyone is using dice with something other than an RNG, though. So, if we were to change it, it would need to be something like: to ensure that it doesn't change the current behavior (probably would set the StorageType to real for reals, though, so that would make it a bit more complicated). A unittest could be written to enforce that it doesn't change the result of a few example test cases (although I wouldn't keep the unittest around longer than it ensure they return the same results). Of course, if it doesn't matter if it changes the results slightly (as long as they're still correct), then it'd be preferable to just forward to diceRange for simplicity's sake. |
|
@WebDrake , pinging you for your opinion about this. |
|
General thoughts:
auto foo()
{
Random rng; // will be deleted on exit of this scope
return diceRange([0.5, 0.5], rng); // when we try and use this, it'll fall over
}This is an unsolvable problem until we implement the long-desired RNGs-as-reference-types solution.
dice(rng, 5, 5, 10, 5); // proportions as individual parameters
dice(rng, [5, 5, 10, 5]); // proportions as elements of an input rangeA dice range should be similarly flexible. N.B. for Bottom line: ANY functionality like this that wraps an underlying RNG instance is going to be crippled until we do Nice side-benefit: the design principle at work here is going to be very useful in my Dgraph library. Quite a few algorithms for growing complex networks revolve around selecting nodes with a probability proportional to their degree. So thank you, Zshazz! :-) |
|
Thanks for your comments.
Anyway, I'm glad there will be some benefit to this. I know I'll be using it for a few things on occasion (I've used |
Using "RandomDie" instead of DiceRange makes it more consistent with the names of other ranges wrapping RNG (such as RandomCover and RandomSample) in std.random.
InputTypes, a list of all valid input types, was used in two different tests but was not shared. The refactoring moved it up a scope and allowed it to be shared instead of duplicated. A test to check that dice rolls didn't always return the same was added. This was something that was missing previously and might be useful to assure the validity of future changes.
This change makes randomDie consistent with other random ranges w.r.t.
storing the rng as a value instead of as a pointer. This will prevent
issues arising in code such as this:
auto foo()
{
Random rng; // will be deleted on exit of this scope
return randomDie([0.5, 0.5], rng); // using later will error
}
Additionally, the documentation has been updated to display a warning
similar to the warnings given to randomCover and randomSample to alert
users of the common pitfall of using value-based rngs.
Finally, a test assuring that using the default rng is safe from this
pitfall has been added.
Changing the order of the arguments makes randomDice more consistent
with dice and allows randomDice to have a typesafe variadic form,
such as allowing for:
auto list = ["somewhat common", "very common", "rare"];
auto choices = randomDie(25, 50, 1).map!(e => list[e])
.take(1000).array();
The documentation has been updated to reflect the changes and the new
feature and a unittest has been written to confirm its functionality.
| static assert(0); | ||
| } | ||
|
|
||
| e = accumulator; |
There was a problem hiding this comment.
This fails when storage type is immutable or const real (which can happen when an immutable or const real range is provided with the way that RandomDieStorageFor works). Will fix & improve tests in next commit push.
I might actually change StorageType to be immutable by default and just use assumeUnique when actually stored (since there is no reason to keep the cumulative sum array mutable after it's made).
Previously, a bug concerning immutable reals slipped through the
cracks. The unittests now test against const and immutable types to
cover those kinds of cases. Example of code that would fail to compile:
immutable real[] props = [0.0, 1.0];
auto dicer = randomDie(props);
To make the unittest pass, a simple fix was also implemented in the
way that the StorageType is chosen by default.
Yup, you can see exactly how to do this in RandomCover and RandomSample. It's an annoyance but necessary with the current
You don't need to but it's a good idea as the issue report is used to generate the change list for new releases.
I hadn't particularly thought it through along those lines, so if you want to change it back to allow for UFCS chaining etc., I'd be happy with that. |
This makes it so that randomDie explicitly supports Ranges that have qualified ElementTypes (e.g. immutable, const, shared). In addition it changes the way that RandomDie stores the cumulative array internally to be immutable (since it will never change after being made). This change would make it so that qualified StorageTypes are equivalent to their unqualified forms, so to make that clear only unqualified StorageTypes are accepted. The documentation and unittests for RandomDie have been updated to reflect this change.
This removes the variadic argument feature and restores the original order of the arguments to improve the usage of randomDie as a range. This allows for consistent UFCS usage of randomDie.
|
A note here -- this range is effectively identical to what in Boost/C++11 is called |
This is adapted from Chris Cain's RandomDie submission to Phobos in dlang/phobos#1702 The alternative name is chosen by analogy to Boost.Random and the C++11 standard.
This is adapted from Chris Cain's RandomDie submission to Phobos in dlang/phobos#1702 The alternative name is chosen by analogy to Boost.Random and the C++11 standard.
|
ping |
|
Looks to me like all comments has been addressed. @WebDrake I will trust your judgement, is it ready to merge? |
|
I have mixed feelings. To be honest I think that I feel uncomfortable about the idea of introducing new RNG-wrapping ranges to Let me remind myself of the details and compare to what's currently in |
This shouldn't bother you. As long as it is consistent with existing |
|
Should add that I'm also happy to do that work myself -- I'll see if I can give it some time at the weekend. |
OK, but it will have to be carefully documented with warnings about its points of failure. As I said, I'll see if I can take a look at the weekend and propose some revisions. |
|
ping @WebDrake |
|
I'm really sorry for the delay, it's been a very busy period. Will try and get onto this soon. |
|
ping |
Fix for:
https://d.puremagic.com/issues/show_bug.cgi?id=5849
This pull provides a range interface to simulate dice rolls much like
dice. The difference is that thisdiceRangecan do some preprocessing to the proportions vector to make it much faster to do repeated rolls with the same die. The algorithm used is simply a cumulative sum array with binary search, so the implementation is straightforward.A benefit from this type of implementation is that it is essentially equivalent to the current dice implementation when
doubles are used. One of the test cases in the unittest covers that. This should allow a user to nearly drop-in replace dice calls and get precisely the same results if needed.Some internals are exposed in the form of template parameters which can allow a user to optimize the dice rolls for their particular data.
See also (for a use case):
http://forum.dlang.org/thread/prkthuzsmbppjltvohdh@forum.dlang.org?page=5