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

CHANGE: rename Rng methods random, gen_* #1503

Closed
dhardy opened this issue Oct 7, 2024 · 9 comments
Closed

CHANGE: rename Rng methods random, gen_* #1503

dhardy opened this issue Oct 7, 2024 · 9 comments

Comments

@dhardy
Copy link
Member

dhardy commented Oct 7, 2024

#989 discusses the addition of methods like rand::range, which leads back to Rng::gen_range.

#1438 and #1500 already renamed genrandom and gen_iterrandom_iter.

Since we no longer have Rng::gen, should we rename gen_range, gen_bool and gen_ratio?

Aim: make method/function names consistent, memorable and easily comprehensible.

Proposals

Old (v0.8)

Before gen was deprecated, we had:

rng.gen()
rng.gen_range(0..len)
rng.gen_bool(0.2)
rng.gen_ratio(2, 3)
rng.sample(Open01)
rng.sample_iter(Open01)
rng.fill(&mut buf)
rng.try_fill(&mut buf)

Status quo

If we make no further changes, we have the following Rng methods:

rng.random()
rng.random_iter()
rng.sample(Open01)
rng.sample_iter(Open01)
rng.fill(&mut buf)
rng.gen_range(..len)
rng.gen_bool(0.2)
rng.gen_ratio(2, 3)
#[deprecated] rng.gen()

gen_bool → p; drop gen prefix

We could just:

rng.range(..len)
rng.bool(0.2)
rng.ratio(2, 3)

And yes, it appears that we can use bool as a method name (it's a primitive type, not a keyword).

gen_bool → p; drop gen prefix

This is the most concise change:

rng.range(..len)
rng.p(0.2)
rng.ratio(2, 3)

Is it clear enough that rng.p(p) is generating a random variate where P(X = true) = p?

Some weird variations on this:

rng.bool_p(0.2)
rng.bool_ratio(2, 3)

rng.true_p(0.2)
rng.true_ratio(2, 3)

gen_ → random_

A straightforward replacement gives us:

rng.random()
rng.random_iter()
rng.random_range(..len)
rng.random_p(0.2)
rng.random_ratio(2, 3)

This seems fine, if a bit long.

random → next

We already have RngCore::next_u32 etc., so we could rename random (what was gen) to next:

rng.next()
rng.next_iter()
rng.next_range(..len)
rng.next_p(0.2)
rng.next_ratio(2, 3)

Caveat 1: this no longer matches rand::random (which we probably don't want to rename to rand::next).

Caveat 2: rng.next_iter() is a weird name; we can't really use rng.iter() however since it generates an iterator over Standard samples, not an abstract iterator over the RNG.

@newpavlov
Copy link
Member

newpavlov commented Oct 7, 2024

I am in favor of using the random_ prefix, we also could shorten it to rand_. rng.ratio(2, 3) looks a bit weird in my opinion. rng.next_ratio(2, 3) is even worse.

@dhardy
Copy link
Member Author

dhardy commented Oct 7, 2024

I agree, except that random_range is rather long.

@newpavlov
Copy link
Member

It's just 3 letters longer than gen_range and it has good readability.

@dhardy
Copy link
Member Author

dhardy commented Oct 7, 2024

I agree. Any other thoughts @vks, @oconnor663?

@JamboChen
Copy link
Contributor

I prefer using rng.bool_p(0.2) over rng.p(0.2). If I were using the rand library for the first time and had no documentation, it would be hard for me to figure out that .p(0.2) generates a bool just from the name. If someone wants to flip a bunch of coins quickly, I doubt they'd search the docs for "p." They're more likely to look for "bool," "true," or even "succ."

image

Maybe this understanding issue could be addressed with a larger poll.

@vks
Copy link
Collaborator

vks commented Oct 7, 2024

This is just bikeshedding, but I think p is too short. In my opinion, it should be at least probability or with_probability.

There is precedence for just using the type names, this is what fastrand does.

@dhardy
Copy link
Member Author

dhardy commented Oct 7, 2024

Then it sounds like the consensus may be:

rng.random()
rng.random_iter()
rng.random_range(..len)
rng.bool(0.2)
rng.bool_ratio(2, 3)

This is just bikeshedding, but I think p is too short. In my opinion, it should be at least probability or with_probability.

I think your suggestions are too long!

fastrand is weird in that bool, float types and a few others are unparameterised, integer types and char are parameterised via a range, digit is parameterised via a base, and then other functions like seed and shuffle are thrown in there.

@newpavlov
Copy link
Member

rng.bool(0.2)

How about rng.random_bool(p: impl Into<Probability>)? It would be generic over floats and ratio tuple. For explicitness user also could use Probability::Ratio(x, y).

@oconnor663
Copy link
Contributor

I haven't had occasion to use gen_bool myself, so I don't have many thoughts about it, but just to throw another name in the pot, how do folks feel about chance? Something like this:

if rng.chance(0.5) {
    println!("winner!");
}

@dhardy dhardy closed this as completed Oct 16, 2024
dhardy added a commit that referenced this issue Oct 31, 2024
Adds random_iter, random_range, random_bool, random_ratio, fill.
See also #989, #1503.

Co-authored-by: Diggory Hardy <[email protected]>
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

No branches or pull requests

5 participants