Port new Range implementation and only have one uniform float distribution #274

pitdicker · 2018-02-28T07:38:13Z

Finally finished the first part of this.

The biggest change is the porting the new Range implementation. Integers now use a much faster implementation based on a widening multiply instead of modulus.

I have added a new private trait IntoFloat with an into_float_with_exponent method as a building block to convert from integers to floats.

The Open01 and Closed01 distributions are removed, and Uniform for floats will now return values in the open range (0, 1).
IntoFloat is also used in an optimised implementation in Range, and in ziggurat.

Will post benchmarks later today.

dhardy

Doesn't this leave next_f32/64 hanging around uselessly? You might as well remove those, and I'll remove that part of my PR.

I still need to take a closer look, but it's great to finally have this bit land!

@tspiteri are you interested in reviewing this?

dhardy · 2018-02-28T09:11:09Z

src/distributions/float.rs

+    /// 52 for `f64`.
+    /// The resulting value will fall in a range that depends on the exponent.
+    /// As an example the range with exponent 0 will be
+    /// [2<sup>0</sup>..2<sup>1</sup>-1), which is [1..2).


2.pow(1) - 1 = 1, not 2 — or am I not reading it right?

Oops, -1 is not supposed to be there

dhardy · 2018-02-28T09:18:12Z

src/distributions/mod.rs

