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

Initial implementation of Slice distribution #1107

Merged
merged 8 commits into from
May 12, 2021

Conversation

Lucretiel
Copy link
Contributor

This PR implements a new Slice distribution which uniformly samples items from a slice.

Fixes #1105

@Lucretiel
Copy link
Contributor Author

Looking through the Uniform tests as potential inspiration for unit tests here

@vks
Copy link
Collaborator

vks commented Mar 27, 2021

It seems the newly added tests don't compile.

@Lucretiel
Copy link
Contributor Author

Sorry about that, fixed

src/distributions/slice.rs Outdated Show resolved Hide resolved
- Replace Option<Self> with Result<Self, EmptySlice>
- Add a debug_assert to the unsafe part of the code
Copy link
Collaborator

@vks vks left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me!

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Please also add a note to the changelog.

src/distributions/slice.rs Show resolved Hide resolved
@vks vks added the D-changes Do: changes requested label Apr 30, 2021
src/distributions/slice.rs Outdated Show resolved Hide resolved
src/distributions/slice.rs Outdated Show resolved Hide resolved
@vks
Copy link
Collaborator

vks commented May 12, 2021

@kazcw Well spotted, thanks!

@vks vks merged commit 13738d3 into rust-random:master May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D-changes Do: changes requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slice sampling distribution
4 participants