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

Adds rand_distr::Distribution implementations for f16 & bf16 #83

Merged
merged 7 commits into from
Apr 26, 2023

Conversation

coreylowman
Copy link
Contributor

Resolves #82

Tests need to be run with the sample and num-traits feature enabled

@coreylowman
Copy link
Contributor Author

Happy to make any modifications you'd like to any names!

@starkat99
Copy link
Owner

I'll happily merge this with the following changes:

  • Could you rename the feature to simply rand_distr to match the name of the optional dependency. Since it's just implementing traits from the crate and not adding any additional functionality, this makes the feature the most clear, matching the other optional crate trait implementations.
  • Could you add the new rand_distr feature to CARGO_MAKE_CARGO_ALL_FEATURES in Makefile.toml so the tests get run in CI? That way the tests can run before I merge.
  • I didn't see anything relying on num-traits in the tests, but if they do rely on that dependency like you said, could you put a #[cfg] on the tests so they don't break compiles if tests are run with rand_distr but without num-traits?
  • Could you also add a brief note in the Readme and crate docs (in lib.rs) documenting the optional cargo feature? There's a section in both docs for optional cargo features. Saves me from having to write it, especially since I've never used rand_distr before.

That's it, just minor things other than the rename.

@starkat99
Copy link
Owner

starkat99 commented Mar 28, 2023

Oh also, it doesn't look like rand crate is actually used outside of tests, so it doesn't need to be a regular dependency, just a dev-dependency. And add the feature to the [package.metadata.docs.rs] section so trait implementations will appear on docs.rs

@coreylowman
Copy link
Contributor Author

Perfect, will make those updates soon! Thanks for the response 🚀

@coreylowman
Copy link
Contributor Author

Also for completeness, here's the errors you'll get if you run the tests without num-traits:

error[E0277]: the trait bound `binary16::f16: Float` is not satisfied
   --> src\rand_distr.rs:103:13
    |
102 |         let _: crate::f16 = rng.sample(
    |                                 ------ required by a bound introduced by this call
103 |             rand_distr::Normal::new(crate::f16::from_f32(0.0), crate::f16::from_f32(1.0)).unwrap(),
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Float` is not implemented for `binary16::f16`
    |
    = help: the following other types implement trait `Float`:
              f32
              f64
    = note: required for `rand_distr::Normal<binary16::f16>` to implement `Distribution<binary16::f16>`
note: required by a bound in `rand::Rng::sample`
   --> C:\Users\clowm\.cargo\registry\src\github.com-1ecc6299db9ec823\rand-0.8.5\src\rng.rs:152:21
    |
152 |     fn sample<T, D: Distribution<T>>(&mut self, distr: D) -> T {
    |                     ^^^^^^^^^^^^^^^ required by this bound in `rand::Rng::sample

Comment on lines 1 to 7
use rand::Rng;
use rand_distr::{uniform::UniformFloat, Distribution};

macro_rules! impl_distribution_via_f32 {
($Ty:ty, $Distr:ty) => {
impl Distribution<$Ty> for $Distr {
fn sample<R: Rng + ?Sized>(&self, rng: &mut R) -> $Ty {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rand::Rng trait is used in this impl Distribution here. Unfortunately rand_distr doesn't re-export Rng, so thats the only use of it.

This would mean users would always have to use --features rand,rand_distr together. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just kidding I think I figured out how to do this by adding an actual feature named rand_distr that requires both rand & rand_distr

@coreylowman
Copy link
Contributor Author

Okay should be all good for re-review!

src/lib.rs Outdated Show resolved Hide resolved
@coreylowman
Copy link
Contributor Author

@starkat99 anything else I need to update?

@starkat99 starkat99 merged commit 1d7f862 into starkat99:main Apr 26, 2023
9 checks passed
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 rand_distr::SampleUniform for f16
2 participants