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

Make distributions generic / impl for f32 #785

Merged
merged 4 commits into from
May 10, 2019
Merged

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Apr 27, 2019

Implement #100: impl for f32 and make distributions generic over the parameter type.

Status: partial type changes only. I haven't started on optimising the algorithms, but have found a few resources of interest.

So far I am liking the way this is going. A few extra type annotations are needed but not many due to the language's preference to resolve floats as f64 (example).

This adds a dependency on the num-traits crate, but only for rand_distr; I think this is acceptable.

Slight annoyance: NumCast::from returns an Option, thus requires .unwrap() (less concise and compiler cannot prove there is no panic). We could instead use std::comvert::From, but this would require an extra bound: N: From<f32>.

@dhardy
Copy link
Member Author

dhardy commented Apr 27, 2019

Note that initial attempts to make Binomial generic over its integer type proved less straightforward. Possibly this one will not be made generic (I see less incentive, and even if it were, the probability type would likely stay f64 only).

@dhardy
Copy link
Member Author

dhardy commented May 3, 2019

@vks would you be able to review?

@vks
Copy link
Collaborator

vks commented May 3, 2019

I wonder whether it would make sense to make the parameter an associated type of Distribution, similar to the iterator trait. (In fact, distributions could implement Iterator.)

let x: f64 = self.sample(rng);
x as f32
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the motivation of this? To exercise the type change and optimize later? As of now, it does not seem worth to add this, when dist.sample(&mut rng) as f32 is equivalent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, an optimised impl can follow later. Even like this though it helps with some generic coding.

@dhardy
Copy link
Member Author

dhardy commented May 3, 2019

Remember, the (unparameterised) Standard distribution implements Distribution<u32>, Distribution<f64>, and many more types. To make the type an associated type, we would have to add type parameters to Standard instead. This would be a major breaking change to the main Rand lib, and I think probably less ergonomic to use (would let x: i8 = Standard.sample(&mut rng) work without an internal type parameter?).

@vks
Copy link
Collaborator

vks commented May 10, 2019

That is a good point, it would make using unparametrized distributions much more cumbersome, so the current approach is probably best for now.

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.

Looks good! I think this is the way we want the interface to look like.

@dhardy dhardy merged commit b664e64 into rust-random:master May 10, 2019
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.

2 participants