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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Move ks #1525

wants to merge 12 commits into from

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