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 alias_method::WeightedIndex to WeightedAliasIndex #1008

Merged
merged 4 commits into from Aug 2, 2020
Merged

Conversation

newpavlov
Copy link
Member

@newpavlov newpavlov commented Aug 1, 2020

I think it makes things a bit more clear and simplifies module structure.

Also how about moving distributions from weighted and uniform modules to top-level? It's a bit strange that only they stay in their own module, but not other distributions.

@vks
Copy link
Collaborator

vks commented Aug 1, 2020

Also how about moving distributions from weighted and uniform modules to top-level? It's a bit strange that only they stay in their own module, but not other distributions.

I think it makes sense for weighted. Are you suggesting to move all public types from uniform to the top level or just the distributions? I think it makes sense to have Uniform top-level, but the backend traits and structs should probably stay in uniform, since most users don't need to worry about them.

@newpavlov
Copy link
Member Author

the backend traits and structs should probably stay in uniform

Makes sense. What about the AliasableWeight trait? Should we keep weighted module just for it?

@dhardy
Copy link
Member

dhardy commented Aug 1, 2020

There's an issue on this linked in the 0.8 tracker: #763. Do we still want HighPrecision? In a way, the current name makes sense because users may simply replace rand::distributions::WeightedIndex with rand_distr::alias_method::WeightedIndex (although a renaming import allows this anyway).

@dhardy
Copy link
Member

dhardy commented Aug 1, 2020

Note also that the distributions::uniform module is in line with std::collections::hash_map: the main type is re-exported from the parent module, but related items aren't.

@newpavlov
Copy link
Member Author

I wonder if need to re-export those items from rand in the first place. I think rare direct users will be able to use them directly from rand.

@dhardy
Copy link
Member

dhardy commented Aug 1, 2020

Sure, but is there a reason not to? The original design for rand_distr was to be a super-set of rand::distributions.

pub use self::weighted::{WeightedError, WeightedIndex};
pub use rand::distributions::weighted::{WeightedError, WeightedIndex};
#[cfg(feature = "alloc")]
pub use weighted_alias::WeightedAliasIndex;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this results in WeightedAliasIndex being shown as re-exported item in documentation.

@newpavlov
Copy link
Member Author

Ok, let's leave modules. If you don't hav any objections, I think this PR is ready for merge.

@vks
Copy link
Collaborator

vks commented Aug 1, 2020

There is a test failure:

error[E0432]: unresolved import `rand_distr::weighted`
 --> src/weighted_alias.rs:42:17
  |
4 | use rand_distr::weighted::WeightedAliasIndex;
  |                 ^^^^^^^^ could not find `weighted` in `rand_distr`

@newpavlov
Copy link
Member Author

Ouch, fixed.

@newpavlov
Copy link
Member Author

newpavlov commented Aug 1, 2020

BTW it looks like CI does not properly test this crate with disabled default features (see use of std::f64::NAN which was not caught in the first commit) and with only alloc being enabled (we have to forward features, since WeightedIndex is feature gated). rust-lang/cargo#4866 could be partially responsible. I guess it should be fixed in a different PR.

@vks
Copy link
Collaborator

vks commented Aug 2, 2020

Looks good, thanks!

@vks vks merged commit 321ae39 into master Aug 2, 2020
@vks vks deleted the rename_weighted branch August 2, 2020 00:40
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.

None yet

3 participants