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

Move Alias-method WeightedIndex to rand_distr #945

Merged
merged 6 commits into from
Mar 26, 2020

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Mar 9, 2020

This is ~500 lines of code that we don't need in the main Rand crate IMO. (The reason WeightedIndex is there is that it is used by the seq module; but the alias_method variant is not since it's initialisation cost makes it completely inappropriate for this single-use API.)

Question: should we deprecate the weighted pub module in rand, since this module doesn't need to be public now? (I guess this might require some re-exports or something internally, but we can do it.)

Similarly, the weighted pub module and alias_method sub-module in rand_distr seem somewhat unnecessary.

@vks
Copy link
Collaborator

vks commented Mar 19, 2020

Question: should we deprecate the weighted pub module in rand, since this module doesn't need to be public now? (I guess this might require some re-exports or something internally, but we can do it.)

Probably, but arguably this can be decided after merging this. We should probably have some release where it is just deprecated anyway. For rand_distr, maybe flattening the module structure from weighted::alias_method to weighted_alias_method makes sense?

@dhardy
Copy link
Member Author

dhardy commented Mar 20, 2020

Well, it also makes sense to tie up loose ends while we're on the topic. I'll make a change to deprecate the rand::distributions::weighted module then.

Regarding rand_distr, I think not, (1) because it's a breaking change with little motivation, and (2) because alias_method does use the error type from weighted.

@dhardy
Copy link
Member Author

dhardy commented Mar 20, 2020

Right, could you still review this @vks so I don't have to "force merge"?

@vks vks merged commit 8592ad3 into rust-random:master Mar 26, 2020
@vks
Copy link
Collaborator

vks commented Mar 26, 2020

Sorry, I forgot about this. The failure seemed unrelated, so I went ahead and merged.

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.

2 participants