+            // TODO: This range is not open, is that a poblem?
+            (bits >> 12).into_float_with_exponent(1) - 3.0
+        } else {
+            // Convert to a value in the range [0,1) and substract to get (0,1)


range [1, 2) ?

dhardy · 2018-02-28T09:19:00Z

src/distributions/mod.rs

+        let u = if symmetric {
+            // Convert to a value in the range [2,4) and substract to get [-1,1)
+            // TODO: This range is not open, is that a poblem?
+            (bits >> 12).into_float_with_exponent(1) - 3.0


Don't know. But is it not easy to make it open?

It can't be done by changing the constant 3.0, because 3.0-EPSILON is not representable. So we would need one extra addition. But I looked a bit more careful at the function(s) and can't imagine it to be a problem.

pitdicker · 2018-02-28T16:52:25Z

Benchmarks, taken with the following commands using cargo benchcmp:

git checkout master
cargo bench --features=i128_support > master
git checkout port_range
cargo bench --features=i128_support > port_range
cargo benchcmp control variable --threshold 1

 name                             master ns/iter        port_range ns/iter    diff ns/iter   diff %  speedup 
 distr_exp                        6,193 (1291 MB/s)     5,925 (1350 MB/s)             -268   -4.33%   x 1.05 
 distr_gamma_large_shape          19,570 (408 MB/s)     17,865 (447 MB/s)           -1,705   -8.71%   x 1.10 
 distr_gamma_small_shape          79,406 (100 MB/s)     77,951 (102 MB/s)           -1,455   -1.83%   x 1.02 
 distr_log_normal                 25,938 (308 MB/s)     24,019 (333 MB/s)           -1,919   -7.40%   x 1.08 
 distr_normal                     6,877 (1163 MB/s)     6,470 (1236 MB/s)             -407   -5.92%   x 1.06 
 distr_range_i128                 143,214 (111 MB/s)    8,529 (1875 MB/s)         -134,685  -94.04%  x 16.79 
 distr_range_i16                  4,441 (450 MB/s)      2,512 (796 MB/s)            -1,929  -43.44%   x 1.77 
 distr_range_i32                  4,968 (805 MB/s)      3,062 (1306 MB/s)           -1,906  -38.37%   x 1.62 
 distr_range_i64                  9,623 (831 MB/s)      2,910 (2749 MB/s)           -6,713  -69.76%   x 3.31 
 distr_range_i8                   5,147 (194 MB/s)      2,510 (398 MB/s)            -2,637  -51.23%   x 2.05 
 gen_range_i128                   144,053 (111 MB/s)    15,796 (1012 MB/s)        -128,257  -89.03%   x 9.12 
 gen_range_i16                    4,202 (475 MB/s)      2,530 (790 MB/s)            -1,672  -39.79%   x 1.66 
 gen_range_i32                    4,877 (820 MB/s)      3,069 (1303 MB/s)           -1,808  -37.07%   x 1.59 
 gen_range_i64                    9,691 (825 MB/s)      6,946 (1151 MB/s)           -2,745  -28.33%   x 1.40 
 gen_range_i8                     5,102 (196 MB/s)      2,530 (395 MB/s)            -2,572  -50.41%   x 2.02 
 misc_sample_indices_100_of_1k    1,784                 715                         -1,069  -59.92%   x 2.50 
 misc_sample_indices_10_of_1k     706                   585                           -121  -17.14%   x 1.21 
 misc_sample_indices_50_of_1k     979                   429                           -550  -56.18%   x 2.28 
 misc_sample_iter_10_of_100       1,601                 954                           -647  -40.41%   x 1.68 
 misc_sample_slice_10_of_100      229                   150                            -79  -34.50%   x 1.53 
 misc_sample_slice_ref_10_of_100  225                   150                            -75  -33.33%   x 1.50 
 misc_shuffle_100                 1,529                 843                           -686  -44.87%   x 1.81

Most of the distributions are a little faster with the optimised float conversion, and the others thanks to the new range code.

pitdicker · 2018-02-28T16:54:04Z

Doesn't this leave next_f32/64 hanging around uselessly? You might as well remove those, and I'll remove that part of my PR.

👍

pitdicker · 2018-02-28T17:03:40Z

Travis has some problem with incremental compilation, but closing and reopening the PR does not make it retry. No problem

pitdicker · 2018-02-28T18:08:15Z

Added two tiny commits. One to use Range to generate char's, and one to use a sign check for bool's.
This improves the benchmark like this:

distr_uniform_bool       4,357 (229 MB/s)     4,366 (229 MB/s)                9    0.21%   x 1.00
distr_uniform_codepoint  9,004 (444 MB/s)     2,801 (1428 MB/s)          -6,203  -68.89%   x 3.21

I vaguely remember that generating bools also became faster, but apparently not...

dhardy · 2018-02-28T18:26:45Z

One thing I think you may have missed: distributions::Uniform (in mod.rs) describes its implementations; this probably needs updating.

I don't think bool got faster, just that it didn't get slower when using the most significant bit instead?

…ution

The previous code would reject about 50% of the generated numbers, because chars are always lower than `0x11_0000`, half of the masked `0x1f_ffff`.

pitdicker · 2018-02-28T18:46:43Z

Rebased after the merge of #273.

I don't think bool got faster, just that it didn't get slower when using the most significant bit instead?

Comparing against zero should be just a bit faster than doing a mask first, but I remembered wrong.

dhardy

Wow; that's a lot to review! I see a lot of the range code is unchanged from my master branch but that you simplified float sampling; I guess this makes sense with reduced precision.

dhardy · 2018-03-02T10:56:58Z

src/distributions/float.rs

+            #[inline(always)]
+            fn into_float_with_exponent(self, exponent: i32) -> $ty {
+                // The exponent is encoded using an offset-binary representation,
+                // with the zero offset being 127


127 is only correct for f32 I think? Maybe reduce this doc.

dhardy · 2018-03-02T11:15:01Z

src/distributions/float.rs

+
+                let value = rng.$next_u();
+                let fraction = value >> (float_size - $fraction_bits);
+                fraction.into_float_with_exponent(0) - (1.0 - EPSILON / 2.0)


If 1+ε is the smallest representable number above 1, then 1-ε/2 is representable; ok. This is the same adjustment as the Open01 removed here but in a single number. Looks fine and functionally identical.

dhardy · 2018-03-02T11:20:01Z

src/distributions/float.rs

+            fn into_float_with_exponent(self, exponent: i32) -> $ty {
+                // The exponent is encoded using an offset-binary representation,
+                // with the zero offset being 127
+                let exponent_bits = (($exponent_bias + exponent) as $uty) << $fraction_bits;


Equivalent to removed UPPER_MASK when exponent == 0; ok.

dhardy · 2018-03-02T11:40:13Z

src/distributions/mod.rs

    }

-    impl<Sup: SampleRange> Sample<Sup> for Range<Sup> {
+    impl<Sup: SampleRange + RangeImpl<X = Sup>> Sample<Sup> for Range<Sup> {


I think this is wrong and won't actually generate any implementations — I think it should be impl<T: RangeImpl> Sample<T::X> for Range<T>. Also below.

dhardy · 2018-03-02T11:41:39Z

src/distributions/mod.rs

-///   [`StandardNormal`] distributions produce floating point numbers with
-///   alternative ranges or distributions.)
+///   open range `(0, 1)`. (The [`Exp1`], and [`StandardNormal`] distributions
+///   produce floating point numbers with alternative ranges or distributions.)


This last sentence is off-topic now; I think just remove it.

dhardy · 2018-03-02T16:08:46Z

src/distributions/range.rs

+
+macro_rules! range_int_impl {
+    ($ty:ty, $signed:ident, $unsigned:ident,
+     $i_large:ident, $u_large:ident) => {


All types should be ty, not ident.

The names are also used like ::core::$u_large::MAX. ident works for both, but ty only for types.

dhardy · 2018-03-02T16:15:24Z

src/distributions/range.rs

+}
+
+macro_rules! wmul_impl {
+    ($ty:ty, $wide:ident, $shift:expr) => {


dhardy · 2018-03-02T16:32:41Z

src/distributions/range.rs

+            fn sample<R: Rng + ?Sized>(&self, rng: &mut R) -> Self::X {
+                // Generate a value in the range [1, 2)
+                let value1_2 = (rng.$next_u() >> $bits_to_discard)
+                               .into_float_with_exponent(0);


So this range is half-open, unlike Uniform. Slightly odd, but okay I guess.

Yes, I was not perfectly happy about the difference. But I haven't yet thought through all the problematic rounding cases. We should not make any guarantees about whether the ranges are open or closed yet.

dhardy · 2018-03-02T16:35:50Z

src/distributions/range.rs

+    use distributions::range::{Range, RangeImpl, RangeFloat, SampleRange};
+
+    #[test]
+    fn test_fn_range() {


This test is pretty strange: why two separate loops? why not cache the ranges? do we also test the single-sample variant, and for various types?

Oops, I copied the tests but didn't look closely. You are right, the first three test do not make much sense or are duplicates.

dhardy · 2018-03-02T16:37:43Z

src/distributions/range.rs

+    #[should_panic]
+    fn test_fn_range_panic_usize() {
+        let mut r = ::test::rng(816);
+        Range::new(5, 2).sample(&mut r);


Doesn't use usize like name implies. This and the previous fn are redundant. Maybe add one unit-test to test all supported int types?

pitdicker · 2018-03-03T11:16:58Z

Thanks for the careful read!

dhardy · 2018-03-03T12:15:08Z

src/distributions/range.rs

-/// 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


No 'e' in 'sampling' (also below), but 👍

pitdicker force-pushed the port_range branch from 9c8c4ad to d865501 Compare February 28, 2018 07:52

dhardy reviewed Feb 28, 2018

View reviewed changes

pitdicker force-pushed the port_range branch from d865501 to a4cad05 Compare February 28, 2018 16:26

pitdicker closed this Feb 28, 2018

pitdicker reopened this Feb 28, 2018

pitdicker force-pushed the port_range branch from 0f3d818 to 1b916d5 Compare February 28, 2018 18:42

pitdicker added 4 commits February 28, 2018 19:43

Port new Range implementation and only have one uniform float distrib…

741c310

…ution

Use Range in Distribution<char>

526667e

The previous code would reject about 50% of the generated numbers, because chars are always lower than `0x11_0000`, half of the masked `0x1f_ffff`.

Generate bool using sign test

68bb4aa

Update Uniform documentation

4a9c465

pitdicker force-pushed the port_range branch from 1b916d5 to 4a9c465 Compare February 28, 2018 18:43

dhardy mentioned this pull request Mar 1, 2018

Tracker: planned changes for 0.5 #232

Closed

33 tasks

dhardy approved these changes Mar 2, 2018

View reviewed changes

pitdicker added 3 commits March 3, 2018 13:03

Documentation fixes

616861b

Correct Range compatability code

c90fee7

Remove unnecessary range tests

783277a

dhardy approved these changes Mar 3, 2018

View reviewed changes

Fix spelling

7b968e8

dhardy merged commit 7b968e8 into rust-random:master Mar 3, 2018

This was referenced Mar 3, 2018

impl SampleRange for user types #146

Closed

Range is not inclusive. Not possible to generate Type::MAX values #188

Closed

pitdicker deleted the port_range branch March 22, 2018 10:14

pitdicker mentioned this pull request Apr 12, 2018

Implement From<std::ops::Range> for Range #382

Merged

pitdicker mentioned this pull request May 3, 2018

Switch the Standard distribution for floats to [0, 1) #420

Merged

Uh oh!

Port new Range implementation and only have one uniform float distribution #274

Port new Range implementation and only have one uniform float distribution #274

Uh oh!

Conversation

pitdicker commented Feb 28, 2018

Uh oh!

dhardy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitdicker commented Feb 28, 2018

Uh oh!

pitdicker commented Feb 28, 2018

Uh oh!

pitdicker commented Feb 28, 2018

Uh oh!

pitdicker commented Feb 28, 2018

Uh oh!

dhardy commented Feb 28, 2018

Uh oh!

pitdicker commented Feb 28, 2018

Uh oh!

dhardy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitdicker commented Mar 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants