search

rust-random / rand

A Rust library for random number generation.
https://crates.io/crates/rand
Other
1.67k stars 432 forks source link

Move KS tests #1519

Closed dhardy closed 4 days ago

dhardy commented 4 weeks ago

See here: we should move rand_distr/tests/cdf.rs to the benches crate or a new crate so that it can be run as a dedicated CI task.

Possibly we should (re)move the sparkline tests at the same time.

benjamin-lieser commented 4 weeks ago

Lets wait until https://github.com/rust-random/rand/issues/1394

dhardy commented 4 weeks ago

There's no reason to: the benches sub-crate will be moved too. CI is currently slowed by this. And we have several open PRs affecting rand_distr which is the main reason that hasn't been done already.

benjamin-lieser commented 4 weeks ago

Ok fair point. Do we want to keep our cdf implementations or do we want to wait until we can depend on statrs for this? We could do it already if we adjust the CI, because cargo can handle two different versions of rand_distr, but it might be more pain than it is worth. statrs, might also decide at one point to depend on rand_distr directly

dhardy commented 4 weeks ago

I'd prefer we don't depend on a non-release version of statrs (or anything outside of the repo). It's possible, but harder to manage (frequent updates, more likely to encounter bugs, more likely to encounter breaking changes, not human-readable versioning).

This is also a reason not to move rand_distr yet (wait for the next rand release).

benjamin-lieser commented 4 weeks ago

We can depend on the current release version. This "just" complicates the CI, because there will be two rand_distr packages and you have to specify which one to build doc, tests, etc ...

dhardy commented 4 weeks ago

The current release of statrs doesn't depend on rand_distr. Do you mean rand?

Yes, Cargo can handle that, and for a non-release crate like benches that's fine.

benjamin-lieser commented 4 weeks ago

It does indirectly through nalgebra

dhardy commented 4 weeks ago

Aha. It's a weird arrangement, but I don't think there's any real problem doing that.

benjamin-lieser commented 4 weeks ago

Ok. Than I would draft a PR, moving it and replacing the cdf implementations with statrs