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

Rng renames: gen_ → random_ #1505

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Oct 9, 2024

  • Added a CHANGELOG.md entry

Summary

This appears to be the conclusion of #1503:

rng.random()
rng.random_iter()
rng.random_range(..len)
rng.random_bool(0.2)
rng.random_ratio(2, 3)
rng.sample(Open01)
rng.sample_iter(Open01)
rng.fill(&mut buf)

Opinion

My personal feeling is that this is a poor choice. We recently renamed rand::distributions to rand::distr because the latter was easier to type and consistent with rand_distr (see #1381). This achieves some level of consistency (excepting that random_bool describes its output while other random* methods describe their input), but is not concise.

@dhardy dhardy marked this pull request as ready for review October 11, 2024 09:01
@dhardy dhardy requested a review from newpavlov October 11, 2024 09:02

/// Alias for [`Rng::random_range`].
#[inline]
#[deprecated(since = "0.9.0", note = "Renamed to `random_range`")]
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's better to do a deprecating backport and release in the v0.8 branch and remove these methods completely in v0.9?

/// ```
///
/// [`Bernoulli`]: distr::Bernoulli
#[inline]
#[track_caller]
fn gen_ratio(&mut self, numerator: u32, denominator: u32) -> bool {
fn random_ratio(&mut self, numerator: u32, denominator: u32) -> bool {
Copy link
Member

@newpavlov newpavlov Oct 11, 2024

Choose a reason for hiding this comment

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

I think this name could be misleading. It sounds as if it generates a "random ratio". But I don't have a better proposal than much longer random_bool_with_ratio or the generic argument proposal, so I guess it's fine.

@vks
Copy link
Collaborator

vks commented Oct 11, 2024

I don't really like the new name. I would prefer something like sample_* or uniform_*, but I guess this is too technical.

I agree that random_ratio is misleading, but so is gen_ratio. I think bool_from_ratio or bool_from_prob would also work.

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.

3 participants