Rename "range" to "uniform" by vks · Pull Request #395 · rust-random/rand

vks · 2018-04-12T17:06:45Z

gen_range was not changed.
The types are still available under their deprecated, old names.

Fixes #393.

vks · 2018-04-12T18:01:42Z

I reexported Uniform as Range, but this was not done when Uniform was renamed to Standard. It is impossible to support both. Should I just remove the compatibility reexports?

pitdicker · 2018-04-12T20:38:14Z

To be honest I feel like this PR is a bit premature, but I'm not really sure what to say. @dhardy, can I leave things to you 😄?

I believe if we want to have Uniform, it should purely be an addition. Uniform should be a wrapper around Range or visa versa.

I reexported Uniform as Range, but this was not done when Uniform was renamed to Standard. It is impossible to support both. Should I just remove the compatibility reexports?

We don't need compatibility reexports for the Uniform to Standard rename, because there never really was a release that included Uniform. But Range has been there for years.

vks · 2018-04-12T22:03:31Z

I believe if we want to have Uniform, it should purely be an addition. Uniform should be a wrapper around Range or visa versa.

Why? They are basically the same. I could imagine to have type aliases.

We don't need compatibility reexports for the Uniform to Standard rename, because there never really was a release that included Uniform. But Range has been there for years.

👍

dhardy · 2018-04-15T19:19:42Z

I like the name Uniform a little more myself, but there doesn't seem to be an absolute answer.

Either way there are a few other changes in this PR we want (finishing the rename from Uniform → Standard).

dhardy

After some thinking about this change, it makes a lot of sense. We are emphasising how variables are sampled, not the domain in which they are sampled.

This also opens up some possible additional constructors like Uniform::default() and Uniform::full_range() — although the former is redundant with Standard and the latter doesn't make sense for floating-point types.

I think Standard still has an important role to play: it is simpler, thus easier to understand and probably faster to compile code using it. Also Standard uses no memory.

dhardy · 2018-04-16T14:43:30Z

src/distributions/mod.rs

 pub use self::other::Alphanumeric;
-pub use self::range::Range;
+pub use self::uniform::Uniform;
+#[deprecated(since="0.5.0", note="use Distribution instead")]


Shouldn't this say "use Uniform instead"?

Yes, I fixed it.

dhardy · 2018-04-16T14:46:14Z

src/distributions/mod.rs

+    pub use distributions::uniform::Uniform as Range;
+    pub use distributions::uniform::UniformInt as RangeInt;
+    pub use distributions::uniform::UniformFloat as RangeFloat;
+    pub use distributions::uniform::SampleUniform as SampleRange;


I don't think there's any point exporting the last three here because RangeInt and RangeFloat were never in a release and SampleRange is totally different.

I agree about Range* and removed them. (I was not aware they were not released.)

However, what do you mean about SampleRange? It was public (although not supposed to be used), so removing is a breaking change, hence the deprecated reexport.

Compare the old and the new. But I guess usage as a bound (T: SampleRange) is not affected at least.

Oh, so you are saying it is a breaking change anyway? I still think it makes sense to have it deprecated for consistency. As you said, it might possibly keep some code working this way, but I suspect it was not used much.

dhardy · 2018-04-16T15:00:49Z

To be honest I feel like this PR is a bit premature

IMO it's much better to get this right now than after the 0.5 release.

I believe if we want to have Uniform, it should purely be an addition. Uniform should be a wrapper around Range or visa versa.

No, having two names for the same thing makes no sense IMO (except as deprecated re-export as in this PR).

pitdicker

Either way there are a few other changes in this PR we want (finishing the rename from Uniform → Standard).

👍

After some thinking about this change, it makes a lot of sense. We are emphasising how variables are sampled, not the domain in which they are sampled.

Purely thinking within the domain it makes sense. As long as we keep the name Rng::gen_range, and don't purge the name "range" entirely from the documentation, I think it is ok.

I left some small notes on the documentation.

pitdicker · 2018-04-16T18:22:37Z

src/distributions/mod.rs

    fn ind_sample<R: Rng>(&self, &mut R) -> Support;
 }

+/// DEPRECATED: Use `distributions::uniform` instead.


Nit (sorry): distributions::Uniform

The spelling is as intended, because it addresses the module and not the type.

pitdicker · 2018-04-16T18:23:24Z

src/distributions/uniform.rs

 /// Sample values uniformly between two bounds.
 ///
-/// `Range::new` and `Range::new_inclusive` will set up a `Range`, which does
+/// `Uniform::new` and `Uniform::new_inclusive` will set up a `Uniform`, which does


I don't think this sentence read well any more, but have difficulty saying why.

Can you also mention "range" once in this description?

You're right. Perhaps something like U::new and U::new_inclusive construct a Uniform distribution sampling from the half-open and closed (inclusive) ranges respectively.

I reworded it.

pitdicker · 2018-04-16T18:26:56Z

src/lib.rs

+    /// repeatedly with the same arguments, one should use `Uniform`, as
    /// that will amortize the computations that allow for perfect
-    /// uniformity, as they only happen when constructing the `Range`.
+    /// uniformity, as they only happen when constructing the `Uniform`.


Can we mention "range" one or two times in this comment? I don't want to see no mention at all to this common term any more.

It is already mentioned in the name and the fist sentence.

I agree with @pitdicker actually that a couple of things could be changed:

"wrapper around distributions::Uniform::sample_single"

"when constructing the Uniform range."

vks · 2018-04-17T11:38:53Z

I will rebase after review is finished.

dhardy

I see you pushed the From<ops::Range<X>> support into the same PR; I think we're all happy with that?

