Add WeightedIndexTree to rand_distr #1372
Conversation
rand_distr/src/weighted_tree.rs
Outdated
| impl<W: Clone + PartialEq + PartialOrd + SampleUniform + SubAssign<W> + Weight> | ||
| Distribution<Result<usize, WeightedError>> for WeightedTreeIndex<W> |
There was a problem hiding this comment.
We don't usually implement Distribution<Result<..>>. Do we need to?
I think I would prefer to panic on error, but guarantee no panic if self.is_valid() (self.can_sample()).
@vks?
There was a problem hiding this comment.
I removed the Result<>, so sample now panics instead. I added the info about is_valid to the doc string and introduced safe_sample as an alternative that does not panic.
Thank you for the review. I addressed all the comments |
|
Thanks, that is much better, changed to |
| // Otherwise we found the index with the target weight. | ||
| break; | ||
| } | ||
| assert!(target_weight >= W::ZERO); |
There was a problem hiding this comment.
If I understand correctly, this must be strictly > 0?
There was a problem hiding this comment.
As an intermediate state, this can be zero if all weights are zero or there are no elements. is_valid would then return false. Allowing this is useful, so that the user does not have to apply the updates in the right order to avoid intermediate zero states.
There was a problem hiding this comment.
In this case the function will already have returned an error.
There was a problem hiding this comment.
Sorry I mixed up what this comment was referring to.
I think target_weight can be 0 in this line, for example: if we have a tree with only one node and gen_range samples 0, then the loop block is a no-op and after we hit this line with target_weight = 0.
However, I realized the line right after was incorrect, it should be: assert!(target_weight < self.get(index)). Should be correct now.
|
We can ignore the Thanks for the implementation, @xmakro. |
dhardy
changed the title
As discussed in #1053