Skip to content

Implement UniformSampler for Wrapping<T> #467

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

Closed
wants to merge 1 commit into from

Conversation

pitdicker
Copy link
Contributor

This is the commit from #436 that we delayed until after the 0.5 release.

@pitdicker
Copy link
Contributor Author

r @vks?

@dhardy
Copy link
Member

dhardy commented May 24, 2018

You really like writing code, don't you? 😄 I just wonder whether this will actually be useful to many people, same as how you didn't like the updated rand_derive thing.

@vks
Copy link
Collaborator

vks commented May 24, 2018

What is the use case of this? I can't really think of a reason why you want want to sample from a wrapped range.

@dhardy
Copy link
Member

dhardy commented May 26, 2018

Since users don't normally need to look at the impl docs when using Uniform, it's not even obvious that this handles low > high differently. Often for users it will be enough to just sample integers, then "wrap" afterwards (if there is actually any reason to want to sample wrapping integers from a restricted range). So unless there is a really good use-case for this code I think we should just forget it, sorry.

@pitdicker
Copy link
Contributor Author

What is the use case of this?

I guess I got carried away a bit when implementing Standard for Wrapping<T> and though implementing Uniform would be nice to.

For a wrapping type I think it makes sense to be able to sample from a range that wraps around, but no, I can't see many uses. In a way it would be "sample from the entire range of the type, except between [high, low)".

@pitdicker pitdicker closed this May 28, 2018
@pitdicker pitdicker deleted the uniform_wrapping branch July 20, 2018 16:34
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.

3 participants