-
-
Notifications
You must be signed in to change notification settings - Fork 486
Port new Range implementation and only have one uniform float distribution #274
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
Changes from 1 commit
741c310
526667e
68bb4aa
4a9c465
616861b
c90fee7
783277a
7b968e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,9 +54,6 @@ pub trait Sample<Support> { | |
| /// Since no state is recorded, each sample is (statistically) | ||
| /// independent of all others, assuming the `Rng` used has this | ||
| /// property. | ||
| // FIXME maybe having this separate is overkill (the only reason is to | ||
| // take &self rather than &mut self)? or maybe this should be the | ||
| // trait called `Sample` and the other should be `DependentSample`. | ||
| #[allow(deprecated)] | ||
| #[deprecated(since="0.5.0", note="use Distribution instead")] | ||
| pub trait IndependentSample<Support>: Sample<Support> { | ||
|
|
@@ -153,8 +150,7 @@ impl<'a, T, D: Distribution<T>> Distribution<T> for &'a D { | |
| /// unassigned/reserved code points. | ||
| /// * `bool`: Generates `false` or `true`, each with probability 0.5. | ||
| /// * Floating point types (`f32` and `f64`): Uniformly distributed in the | ||
| /// open range `(0, 1)`. (The [`Exp1`], and [`StandardNormal`] distributions | ||
| /// produce floating point numbers with alternative ranges or distributions.) | ||
| /// open range `(0, 1)`. | ||
| /// | ||
| /// The following aggregate types also implement the distribution `Uniform` as | ||
| /// long as their component types implement it: | ||
|
|
@@ -347,13 +343,15 @@ fn ziggurat<R: Rng + ?Sized, P, Z>( | |
| // From the remaining 12 most significant bits we use 8 to construct `i`. | ||
| // This saves us generating a whole extra random number, while the added | ||
| // precision of using 64 bits for f64 does not buy us much. | ||
| // Because for some RNG's the least significant bits can be of lower | ||
| // statistical quality, we use bits 3..10 for i. | ||
| let bits = rng.next_u64(); | ||
| let i = bits as usize & 0xff; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are least significant bits — doesn't align with comment above. |
||
|
|
||
| let u = if symmetric { | ||
| // Convert to a value in the range [2,4) and substract to get [-1,1) | ||
| // We can't convert to an open range directly, that would require | ||
| // substracting `3.0 - EPSILON`, which is not representable. | ||
| // It is possible with an extra step, but an open range does not | ||
| // seem neccesary for the ziggurat algorithm anyway. | ||
| (bits >> 12).into_float_with_exponent(1) - 3.0 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't know. But is it not easy to make it open?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It can't be done by changing the constant
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see; you need a separate operation to make this an open range, since 3-ε/2 is not representable. Okay, but it might be nice to have a comment about this. |
||
| } else { | ||
| // Convert to a value in the range [1,2) and substract to get (0,1) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,13 +16,20 @@ use distributions::float::IntoFloat; | |
|
|
||
| /// Sample values uniformly between two bounds. | ||
| /// | ||
| /// This gives a uniform distribution (assuming the RNG used to sample | ||
| /// it is itself uniform and the `RangeImpl` implementation is correct). | ||
| /// `Range::new` and `Range::new_inclusive` will set up a `Range`, which does | ||
| /// some preparations up front to make sampeling values faster. | ||
| /// `Range::sample_single` is optimized for sampeling values once or only a | ||
|
||
| /// limited number of times from a range. | ||
| /// | ||
| /// This can be surprisingly complicated to be both generic and correct, for | ||
| /// example for edge cases like `low = 0u8`, `high = 170u8`, for which a naive | ||
| /// modulo operation would return numbers less than 85 with double the | ||
| /// probability to those greater than 85. | ||
| /// If you need to sample many values from a range, consider using `new` or | ||
| /// `new_inclusive`. This is also the best choice if the range is constant, | ||
| /// because then the preparations can be evaluated at compile-time. | ||
| /// Otherwise `sample_single` may be the best choice. | ||
| /// | ||
| /// Sampeling uniformly from a range can be surprisingly complicated to be both | ||
| /// generic and correct. Consider for example edge cases like `low = 0u8`, | ||
| /// `high = 170u8`, for which a naive modulo operation would return numbers less | ||
| /// than 85 with double the probability to those greater than 85. | ||
| /// | ||
| /// Types should attempt to sample in `[low, high)` for `Range::new(low, high)`, | ||
| /// i.e., excluding `high`, but this may be very difficult. All the primitive | ||
|
|
@@ -141,15 +148,24 @@ pub trait RangeImpl: Sized { | |
| /// Construct self, with inclusive bounds `[low, high]`. | ||
| /// | ||
| /// Usually users should not call this directly but instead use | ||
| /// `Range::new`, which asserts that `low < high` before calling this. | ||
| /// `Range::new_inclusive`, which asserts that `low < high` before calling | ||
| /// this. | ||
| fn new_inclusive(low: Self::X, high: Self::X) -> Self; | ||
|
|
||
| /// Sample a value. | ||
| fn sample<R: Rng + ?Sized>(&self, rng: &mut R) -> Self::X; | ||
|
|
||
| /// Sample a single value uniformly from a range with inclusive lower bound | ||
| /// and exclusive upper bound `[low, high)`. | ||
| /// Panics if `low >= high`. | ||
| /// | ||
| /// Usually users should not call this directly but instead use | ||
| /// `Range::sample_single`, which asserts that `low < high` before calling | ||
| /// this. | ||
| /// | ||
| /// Via this method range implementations can provide a method optimized for | ||
| /// sampeling only a limited number of values from range. The default | ||
| /// implementation just sets up a range with `RangeImpl::new` and samples | ||
| /// from that. | ||
| fn sample_single<R: Rng + ?Sized>(low: Self::X, high: Self::X, rng: &mut R) | ||
| -> Self::X | ||
| { | ||
|
|
@@ -167,7 +183,7 @@ pub struct RangeInt<X> { | |
| } | ||
|
|
||
| macro_rules! range_int_impl { | ||
| ($ty:ty, $signed:ident, $unsigned:ident, | ||
| ($ty:ty, $signed:ty, $unsigned:ident, | ||
| $i_large:ident, $u_large:ident) => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All types should be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The names are also used like |
||
| impl SampleRange for $ty { | ||
| type T = RangeInt<$ty>; | ||
|
|
@@ -195,8 +211,7 @@ macro_rules! range_int_impl { | |
| // end up with a uniform distribution if we map _all_ the random | ||
| // integers that can be generated to this range. We have to map | ||
| // integers from a `zone` that is a multiple of the range. The | ||
| // rest of the integers, that cause a bias, are rejected. The | ||
| // sampled number is `zone % range`. | ||
| // rest of the integers, that cause a bias, are rejected. | ||
| // | ||
| // The problem with `range` is that to cover the full range of | ||
| // the type, it has to store `unsigned_max + 1`, which can't be | ||
|
|
@@ -327,7 +342,7 @@ trait WideningMultiply<RHS = Self> { | |
| } | ||
|
|
||
| macro_rules! wmul_impl { | ||
| ($ty:ty, $wide:ident, $shift:expr) => { | ||
| ($ty:ty, $wide:ty, $shift:expr) => { | ||
| impl WideningMultiply for $ty { | ||
| type Output = ($ty, $ty); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this transmute well-defined? Is it correct on BE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CI is happy... More seriously it should be ok on all architectures as long as we don't transmute into signalling NANs.