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

Cdf testing with Kolmogorov Smirnov #1494

Merged
merged 14 commits into from
Oct 8, 2024

Conversation

benjamin-lieser
Copy link
Collaborator

@benjamin-lieser benjamin-lieser commented Sep 17, 2024

  • Added a CHANGELOG.md entry

Summary

As addition and alternative to pdf testing, I implemented distribution testing based on cdf

Motivation

The cdf tests should be more robust and avoid having the bin-width choice problem

Details

I have two simple example tests for normal and binomial. If we like these tests we should also add some way to test on different parameter sets and the other distributions.

This can't be easily generalized to multivariate ones.

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

There are a couple of other Rust crates for KS stat/test, but on quick examination they may not be any use to us.

rand_distr/tests/cdf.rs Outdated Show resolved Hide resolved
rand_distr/tests/cdf.rs Outdated Show resolved Hide resolved
rand_distr/tests/cdf.rs Outdated Show resolved Hide resolved
rand_distr/tests/cdf.rs Outdated Show resolved Hide resolved
@benjamin-lieser
Copy link
Collaborator Author

benjamin-lieser commented Sep 30, 2024

I am quite happy with the code. After a review I would add the missing tests.

We might have to wait until a release of statrs which can be configured to not depend on rand_distr. Or we get the cdf implementations from elsewhere or have our own.
It is only a dev-dep, so the current solution (installing it from their master branch) might be fine

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Could you please either rebase on master or just to drop the merge commit? I don't believe anything related was changed, so there should be no need to merge master into this PR.

I like the new approach using multiple seeds and parameter sets. Lets not add additional tests into this PR though; a new PR would be better for that (I mean, you can implement it all and push to a new PR immediately if you like; it would just need rebasing and possibly adjusting later).

rand_distr/Cargo.toml Outdated Show resolved Hide resolved
@benjamin-lieser benjamin-lieser force-pushed the cdf_testing branch 2 times, most recently from 2f2ae89 to f7f029c Compare October 3, 2024 14:41
Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

In progress...

rand_distr/tests/cdf.rs Outdated Show resolved Hide resolved
rand_distr/tests/cdf.rs Outdated Show resolved Hide resolved
rand_distr/tests/cdf.rs Outdated Show resolved Hide resolved
@benjamin-lieser
Copy link
Collaborator Author

I got an interesting test failure on macos:

---- distr::bernoulli::test::bernoulli_distributions_can_be_compared stdout ----
thread 'distr::bernoulli::test::bernoulli_distributions_can_be_compared' panicked at src/distr/bernoulli.rs:229:9:
assertion `left == right` failed
  left: Err(InvalidProbability)
 right: Ok(Bernoulli { p_int: 18446744073709551615 })
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Could be a compiler bug, maybe?

Maybe someone with a mac can reproduce this?

@dhardy
Copy link
Member

dhardy commented Oct 7, 2024

It passed when re-run.

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Looks good; just some nits below

rand_distr/tests/cdf.rs Show resolved Hide resolved
rand_distr/tests/cdf.rs Outdated Show resolved Hide resolved
rand_distr/tests/cdf.rs Outdated Show resolved Hide resolved
@dhardy dhardy merged commit 9c2787d into rust-random:master Oct 8, 2024
14 checks passed
@benjamin-lieser benjamin-lieser deleted the cdf_testing branch October 8, 2024 11:02
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.

4 participants