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

vks · 2018-04-09T18:29:35Z

This allows the following syntax:

let between = Range::from(10..10000);

pitdicker · 2018-04-09T20:23:05Z

Very nice!

~~Would it even be possible implement the Standard distribution for core::ops::Range, so that you can write rng.gen(0..10)? Than we may not even need Rng::gen_range anymore.~~
Edit: I'll never learn...

pitdicker · 2018-04-10T10:47:24Z

src/distributions/range.rs

@@ -314,6 +314,19 @@ macro_rules! range_int_impl {
                }
            }
        }
+
+        impl From<::core::ops::Range<$ty>> for RangeInt<$ty> {


Can you also implement From<::core::ops::Range<$ty>> for RangeFloat?

I'm not sure this is a good idea, since we don't have an implementation for closed ranges. On the other hand, this also affects Range::new and Range::new_inclusive, which are currently identical for floats.

And here it's annoying that the a..b syntax is half-open — if it were closed it wouldn't be confusing using the syntax for sampling both float and int ranges. As it is though I don't know.

pitdicker · 2018-04-10T10:52:39Z

src/distributions/range.rs

+            }
+        }
+
+        impl From<::core::ops::Range<$ty>> for Range<$ty>


Maybe make this a generic implementation for all RangImpls?

impl<X> From<::core::ops::Range<X>> for Range<X> where X: SampleRange, <X as SampleRange>::Impl: From<::core::ops::Range<X>> { fn from(r: ::core::ops::Range<X>) -> Range<X> { Range { inner: <X as SampleRange>::Impl::from(r) } } }

Thinking about it a bit more, a simple generic implementation is all that is needed, no implementation on RangeImpl is necessary:

impl<X: SampleRange> From<::core::ops::Range<X>> for Range<X> { fn from(r: ::core::ops::Range<X>) -> Range<X> { Range::new(r.start, r.end) } }

pitdicker · 2018-04-10T11:48:17Z

src/distributions/range.rs

-                // Doing multiply before addition allows some architectures to
-                // use a single instruction.
-                value1_2 * self.scale + self.offset
+                value1_2.mul_add(self.scale, self.offset)


mul_add is much slower than a separate multiply and addition, because it tries to minimize the rounding error. LLVM can turn the two instructions into one for the platforms that support it.

mul_add is much slower

Not according to the documentation.

Yes, I read that and tried it before. Can you benchmark it otherwise, because I believe that documentation is wrong.

Before:

test distr_range_f32 ... bench: 1,517 ns/iter (+/- 31) = 2636 MB/s test distr_range_f64 ... bench: 3,854 ns/iter (+/- 80) = 2075 MB/s

After:

test distr_range_f32 ... bench: 2,329 ns/iter (+/- 12) = 1717 MB/s test distr_range_f64 ... bench: 3,875 ns/iter (+/- 83) = 2064 MB/s

So it seems only slower for f32. But certainly not faster.

Looking at the assembly, mul_add generates vfmadd213ss xmm0, xmm1, xmm2 while a * b + c generates vmulss xmm0, xmm0, xmm1; vaddss xmm0, xmm0, xmm2.

Another problem: it does not work with no_std (CI fails).
B.t.w. if you rebase on master the CI should work again.

Actually, when using target-cpu=native, mul_add is faster for f32:

test distr_range_f32 ... bench: 1,163 ns/iter (+/- 17) = 3439 MB/s test distr_range_f64 ... bench: 3,852 ns/iter (+/- 6) = 2076 MB/s

vks · 2018-04-10T11:56:07Z

Would it even be possible implement the Standard distribution for core::ops::Range, so that you can write rng.gen(0..10)?

How would that work? Standard is not parametrized and Distribution<Range<f64>> can only generate Range<f64>.

pitdicker · 2018-04-10T11:59:16Z

I edited the comment, I'm afraid I'll never learn...

It doesn't work with `no_std` and it is sometimes slower. This was documented in a comment.

vks · 2018-04-11T14:02:04Z

I removed mul_add and updated the comment. I think we should not use it, because it is not available without std and we can't fall back to a * b + c because it is not equivalent.

pitdicker · 2018-04-11T14:27:50Z

What did you think of my comment to not implement range for the RangeImpls, but only for Range? That would also make it easily available for user types.

vks · 2018-04-11T17:01:58Z

Sorry, I missed that comment. I like it!

pitdicker

@vks Sorry for making things difficult, but two more comments

