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

Generic distributions: use custom trait #793

Merged
merged 9 commits into from
May 15, 2019
Merged

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented May 10, 2019

Continuing from #785, this replaces num_traits::Float with a custom version, then also makes Cauchy generic.

Closes #100.

Thoughts (@vks)? The implementation of Float in the first commit doesn't look bad, but we see in the second it is growing quickly.

One motivation is to avoid a dependency on an unstable lib (there appear to be suggestions the trait should change). Another is to avoid requiring .unwrap() on N::from(..), although as implemented here we use as which isn't exactly safe (we could instead restrict input precision and use From<f32>).

///
/// The bounds and methods are based purely on internal
/// requirements, and will change as needed.
pub trait Float: Copy + Sized + cmp::PartialOrd
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe pub(crate) is more appropriate?

Copy link
Member Author

Choose a reason for hiding this comment

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

Then it would be a private trait used in a public API — however, the compiler doesn't complain.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I do get a compiler error if marking this pub(crate), though not if it is a pub item within a private module which is not re-exported (I guess this must be a compiler bug and don't know that we should exploit this).

error[E0445]: crate-visible trait utils::Float in public interface

There are discussions on roughly this here and here, but I'm not sure exactly where this got to.

@vks
Copy link
Collaborator

vks commented May 10, 2019

I think using a custom trait is fine, but we should not make it public.

@dhardy
Copy link
Member Author

dhardy commented May 11, 2019

Aside from the technicalities of whether hiding the Float trait is legal in Rust, I am of the opinion that it is preferable to expose this trait anyway, though with a warning that its specification can be expected to change more often than most parts of the API (making custom implementations liable to break often, but not other usage).

Writing generic code is quite common in Rust, and if we do so using an internal trait as a bound it makes writing generic code over FP types significantly more difficult for users.

@dhardy
Copy link
Member Author

dhardy commented May 13, 2019

I have completed the type transitions for most distributions, with two exceptions: Binomial and Poisson. Both involve integer and float types, while the latter also uses the log_gamma function. I'm not quite sure how best to handle this; since optimal f32 and f64 implementations of the log_gamma function would differ a trait would be best. This is another piece of functionality which could be exposed as a useful building block (here one can compare statrs, which provides this and many more functions we don't need).

These are required as a side-effect of the previous generalisation.
@dhardy
Copy link
Member Author

dhardy commented May 14, 2019

For some reason I missed some obvious compile errors in the previous push. Fixed, but the new requirements on type specification for UnitCircle and UnitSphereSurface are a bit annoying to use; is there a good alternative?

@dhardy
Copy link
Member Author

dhardy commented May 14, 2019

Regarding the previous question: we could add a PhantomData field usable to specify the type T of Distribution<T>, but it wouldn't make UnitCircle etc. more ergonomic to use (we'd need a new() fn too). It is already the case that the output type of some distributions, e.g. Standard, is easiest specified via a typed let binding, so this is nothing new.

@dhardy
Copy link
Member Author

dhardy commented May 14, 2019

Regarding the Binomial distribution: it could be generalised with respect to its integer type, but has some quite specific requirements, perhaps best specified by a specific trait. I see little point to this however; it uses f64 arithmetic internally (and should use this type even with u32 parameter) so there would be little performance advantage, while from the other perspective using such a generalisation to handle a u128 parameter would not give an accurate result. Resolution: I think this is best left as-is (without any generalisation).

The Poisson distribution is a different matter; input and all workings are currently f64 and could easily generalise to f32 (though, as mentioned before, the log_gamma function would also need generalising). The output, however, is currently cast to u64, and I don't think for any other reason than that the output is a non-negative integer (the current implementation is buggy since it casts an f64 without upper bound to u64 via the as operator, which is undefined behaviour for out-of-range input). Resolution: we should probably switch the output of Poisson to f64 (or generalise and F: Float), then possibly also implement Distribution<u64> casting the above and using an assert or returning a Result<u64>.

I suggest we make a new PR for the second change since it's a new topic and the rest of this is ready. (I also want to leave the optimal f32 Ziggurat implementations until later; it's related to #257).

@dhardy dhardy mentioned this pull request May 15, 2019
22 tasks
@vks
Copy link
Collaborator

vks commented May 15, 2019

the new requirements on type specification for UnitCircle and UnitSphereSurface are a bit annoying to use; is there a good alternative?

I think this is the best we can do, unless we get default type parameters in Rust.

@vks
Copy link
Collaborator

vks commented May 15, 2019

There is a problem with the binomial distribution in rand_distr: It still uses the old code, not the one added in #740, which only made it to the deprecated rand implementation.

I agree that it does not make sense to make the binomial distribution generic, given we don't plan to use a different algorithm for f32.

@vks
Copy link
Collaborator

vks commented May 15, 2019

we should probably switch the output of Poisson to f64

The advantage of returning u64 is that it guarantees that the result is positive and integer, so users don't have to verify this. However, I agree that we are currently not doing this (by checking that the result is positive and smaller than u64::MAX) and possibly returning incorrect results. However, I agree it is probably best to have both f64 and u64 as a result.

@dhardy
Copy link
Member Author

dhardy commented May 15, 2019

There is a problem with the binomial distribution in rand_distr: It still uses the old code, not the one added in #740, which only made it to the deprecated rand implementation.

Whoops! Perhaps you can make a PR after this is merged.

The advantage of returning u64 is that it guarantees that the result is positive and integer,

Except that since the algorithm does not enforce the upper bound, this is a false promise. Probably adding an assert would cover the vast majority of uses, but it would be possible to break by using extremely large rate λ. So lets try doing both in a new PR.

Time to merge this one!

@dhardy dhardy merged commit 0f1b1ff into rust-random:master May 15, 2019
@vks
Copy link
Collaborator

vks commented May 15, 2019

Whoops! Perhaps you can make a PR after this is merged.

I opened #794.

@dhardy dhardy deleted the distr branch May 16, 2019 11:27
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.

Implement distributions for f32
2 participants