Switch the Standard distribution for floats to [0, 1)
#420
Conversation
src/distributions/mod.rs
Outdated
| /// The `Open01` distribution provides an alternative: it generates values in | ||
| /// the open interval (0, 1), with one less random bit. It uses the slightly | ||
| /// faster transmute-based approach for the conversion to a floating point | ||
| /// value. |
There was a problem hiding this comment.
"Slightly faster" isn't really correct; I just benchmarked and found no difference. You could however say (which may be faster on some architectures).
src/distributions/mod.rs
Outdated
| /// ``` | ||
| /// use rand::{thread_rng, Rng}; | ||
| /// `Rng::gen_range(0, 1)` also uses the faster transmute-based method, but | ||
| /// produces values in a half-open interval just like `Standard`. |
src/distributions/mod.rs
Outdated
| /// ``` | ||
| /// As a final alternative, if you require the floating point value to never be | ||
| /// exactly `0.0`: consider using `1.0 - rng.gen()`, which transforms the | ||
| /// half-open `Standard` distribution to the `(0, 1]` interval. |
There was a problem hiding this comment.
I would prefer to assume any users who care know what they want:
If you wish to sample from the
(0, 1]half-open interval consider using1.0 - rng.gen().
Looking at the code, I couldn't actually think of an alternative which samples with this precision and doesn't use an extra op (though of course one could add ε/2 or 1 before conversion to float).
There was a problem hiding this comment.
Looking at the code, I couldn't actually think of an alternative which samples with this precision and doesn't use an extra op (though of course one could add ε/2 or 1 before conversion to float).
Explicit OpenClosed01 does have advantages though:
- the intent is clearer and the code less cluttered,
- the
+1integer addition before conversion is faster than the1.0 - uuser-side workaround
There was a problem hiding this comment.
I agree with you. There is also some benefit to an explicit ClosedOpen01, even if just for local documentation (this value is strictly less than 1).
There was a problem hiding this comment.
Yes, this is also something that hit me when looking at the diff: if not for @pitdicker's perceptivity, subtle bugs would have been introduced in Ziggurat and Gamma where rng.gen() was previously used. When I read rng.gen() it is always unclear to me if the author of the code did so for the convenient notation or whether the code actually relies on the guarantees regarding the 0.0 and 1.0 bounds.
There was a problem hiding this comment.
I suppose that makes for a good argument.
In this case I had it easy, as I also was the one introducing the ambiguity in #274 😄.
pitdicker
force-pushed
the
closed_open_float_conversion
branch
from
53e8ebd to
caaaf0a
Compare
|
Changed the documentation as suggested and added back the benchmarks. I am not against |
|
@pitdicker can you pull my commit? I added I think I'm ready to merge after that. And CI works again! 🎉 |
|
Looks good! |
|
Does it make sense to have some alias |
|
Yes and no: it is more explicit, but redundant and not very useful. I don't think we're likely to move If you really want to see it open a PR, but I'm not sure there's sufficient motive. |
|
I would prefer let r = rng.sample(ClosedOpen01);over let r = rng.sample(Standard); // Can never be 1.0but I can't think of a use case. (It would be different if |
…onversion Switch the `Standard` distribution for floats to [0, 1)
See the discussion in #416. cc @sbarral
This moves the current
Standarddistribution for generating floats toOpen01.Standardinstead samples from the closed-open interval[0, 1)using a multiply-based method.I did not change the Ziggurat and
Uniformimplementations, they continue to use the transmute-based methods. An implementation ofUniformoptimized for the other method would look quite different.I hope just those two distributions,
StandardandOpen01, are good enough for the 0.5 release. I am not convinced adding an explicitClosedOpen01adds much benefit, but it can easily be added in the future. I am also not convinced on the usefullness ofOpenClosed01, but added the suggestion of using1.0 - rng.gen()in the documentation.