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

add rand::range, random_bool, random_iter, random_ratio, fill as top-level helper functions #1488

Merged
merged 10 commits into from
Oct 31, 2024

Conversation

oconnor663
Copy link
Contributor

@oconnor663 oconnor663 commented Aug 14, 2024

Original issue: #989

In that issue, @newpavlov originally proposed several new helpers:

  • gen_range
  • fill
  • gen_bool/gen_ratio
  • choose/choose_mut
  • shuffle

As I started playing with implementing these, I narrowed it down to two functions that seemed the most useful/obvious to me:

  • range: The helper I most frequently wish I had. A flexible way to get a random float or an index into a slice. A "pit of success" around the random::<usize>() % len mistake. Good integration with Rust's native range syntax. Renamed from gen_range, mainly because it's more convenient to type/read, and also to give more space to the upcoming gen keyword.
  • shuffle: An obvious/straightforward API. Useful in small examples, tests, etc.

In my mind, the case for these others is less compelling:

  • fill: The most common use case might be generating 128-bit or 256-bit seeds/keys, but random() already works for arrays up to size 32.
  • gen_bool: Also doable by comparing a float to random(), and that alternative might be easier to read.
  • choose: Useful, but you want two or three different variants (&T, and &mut T, and maybe T: Copy). Easier to get an index from range() and let the caller decide what to do with it.

So in this PR I've implemented rand::range and rand::shuffle. Very interested to get other folks' thoughts.

src/lib.rs Outdated
/// println!("{}", y);
/// ```
///
/// If you're calling `range()` in a loop, caching the generator as in the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mean caching the distribution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wording is copied from lines 135-136 above.

Copy link
Member

Choose a reason for hiding this comment

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

If the range is non-constant, caching the distribution helps. Either way, caching the result of rand::thread_rng also helps (slightly).

src/lib.rs Outdated Show resolved Hide resolved
@dhardy
Copy link
Member

dhardy commented Aug 21, 2024

Some thoughts on this:

  • These functions make rand easier/quicker to use in some very specific cases
  • These functions hide the source of the randomness; this is okay though not exactly ideal
  • The real reason this is more convenient than rand::thread_rng().gen_range(0..len) or rand::thread_rng.shuffle(array) is that the latter require trait imports. Custom preludes might help, but we can't bet on that ever happening.
  • We could re-implement the Rng methods under ThreadRng using inherent methods which forward to the trait methods. This avoids the need to import Rng (for ThreadRng only, though there's no reason other RNGs couldn't do the same thing) and shouldn't cause issues (inherent methods are always resolved over trait methods).
  • Inherent traits provides much the same capability as above, potentially with better documentation and less boilerplate.
  • We already have rand::random; if the above (inherent trait methods) happens I might even consider removing this.
  • We don't have Rng::choose and Rng::shuffle because they are redundant with the slice/iterator variants, but with inherent methods on ThreadRng this might actually make sense (enabling rand::thread_rng.shuffle(&mut my_vec)).

Thus I think we should probably reject this PR.


In my mind, the case for these others is less compelling:

  • fill: The most common use case might be generating 128-bit or 256-bit seeds/keys, but random() already works for arrays up to size 32.

fill and random are not equivalent for byte-arrays (#1282).

@oconnor663
Copy link
Contributor Author

I put a wall of text on the issue thread 😅

@dhardy dhardy added the P-postpone Waiting on something else label Sep 9, 2024
@dhardy dhardy mentioned this pull request Sep 10, 2024
1 task
@dhardy
Copy link
Member

dhardy commented Oct 16, 2024

@oconnor663 would you like to update this? #1505 renamed several methods; #1506 renamed thread_rngrng.

I don't think we need sample as a top-level function, but potentially we can add the rest:

  • random — existing
  • random_iter — potentially-useful adapter?
  • random_range — requested
  • random_bool — I'd rather promote this than rng.random() < p
  • random_ratio — less important, but for symmetry why not?
  • fill — faster than random for byte arrays

Documentation should in my opinion be relatively short, e.g. copy the first line of the Rng method then show equivalent code linking rand::rng and the same-named Rng method.

@dhardy dhardy removed the P-postpone Waiting on something else label Oct 23, 2024
@dhardy
Copy link
Member

dhardy commented Oct 25, 2024

Is this a bit much?
image
All of those features are enabled by standard.

We could hide some or all, but this is potentially confusing.

#![feature(doc_cfg_hide)]
#![doc(cfg_hide(feature = "std"))]

We could introduce a new feature-flag like thread_rng and cfg-gate ThreadRng behind that instead. I'm not sure that "make the docs look better" is sufficient justification for this however.

@dhardy
Copy link
Member

dhardy commented Oct 29, 2024

@newpavlov any comments before I merge this? I think it's acceptable now.

src/lib.rs Show resolved Hide resolved
#[cfg(all(feature = "std", feature = "std_rng", feature = "getrandom"))]
#[inline]
#[track_caller]
pub fn random_ratio(numerator: u32, denominator: u32) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

I still don't quite like this name, but I guess it's better to handle separately.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't much like any of the suggested alternatives, but feel free to open a new PR or re-open #1503 for this.

@TheIronBorn
Copy link
Collaborator

We should remove "shuffle" from the PR name as well

@dhardy dhardy changed the title add rand::range and rand::shuffle as top-level helper functions add rand::range, random_bool, random_iter, random_ratio, fill as top-level helper functions Oct 31, 2024
@dhardy dhardy merged commit 585b29f into rust-random:master Oct 31, 2024
14 checks passed
@oconnor663
Copy link
Contributor Author

oconnor663 commented Oct 31, 2024

Thanks for landing this! And apologies for disappearing 😅

I agree with @newpavlov that the names random_ratio and random_bool don't feel great side-by-side. What do you guys think of random_probability and random_probability_f32? If there's any interest in that, I'd be happy to make a PR.

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