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

WeightedIndex distribution naming #763

Closed
dhardy opened this issue Mar 29, 2019 · 4 comments
Closed

WeightedIndex distribution naming #763

dhardy opened this issue Mar 29, 2019 · 4 comments
Labels
E-help-wanted Participation: help wanted

Comments

@dhardy
Copy link
Member

dhardy commented Mar 29, 2019

As noted in the original PR and here, we're not entirely happy with the path/name of weighted index distributions:

use rand::distributions::weighted::WeightedIndex;
// or:
use rand::distributions::weighted::alias_method::WeightedIndex;

These paths are long, slightly repetitive ("weighted"), and one of the paths includes the algorithm name which we usually avoid. Also, Index is potentially unnecessary (this is designed for indexing weighted arrays, but is essentially just a biased finite integer distribution).

This needs review before 0.7.

@dhardy dhardy added this to the 0.7 release milestone Mar 29, 2019
@dhardy dhardy added the E-help-wanted Participation: help wanted label Apr 10, 2019
@dhardy
Copy link
Member Author

dhardy commented Jun 14, 2019

An alternative may be to move this to rand_distr or a new crate. Rationale:

  • it is not very commonly used (154 hits for rand + weightedindex on GH with lang:rust vs 87920 for just rand)
  • aside from Bernoulli, this is the only non-linear distribution included in Rand

@vks
Copy link
Collaborator

vks commented Jun 14, 2019

Moving it to rand_distr is problematic, because rand::seq needs it, and we don't want rand to depend on rand_distr. This would probably require moving rand::seq to its own crate.

@dhardy
Copy link
Member Author

dhardy commented Jun 14, 2019

True, there are some uses of SliceIterator::choose_weighted, but not many (64 total hits, many of them just code copies).

We could instead use another extension trait, but at only 738 lines for both weighted impls it's barely worth it.

@newpavlov
Copy link
Member

Closed in #1008.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-help-wanted Participation: help wanted
Projects
None yet
Development

No branches or pull requests

3 participants