Skip to content

Document rationale for returning float types for a discrete distribution (Poisson) #34

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

Merged
merged 2 commits into from
Feb 21, 2021

Conversation

ndebuhr
Copy link
Contributor

@ndebuhr ndebuhr commented Feb 18, 2021

This PR expands the Poisson documentation:

  • Provide rationale for the Float return type
  • Notes concerns around lossy and panicking integer conversions
  • Provides an example conversion to u64

rust-random/rand#1093

…tion (Poisson), and provide an example conversion to get u64 values
…float-to-int conversions with "as" - detailing behavior in Rust <1.45 vs. Rust >=1.45
@vks
Copy link
Contributor

vks commented Feb 20, 2021

This is ready from my side, I'll merge it tomorrow so others have a chance to take a look.

@dhardy
Copy link
Member

dhardy commented Feb 21, 2021

Correct casting is an under-appreciated problem in Rust, but at least as gets this right (or "good enough") now. I'm actually considering releasing a new library for this (see this code) since none of the current ones do what I want.

But this change looks fine to me 👍

@vks vks merged commit efeaaae into rust-random:master Feb 21, 2021
@vks
Copy link
Contributor

vks commented Feb 21, 2021

@dhardy I have used the conv crate for this in the past, but it is no longer updated and has some issues.

@dhardy
Copy link
Member

dhardy commented Feb 22, 2021

@vks Also not the same thing, especially convenient unwrap-by-default syntax. If you need a lot of conversions and the choice is x as y or y::conv_from(x).unwrap(), which are you likely to reach for? There's also num_traits and cast; the latter does at least try for convenient syntax, though perhaps not clear enough syntax.

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