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

Is the SliceRandom::choose_weighted(...) method guaranteed to never choose an entry if it has zero weight? #1243

Closed
ISibboI opened this issue Jul 29, 2022 · 4 comments · Fixed by #1245

Comments

@ISibboI
Copy link
Contributor

ISibboI commented Jul 29, 2022

The documentation does not mention issues related to entries with zero weight.

It would seem logical, that when the weight is integer, but it is zero, that the corresponding entry can never be chosen.
For example, I would assume that the following code cannot fail:

use rand::prelude::*;

pub fn main() {
    let list = [1, 2, 3, 0];

    assert_ne!(*list.choose_weighted(&mut thread_rng(), |number| *number).unwrap(), 0);
}

(playground)

And additionally, the same should hold for floating point numbers. But there I am less sure, because depending on the implementation maybe there are issues with rounding errors?

use rand::prelude::*;

pub fn main() {
    let list = [1.0, 2.0, 3.0, 0.0];

    assert_ne!(*list.choose_weighted(&mut thread_rng(), |number| *number).unwrap(), 0.0);
}

(playground)

  1. Is it true that the integer version cannot fail? If yes, could this be added to the documentation to make it an official guarantee?
  2. Is it true that the floating point version cannot fail? If yes, could this be added to the documentation to make it an official guarantee?
@ISibboI ISibboI changed the title Is the SliceRandom::choose_weighted(...) method guaranteed to not never choose an entry if it has zero weight? Is the SliceRandom::choose_weighted(...) method guaranteed to never choose an entry if it has zero weight? Jul 29, 2022
@josephlr
Copy link
Member

So choose_weighted is a wrapper around the WeightedIndex distribution. The documentation for that distribution mentions that having a weight of zero gives a 0% chance an index will be selected.

The implementation for WeightedIndex uses binary_search_by here to avoid any edge-case issues when there is a weight of zero. As the comparator never returns Equal, it will always return the Err(i) where i is the index of the least element greater that the randomly selected element.

@josephlr
Copy link
Member

@ISibboI would a more visible link to the WeightedIndex distribution be enough to clarify this?

@ISibboI
Copy link
Contributor Author

ISibboI commented Jul 30, 2022

A link to WeightedIndex would definitely be great! Currently the link at the end of the docs of the choose_weighted method points to a deprecated module. And it is linked with "see also". Should maybe word the link with "For more details" or similar.

But even the WeightedIndex distribution does not mention anything about weights of zero and floats. So maybe additionally one should add a sentence there that zeros are handled correctly, even for floats.

I can make a pull request about this the next days if you agree.

@josephlr
Copy link
Member

@ISibboI a pull request that:

  1. Updates the choose_weighted (and similar) methods to have links to the WeightedIndex distribution.
  2. Removes any links to deprecated modules
  3. Explicitly mentions that zero weights (for floats and ints) are handled correctly, perhaps right before the example using weights of zero in the WeightedIndex docs.

Would be very welcome!

ISibboI added a commit to ISibboI/rand that referenced this issue Aug 2, 2022
…htedIndex` as discussed in rust-random#1243.

Additionally fix some minor issues.
dhardy pushed a commit that referenced this issue Oct 10, 2022
…behavior with floats (#1245)

* Fix the documentation for `choose_weighted(_mut)` as discussed in #1243.

* Mention that elements of zero weight are handled as expected by `WeightedIndex` as discussed in #1243.

Additionally fix some minor issues.

* Let the second example of `WeightedIndex` use floats to stress that they are handled correctly for the zero case.

* Manually indent doc comments.
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 a pull request may close this issue.

2 participants