pitdicker · 2018-04-12T06:02:44Z

src/distributions/range.rs

@@ -454,11 +466,18 @@ macro_rules! range_float_impl {
                // Generate a value in the range [1, 2)
                let value1_2 = (rng.$next_u() >> $bits_to_discard)
                               .into_float_with_exponent(0);
-                // Doing multiply before addition allows some architectures to
-                // use a single instruction.


Before #274 we would do r.low + r.range * rng.gen::<$ty>(). I think it is nice to preserve this comment that explains why we choose a different order.

Could you please be more specific? I'm not sure the previous comment was correct. Without fast-math it is not correct if the compiler uses FMA.

Ah, there is an instruction that does a true FMA, with the benefit of extra precision, and a simple instruction that produuces exactly the same results as multiply and addition. At least on ARM.

Thanks, I'll add a comment!

pitdicker · 2018-04-12T06:03:55Z

src/distributions/range.rs

                value1_2 * self.scale + self.offset
            }
        }
+
+        impl From<::core::ops::Range<$ty>> for RangeFloat<$ty> {


I don't see a point in keeping these when there also is the more generic implementation?

Removing this would break RangeFloat::from(range), wouldn't it? I would rather keep it, if RangeFloat stays public.

RangeFloat is supposed to be an implementation detail, only there to make it easier to implement Range for user types. No one is supposed to use it directly.

Then it should be private, right? If we make it private (or hide the docs) I would agree that the other impls should be removed.

Edit: Can't be private, because we are leaking the type via the RangeImpl trait.

They should not be used directly anyway.

vks added 3 commits April 9, 2018 19:48

impl From<std::ops::Range> for RangeInt

06d7019

impl From<std::ops::Range> for Range

f3495ad

Change Range doctest to use std::ops::Range

ef9a742

pitdicker reviewed Apr 10, 2018

View reviewed changes

vks added 2 commits April 10, 2018 13:37

impl From<std::ops::Range> for RangeFloat

9bc2c2e

Use mul_add

13a639c

pitdicker reviewed Apr 10, 2018

View reviewed changes

Don't use mul_add

c97d6f4

It doesn't work with `no_std` and it is sometimes slower. This was documented in a comment.

Make From<std::ops::Range> impl more generic

869a367

pitdicker reviewed Apr 12, 2018

View reviewed changes

vks added 4 commits April 12, 2018 19:18

Add comment about optimal operation order

ecec2b6

Document that RangeInt and RangeFloat should not be used directly

6d5eda7

Don't impl From<std::ops::Range> for Range{Int,Float}

6a93182

They should not be used directly anyway.

Clarify comment

3756720

pitdicker merged commit 4da919d into rust-random:master Apr 12, 2018

vks deleted the from-range branch April 12, 2018 19:03

dhardy mentioned this pull request Apr 13, 2018

Rename Range to Uniform #393

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vks commented Apr 9, 2018

pitdicker commented Apr 9, 2018 •

edited

Loading

pitdicker Apr 10, 2018

vks Apr 10, 2018

dhardy Apr 11, 2018

pitdicker Apr 10, 2018

pitdicker Apr 10, 2018

vks Apr 10, 2018

pitdicker Apr 10, 2018

vks Apr 10, 2018

pitdicker Apr 10, 2018

vks Apr 10, 2018

pitdicker Apr 10, 2018

vks Apr 10, 2018

vks commented Apr 10, 2018

pitdicker commented Apr 10, 2018

vks commented Apr 11, 2018

pitdicker commented Apr 11, 2018

vks commented Apr 11, 2018

pitdicker left a comment

pitdicker Apr 12, 2018 •

edited

Loading

vks Apr 12, 2018

pitdicker Apr 12, 2018

vks Apr 12, 2018

pitdicker Apr 12, 2018

vks Apr 12, 2018

pitdicker Apr 12, 2018

vks Apr 12, 2018 •

edited

Loading

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

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

Conversation

vks commented Apr 9, 2018

pitdicker commented Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vks commented Apr 10, 2018

pitdicker commented Apr 10, 2018

vks commented Apr 11, 2018

pitdicker commented Apr 11, 2018

vks commented Apr 11, 2018

pitdicker left a comment

Choose a reason for hiding this comment

pitdicker Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vks Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

pitdicker commented Apr 9, 2018 •

edited

Loading

pitdicker Apr 12, 2018 •

edited

Loading

vks Apr 12, 2018 •

edited

Loading