Skip to content

Implement Distribution<u64> support for Zeta, Zipf #1516

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

dhardy
Copy link
Member

@dhardy dhardy commented Oct 21, 2024

  • Added a CHANGELOG.md entry

Closes #1323. I would have added usize and possibly u32 support, but @vks preferred not to, and this is reasonable.

These impls use @JamboChen's tests.

@dhardy dhardy requested review from vks and benjamin-lieser October 21, 2024 09:39
Copy link
Member

@benjamin-lieser benjamin-lieser left a comment

Choose a reason for hiding this comment

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

Looks good.

Having worked a bit more with the two implementation of Poisson and seeing the troubles it caused, I feel like we should not add them and remove them for Possion. For example having an impl Distribution<T> as arg in rust 1.61 where T cannot be specified or the arguable lengthy <Poisson<f64> as Distribution<f64>>::sample(self, rng)

We can just have a doc, where we explicitly say that is is safe to do as u64 or even smaller int types when the user does not care about saturation or is sure it cannot happen.

@dhardy
Copy link
Member Author

dhardy commented Oct 21, 2024

It is unfortunate that there is no simple notation for type-ascribing in this case, aside from let x: f64 = distr.sample(rng);. (There were some unsuccessful RFCs.) Distribution::<f64>::sample(distr, rng) should be enough, but it's still unwieldy and not very intuitive.

To some extent this was baked into the original design: the result is a type parameter of the trait Distribution<T> not an associated type. Yet, other than Standard, this ability to support multiple output types hasn't been used much.

I feel that supporting only floats + u64 is the worst approach: it's enough target types to prevent inference in many cases, yet still doesn't support all potentially desired types (e.g. u32, usize).

@benjamin-lieser
Copy link
Member

I feel that supporting only floats + u64 is the worst approach: it's enough target types to prevent inference in many cases, yet still doesn't support all potentially desired types (e.g. u32, usize).

I feel a silent saturation is often a serious bug and I don't want to make it too easy. For Zipf I would be ok, because the upper bound is part of the parameters, but for Zeta and Poisson I would not implement the smaller intergers. And I agree, the current approach seems to be the worst of both worlds, because getting the u64 impl does not do too much.

@dhardy dhardy mentioned this pull request Oct 24, 2024
1 task
@dhardy
Copy link
Member Author

dhardy commented Oct 24, 2024

Replaced by #1517

@dhardy dhardy closed this Oct 24, 2024
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.

It is not reasonable for rand_distr::Zipf to return floating-point values
2 participants