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 #1525

Merged
merged 14 commits into from
Nov 18, 2024
Merged

Move ks #1525

merged 14 commits into from
Nov 18, 2024

Conversation

benjamin-lieser
Copy link
Collaborator

  • Added a CHANGELOG.md entry

Summary

Motivation

#1519

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.

Thanks.

I'm not sure about depending on spfunc; that appears not to have been touched in three years and has one open issue noting an inaccuracy.

distr_test/Cargo.toml Outdated Show resolved Hide resolved
@dhardy
Copy link
Member

dhardy commented Nov 14, 2024

Further note: could you split cdf.rs please? E.g.:

  • tests/ks/mod.rs provides the pub fns
  • tests/cdf.rs for most of the tests
  • tests/skew_normal.rs which is the only user of fn owen_t

(Optionally more segregation of tests.)

Part of the rationale is to make it easier to use this for additional tests — I'd like to add some for weighted sampling (for which we have several variants, so better to use a separate file).

@benjamin-lieser
Copy link
Collaborator Author

Thanks.

I'm not sure about depending on spfunc; that appears not to have been touched in three years and has one open issue noting an inaccuracy.

I see the point, but before we just copy pasted their zeta implementation

@benjamin-lieser benjamin-lieser marked this pull request as ready for review November 15, 2024 17:54
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.

Why did you include distr_test/Cargo.lock.msrv? I wasn't intending to run these tests on the MSRV, so this shouldn't be needed.

@benjamin-lieser
Copy link
Collaborator Author

It thought I put them with the other tests, so they get run on every supported platform. This would also run the MSRV tests and it seems simpler letting it pass these than try to avoid running them.

@dhardy
Copy link
Member

dhardy commented Nov 18, 2024

Aha. I don't think that's necessary — the value-stability tests are there to ensure that we get the same results everywhere.

@benjamin-lieser
Copy link
Collaborator Author

Good point.
I was also thinking if we should make the value stability tests more exhaustive by some automatic script to generate the tests (or reading values from file). Right now we only test a handful of values.

@dhardy
Copy link
Member

dhardy commented Nov 18, 2024

More exhaustive value-stability tests would be good, yes. It wouldn't surprise me if there are some FP issues we've missed somewhere. In fact, we probably have given that tests pass both with and without std_math.

@dhardy dhardy merged commit e85c923 into rust-random:master Nov 18, 2024
16 checks passed
@benjamin-lieser benjamin-lieser deleted the move_ks branch November 18, 2024 10:22
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.

2 participants