Correctly document rand_distr features and forward "std" feature to num-traits

[vks]

Previously, the features were undocumented and it was incorrectly claimed that no_std is not supported.

[josephlr]

Isn't this not true right now? We don't seem to forward the std trait to num-traits, so would we even end up using the std implementations?

[josephlr]

ya we need to add "rand-traits/std" to this line:

rand/rand_distr/Cargo.toml

Line 24 in 3a03c9e

std = ["alloc", "rand/std"]

[dhardy]

You mean "num-traits/std"?

This actually brings up another question though: should we prefer the std impls? This might affect portability of results.

[josephlr]

You mean "num-traits/std"

Whoops, ya

This actually brings up another question though: should we prefer the std impls?

I feel like we should, if only because that's what num-traits does. Also, preffering std over libm is what I would expect. But I don't have that strong a feeling either way.

[dhardy]

I feel like we should

Maybe; I don't have a strong opinion on it, though we should probably mention it in the book and maybe also try to gather data on where results may differ.

[vks]

Fair enough, I decided to forward our "std" feature as "num-traits/std", since this is the behavior I would expect.

Of course, this might technically be a value-breaking change. If we really want to have value stability for floating-point distributions, we might want to always use libm, because I am not sure that all implementations of the C standard math libraries produce the same results.

[dhardy]

Another point: enabling std in rand does not enable std in rand_distr. Perhaps we don't want this though; in any case, we can't without rust-lang/cargo#8832.

[dhardy]

You mean "num-traits/std"?

This actually brings up another question though: should we prefer the std impls? This might affect portability of results.

[vks]

Another point: enabling std in rand does not enable std in rand_distr. Perhaps we don't want this though; in any case, we can't without rust-lang/cargo#8832.

rand_distr depends on rand, not the other way around. So I think this is as it should be?

[vks]

Are the changes here ok or should I forgo forwarding "std" for now?

[dhardy]

Fair enough, I decided to forward our "std" feature as "num-traits/std", since this is the behavior I would expect.

Of course, this might technically be a value-breaking change. If we really want to have value stability for floating-point distributions, we might want to always use libm, because I am not sure that all implementations of the C standard math libraries produce the same results.

Yes, I'd consider this a breaking change and not for a patch version. Also, regarding the latter point, better value portability sounds desirable. Is there any real cost to using libm — e.g. performance or incompatibility? Because if not, is it better to use it?

For now maybe we should add a different flag like std_math, and if the next release will be a patch release it should not be on by default.

[vks]

Ok, I reverted the change to use std float functions to keep the status quo and updated the README accordingly.

Is there any real cost to using libm — e.g. performance or incompatibility?

There are reports on their issue tracker about libm being twice as slow on some platforms. More worryingly, there are reports about incorrect results due to bugs in the floor implementation on x87 platforms. There are also some accuracy bugs.

For now maybe we should add a different flag like std_math, and if the next release will be a patch release it should not be on by default.

I'm not sure the differences in results would be big enough to justify a feature flag, but let's keep that for another PR.

[dhardy]

I still don't see a reason not to add std_math = ["num_traits/std"] — except maybe supporting it in the future.

[vks]

I added an "std_math" feature and made sure our CI covers the different rand_distr features.

[vks]

@dhardy I think I addressed all of your concerns?

[dhardy]

Yes, sorry. Approved.