Skip to content
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

gen_range(a, b) -> gen_range(a..b) and gen_range(a..=b) #1003

Merged
merged 2 commits into from
Aug 1, 2020

Conversation

vks
Copy link
Collaborator

@vks vks commented Jul 27, 2020

Replaces and closes #821.

@vks vks changed the title Implement Distribution for Range gen_range(a, b) -> gen_range(a..b) and gen_range(a..=b) Jul 30, 2020
@vks
Copy link
Collaborator Author

vks commented Jul 30, 2020

The only failure is a spurious network error.

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, shouldn't have tried reviewing this commit-by-commit!

src/distributions/uniform.rs Outdated Show resolved Hide resolved
src/distributions/uniform.rs Outdated Show resolved Hide resolved
src/rng.rs Outdated Show resolved Hide resolved
src/rng.rs Outdated
Comment on lines 124 to 165
fn gen_range<T: SampleUniform, B1, B2>(&mut self, low: B1, high: B2) -> T
fn gen_range<T, R>(&mut self, range: R) -> T
where
B1: SampleBorrow<T> + Sized,
B2: SampleBorrow<T> + Sized,
T: SampleUniform,
R: RangeBounds<T>
{
T::Sampler::sample_single(low, high, self)
use core::ops::Bound;
if let Bound::Included(low) = range.start_bound() {
match range.end_bound() {
Bound::Excluded(high) => T::Sampler::sample_single(low, high, self),
Bound::Included(high) => T::Sampler::sample_single_inclusive(low, high, self),
Bound::Unbounded => panic!("invalid upper bound"),
}
} else {
panic!("invalid lower bound");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't have to use run-time look-ups and panics. Can we do something like: where R: Into<Uniform<T>>?

No, really we need a specific trait for "sample single":

trait SampleSingle<X> {
    fn sample_single(self) -> X;
}
impl<X: SampleUniform> SampleSingle<X> for Range<X> { .. }
impl<X: SampleUniform> SampleSingle<X> for RangeInclusive<X> { .. }

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we would have to define such a trait if we want to avoid the lookup. Alternatively, we could inline gen_range and hope the lookup gets optimized away. What do you prefer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an implementation using static dispatch in the last commit. It required a new public trait that we may want to hide with #[doc(hidden)] to avoid users implementing it to add support for other range types.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think @newpavlov? Overall it seems more useful to expose the trait (sometimes users do want to extend generic APIs).

src/distributions/uniform.rs Outdated Show resolved Hide resolved
@vks
Copy link
Collaborator Author

vks commented Jul 31, 2020

Aha, shouldn't have tried reviewing this commit-by-commit!

Sorry, this should probably be rebased and squashed.

src/rng.rs Outdated Show resolved Hide resolved
src/rng.rs Outdated Show resolved Hide resolved
@vks vks mentioned this pull request Jul 31, 2020
@vks
Copy link
Collaborator Author

vks commented Jul 31, 2020

I'm not sure how to work around packed_simd not implementing PartialOrd. Is it ok to break gen_range's support for SIMD?

@TheIronBorn
Copy link
Collaborator

I think that's probably fine. Anyone who knows of the packed_simd support probably knows the API well enough to know about Uniform::sample_single

Ralith and others added 2 commits August 1, 2020 15:39
This includes a specialized implementation for integers.
This adds support for `Rng::range(a..b)` and `Rng::range(a..=b)`,
replacing `Rng::range(a, b)`. `a` and `b` must now implement
`PartialEq`.

This is a breaking change. In most cases, replacing
`Rng::gen_range(a, b)` with `Rng::gen_range(a..b)` should be enough,
however type annotations may be necessary, and `a` and `b` can no longer
be references or SIMD types.

SIMD types are still supported by `UniformSampler::sample_single`.

Some clippy warnings were fixed as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants