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

Move KS tests #1519

Open
dhardy opened this issue Oct 24, 2024 · 9 comments
Open

Move KS tests #1519

dhardy opened this issue Oct 24, 2024 · 9 comments

Comments

@dhardy
Copy link
Member

dhardy commented Oct 24, 2024

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
Copy link
Collaborator

Lets wait until #1394

@dhardy
Copy link
Member Author

dhardy commented Oct 25, 2024

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
Copy link
Collaborator

benjamin-lieser commented Oct 25, 2024

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
Copy link
Member Author

dhardy commented Oct 25, 2024

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
Copy link
Collaborator

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
Copy link
Member Author

dhardy commented Oct 25, 2024

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
Copy link
Collaborator

It does indirectly through nalgebra

@dhardy
Copy link
Member Author

dhardy commented Oct 25, 2024

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

@benjamin-lieser
Copy link
Collaborator

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

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

No branches or pull requests

2 participants