dhardy · 2018-04-17T12:31:48Z

src/distributions/uniform.rs

 /// Implementation of `UniformImpl` for integer types.
+///
+/// Unless you are implementing `RangeImpl` for your own type, this type should
+/// not be used directly, use `Range` instead.


You forgot to update the Range names

Sorry, this is a left-over from the merge...

dhardy · 2018-04-17T12:31:56Z

src/distributions/uniform.rs

+    fn from(r: ::core::ops::Range<X>) -> Uniform<X> {
+        Uniform::new(r.start, r.end)
+    }
+}


dhardy · 2018-04-17T12:37:14Z

src/distributions/uniform.rs

+    /// range `[low, high]` (inclusive). Panics if `low > high`.
    pub fn new_inclusive(low: X, high: X) -> Uniform<X> {
-        assert!(low < high, "Uniform::new called with `low >= high`");
+        assert!(low <= high, "Uniform::new_inclusive called with `low > high`");


Good idea to solve #403. I looked over the code and don't see any issues with low == high.

Yes. low == high is nonsensical, but not impossible.

It is the trivial case, but but it is well-defined and supporting it might simplify more general code.

dhardy · 2018-04-17T12:41:06Z

Not sure why the comments from my previous review are "outdated"; there still appear to be some spurious uses of Range and RangeImpl in the doc.

vks · 2018-04-17T14:09:08Z

I rebased on master.

dhardy · 2018-04-17T14:47:20Z

@vks could you fix the tests please?

* `gen_range` was not changed. * The types are still available under their deprecated, old names. Fixes rust-random#393.

Fixes rust-random#403.

vks · 2018-04-17T17:20:01Z

Sorry again about that, something went wrong during the rebase (I already fixed those problems during the merge).

Fixed and rebased so the whole history passes the tests.

dhardy

So other than a couple of very minor things I'm ready to see this merged. @pitdicker are you happy with it now?

dhardy · 2018-04-18T10:28:44Z

src/distributions/uniform.rs

-    /// sampling only a limited number of values from range. The default
-    /// implementation just sets up a range with `RangeImpl::new` and samples
+    /// Via this method, implementations can provide a method optimized for
+    /// sampling only a limited number of values from the range. The default


I guess this should say "for sampling only a single value from the specified range", but not actually a problem with your PR.

dhardy · 2018-04-18T10:31:44Z

src/lib.rs

+    /// repeatedly with the same arguments, one should use `Uniform`, as
    /// that will amortize the computations that allow for perfect
-    /// uniformity, as they only happen when constructing the `Range`.
+    /// uniformity, as they only happen when constructing the `Uniform`.


I agree with @pitdicker actually that a couple of things could be changed:

"wrapper around distributions::Uniform::sample_single"

"when constructing the Uniform range."

pitdicker · 2018-04-18T10:34:54Z

@pitdicker are you happy with it now?

Yes, feel free to merge when ready 😄

Final part of #395

dhardy · 2018-04-18T14:08:18Z

Okay, I made the final doc changes myself.

vks · 2018-04-18T16:21:02Z

Thanks!

dhardy reviewed Apr 16, 2018

View reviewed changes

dhardy added E-question Participation: opinions wanted B-API Breakage: API T-distributions P-high Priority: high labels Apr 16, 2018

dhardy mentioned this pull request Apr 16, 2018

Rename Range to Uniform #393

Closed

pitdicker reviewed Apr 16, 2018

View reviewed changes

dhardy reviewed Apr 17, 2018

View reviewed changes

dhardy approved these changes Apr 17, 2018

View reviewed changes

vks force-pushed the uniform branch from c96afa5 to a33ef6d Compare April 17, 2018 14:07

vks added 7 commits April 17, 2018 19:18

Rename "range" to "uniform"

8fcd8d1

* `gen_range` was not changed. * The types are still available under their deprecated, old names. Fixes rust-random#393.

benches: Update distributions to use new names

344b799

Rexport Uniform as Range for backwards compatibility

dd56c3e

Fix typo

338b909

Don't reexport deprecated unreleased pathes

12a52fc

Reword Uniform docs to sound less awkward

e4db7ce

Fix Uniform::new_inclusive(x, x)

5bb3eb8

Fixes rust-random#403.

vks force-pushed the uniform branch from a33ef6d to 5bb3eb8 Compare April 17, 2018 17:19

dhardy approved these changes Apr 18, 2018

View reviewed changes

dhardy merged commit b418b61 into rust-random:master Apr 18, 2018

dhardy added a commit that referenced this pull request Apr 18, 2018

Uniform range sampling: improve docs

8c5b0ac

Final part of #395

vks deleted the uniform branch April 18, 2018 16:20

dhardy mentioned this pull request May 15, 2018

Update changelog #453

Merged

Uh oh!

Conversation

vks commented Apr 12, 2018

Uh oh!

vks commented Apr 12, 2018

Uh oh!

pitdicker commented Apr 12, 2018

Uh oh!

vks commented Apr 12, 2018

Uh oh!

dhardy commented Apr 15, 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!

dhardy commented Apr 16, 2018

Uh oh!

pitdicker left a comment

Choose a reason for hiding this comment

Uh oh!

pitdicker Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

vks commented Apr 17, 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!

dhardy commented Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vks commented Apr 17, 2018

Uh oh!

dhardy commented Apr 17, 2018

Uh oh!

vks commented Apr 17, 2018

Uh oh!

dhardy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

pitdicker Apr 16, 2018 •

edited

Loading

dhardy commented Apr 17, 2018 •

edited

Loading