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

Rename gen_bool and gen_ratio to with_probability and with_ratio #553

Closed
wants to merge 1 commit into from

Conversation

sicking
Copy link
Contributor

@sicking sicking commented Jul 13, 2018

The gen_bool and gen_ratio functions will most commonly be used in situations like

if rng.gen_bool(0.42) {
    ...
}

However the name of that those functions feel awkward when used in this scenario. Not only is the "gen" part redundant, since that's what rng objects generally do (and what the "g" in "rng" stands for). But the fact that we're branching on a boolean value doesn't actually say anything about how that boolean is created.

Likewise gen_ratio doesn't actually generate a ratio, it generates a boolean with a probability expressed as a ratio.

This PR renames gen_bool and gen_ratio to with_probability and with_ratio. So code instead reads as:

if rng.with_probability(0.42) {
    ...
}

and

if rng.with_ratio(2, 5) {
    ...
}

The old names are kept for now, but with a deprecation warning.

@sicking
Copy link
Contributor Author

sicking commented Jul 13, 2018

Test failure looks like a tool problem here too.

@sicking sicking mentioned this pull request Jul 13, 2018
28 tasks
@sicking
Copy link
Contributor Author

sicking commented Jul 13, 2018

I should also mention that both of these functions have very little usage right now. gen_bool was added in 0.5 and I found somewhere in the 60s uses on GitHub. gen_ratio was added after 0.5 so there's no compatibility concerns there.

In fact, I'll remove the deprecation "wrapper" for gen_ratio.

…Rng::with_ratio. Also rename Bernoulli::from_ratio to Bernoulli::with_ratio.
@sicking
Copy link
Contributor Author

sicking commented Jul 13, 2018

This also renames Bernoulli::from_ratio to Bernoulli::with_ratio since the similarity with Rng::with_ratio is nice. But I think that change is less important.

@dhardy
Copy link
Member

dhardy commented Jul 13, 2018

I restarted that test but it might fail again. 🤷‍♂️

To be honest I'm not keen on your names either. They're long and also feel awkward (it's not "rand with ...", it's "rng with ...").

@dhardy
Copy link
Member

dhardy commented Jul 13, 2018

I asked about the build failure: koute/cargo-web#116

@sicking
Copy link
Contributor Author

sicking commented Jul 13, 2018

The "with probability" felt descriptive given that you enter the if-branch with a given probability. So reading the code it feels a lot more clear what's going on.

But I agree that a shorter name would be nice if it can be kept descriptive.

@pitdicker
Copy link
Contributor

In my opinion the gen_bool name was fine. gen_ratio was not really generating a ratio, but a bool with a ratio as probability. Maybe gen_bool_ratio might be better, but I didn't want to suggest it in the original PR.

I can understand the idea behind with_probability. I doesn't feel natural to me though. rng.gen_bool(0.3) means having an RNG generate a bool with probability 0.3. rng.with_probability(0.3) seems missing something: have the RNG do something with probability 0.3.

@sicking
Copy link
Contributor Author

sicking commented Jul 17, 2018

Ok

@sicking sicking closed this Jul 17, 2018
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