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 doesn't take a Range #744

Closed
cramertj opened this issue Feb 22, 2019 · 12 comments
Closed

gen_range doesn't take a Range #744

cramertj opened this issue Feb 22, 2019 · 12 comments
Labels
F-new-int Functionality: new, within Rand
Milestone

Comments

@cramertj
Copy link
Contributor

Is there a reason https://docs.rs/rand/0.6.5/rand/trait.Rng.html#method.gen_range doesn't take an std::ops::Range? This would be more convenient for specifying inclusive/exclusive boundary behaviors (e.g. 128..=255u8).

@TheIronBorn
Copy link
Collaborator

TheIronBorn commented Feb 23, 2019

gen_range has to allow types which don't implement Range.

You can use ranges with Uniform::from though:

rng.sample(Uniform::from(0..=4))

NOTE: this isn't an exact copy of gen_range. gen_range uses Uniform::sample_single which isn't possible with this syntax

@TheIronBorn
Copy link
Collaborator

Uniform:::new_inclusive also exists

@dhardy
Copy link
Member

dhardy commented Feb 23, 2019

In part it's historical, and since Rust doesn't support function overloading / multi-functions, we can't allow both. Maybe rng.gen_range(1..=6) would be nice to have, but I think this is too widely used a function to bring a breaking change.

@hmble
Copy link

hmble commented Apr 3, 2019

@TheIronBorn if Uniform::new_inclusive works for you then maybe there is no need to use rng.gen_range(1..=6). I'm a beginner I don't know if there is any performance difference between thw two methods, if that's a case then maybe we can implement rng.gen_with_range(1..=6)

@dhardy
Copy link
Member

dhardy commented Apr 5, 2019

@hmble there is potentially some performance difference; UniformSampler::sample_single(low, high, rng) is designed to be faster (for single use) than Uniform::new(low, high).sample(rng).

But is it worth having both rng.gen_range(1, 7) and rng.gen_with_range(1..=6)? If so, should it be gen_from_range or just from_range?

@hmble
Copy link

hmble commented Apr 5, 2019

@dhardy I guess no its not. IMO we can always use rng.gen_range(start, end+1). Is there any case where we cannot use this approach ? If we decide to implement a method then from_range would be nice method name.

@vks
Copy link
Collaborator

vks commented Apr 7, 2019 via email

@hmble
Copy link

hmble commented Apr 7, 2019

It seems a good choice to create another method from_rng

@dhardy
Copy link
Member

dhardy commented Apr 8, 2019

While @vks has a theoretical concern, I don't know how much it is a real concern, and there are workarounds (check the upper-bound or use Uniform::new_inclusive).

We could add another method like sample_range, but this implies the need for a new function like UniformSampler::sample_single_inclusive. I'm not too keen on adding more functionality there, and we can't simply change the meaning of the bounds (the type of breaking change causing bugs).

@dhardy dhardy added the F-new-int Functionality: new, within Rand label Apr 10, 2019
@dhardy
Copy link
Member

dhardy commented Jun 4, 2019

I think there are basically two options on this one:

  1. We keep the status quo. This is reasonably flexible since:

    • we already support Uniform<X>: From<Range<X>>
    • unpacking values from a range is not hard: rng.gen_range(rng.start, rng.end)
    • we already support Uniform::new and Uniform::new_inclusive
  2. We switch rng.gen_range and Uniform::new to support any B: RangeBounds and drop Uniform::new_inclusive. This:

    • is a major breaking change
    • is strictly more flexible and should have neat code (all relevant functions are already generic)
    • should be easy to use (Range<T> only requires that T: PartialOrd which is already satisfied by all types supported by Uniform)

If not for the breaking change, the second option might be better (though would have to see an implementation first). However, I don't think it's enough better to justify this breaking change?

There is a third option: keep Rng::gen_range as-is but add Rng::gen_bounds or similar, and either adjust Uniform::new or add Uniform::new_bounds. However, this adds redundant code and API which we prefer to avoid (we already have a large API).

@vks
Copy link
Collaborator

vks commented Jun 4, 2019

I think we should probably go with option 2 before Rand 1.0. However, for now I think we should spend our budget for breaking changes elsewhere.

@vks
Copy link
Collaborator

vks commented Aug 28, 2020

Rng::gen_range now takes an inclusive or exclusive range.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-new-int Functionality: new, within Rand
Projects
None yet
Development

No branches or pull requests

5 participants