Migrate rand_distr to num-traits for no_std support

[newpavlov]

Closes: #922

If I am not mistaken, reproducibility tests on x86-64 pass without issues.

[dhardy]

Well spotted that since rand_distr is now an external crate, there's less pressure to minimise dependencies. But even so (and at risk of being accused of NIH), is there a good rationale for this change?

Aha, because num_traits provides trig functions not dependent on std / libm.

[dhardy]

Looks like we'll need to bump the MSRV to 1.36; that's nearly a year old so probably fine. The latest Debian release only appears to support 1.34 however, so not so sure. But in any case the issue is only that the alloc crate was stabilised in 1.36 so we could simply not test that configuration in the MSRV CI config (and maybe add another config for 1.36+alloc).

Not sure what the other failure is. Lets retry later.

[newpavlov]

Looks like we'll need to bump the MSRV to 1.36

We could use the #[cfg(all(feature = "alloc", not(feature = "std")))] workaround, but personally I would prefer to bump MSRV, same goes for rand v0.8.

Probably we should synchronize rand_distr v0.3 and rand v0.8?

[vks]

This change looks good, especially the no_std support is something I wanted for a long time. I would like to see the potential undefined behavior from casting f64 to u64 resolved, with or without bringing back the u64 impl for Poisson.

[dhardy]

Regarding the MSRV, couldn't we simply change our testing to say the MSRV is 1.34 in general but 1.36 for the alloc feature? But if it gets complicated, then just bump to 1.36, agreed.

[newpavlov]

I've used the following workaround to get the old MSRV:

#[cfg(all(feature = "alloc", not(feature = "std")))]
extern crate alloc;
#[cfg(feature = "std")]
extern crate std;
// TODO: remove on MSRV bump to 1.36
#[cfg(feature = "std")]
extern crate std as alloc;

Also note that I've tweaked Dirichlet API a bit, I can revert the commit if you don't like the changes.

[vks]

Also note that I've tweaked Dirichlet API a bit, I can revert the commit if you don't like the changes.

What's the rationale for this change?

[newpavlov]

Boxed slice is smaller than vector and all examples effectively use arrays for initialization (unfortunately I couldn't find real-world uses after a cursory github search), so I think it makes API simpler. Yes, it may result in an unnecessary allocation, but I doubt it will be common in practice.

Ideally it should use const generics and I think slice is closer to that than vector, so it could save some amount of churn during potential future migration.

[vks]

I just realized that this PR conflicts with another open PR, #958, which introduces support for usize for Poisson.

Boxed slice is smaller than vector and all examples effectively use arrays for initialization (unfortunately I couldn't find real-world uses after a cursory github search), so I think it makes API simpler. Yes, it may result in an unnecessary allocation, but I doubt it will be common in practice.

Fair enough!

Ideally it should use const generics and I think slice is closer to that than vector, so it could save some amount of churn during potential future migration.

I don't see how this would help with churn, switching to const generics will be a breaking change anyway.

[vks]

For the record, undefined behavior in float-to-int casts will be fixed with Rust 1.45.

[newpavlov]

I just realized that this PR conflicts with another open PR, #958, which introduces support for usize for Poisson.

I think my earlier argument still stands, so I think we should not merge it.

[vks]

This looks good to go to me (with the minor documentation change I suggested).

@dhardy Any concerns?

[dhardy]

For the most part this looks good.

Do you intend to squash or rebase this? It does several different things (such as moving value stability tests and the num-traits migration) which is quite a lot for one commit, but it is not desirable to keep the little fixes separate.

[newpavlov]

Do you intend to squash or rebase this?

Personally, I prefer squashing PRs.

[vks]

I believe this can be merged after squashing and rebasing?

[dhardy]

Yes. Does it need a rebase or just a squash? (I still think it's too much content for one commit, but nvm.)

[newpavlov]

I think simply pressing the "Squash and merge" button should be enough.