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

SkewNormal distribution implementation #1174

Merged
merged 8 commits into from
Sep 10, 2021
Merged

SkewNormal distribution implementation #1174

merged 8 commits into from
Sep 10, 2021

Conversation

saona-raimundo
Copy link
Contributor

Related to #1149.

Implements the Skew Normal distribution.

Most notably I had to add a dev-dependency to include distribution tests similar to #1121.
Adding the test was suggested by @dhardy.
There was a brief discussion about adding dev-dependencies for special functions with @vks in here. We talked about the crate spfunc, since it is a pure Rust implementation of special functions. It is still a very young crate and the function erf is still not there. Therefore, I chose to add statrs as it is a close companion of this crate, which is also pure Rust.

@saona-raimundo
Copy link
Contributor Author

@dhardy, can I ask for help with these tests?
Could it be a problem with the quote crate and its use of constant generics?

@vks
Copy link
Collaborator

vks commented Sep 6, 2021

The nightly failures are unrelated. The other failure seems to be in nalgebra, which I think is pulled in by statrs.

rand_distr/src/skew_normal.rs Outdated Show resolved Hide resolved
rand_distr/src/skew_normal.rs Outdated Show resolved Hide resolved
@vks
Copy link
Collaborator

vks commented Sep 7, 2021

I wonder whether special is a better choice than statrs in this case. It also provides the error function, but has less dependencies.

How does the sampling work for infinite location? It looks like it would just always return the infinite value.

}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably figure out how to deduplicate the code here, but this can be done in a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! Although I am not so sure how to do the code fully generic to test discrete distributions in the future.

For now, for these continuous distributions, I would put together the repetitive code in a function that takes the variables n_samples, min_x, max_x, dist, pdf and name (name of the distribution being tested for reporting it).

It would look like this:

#[test]
fn skew_normal() {
    const N_SAMPLES: u64 = 1_000_000;
    const LOCATION: f64 = 2.;
    const SCALE: f64 = 0.5;
    const SHAPE: f64 = -3.0;
    const MIN_X: f64 = -1.;
    const MAX_X: f64 = 4.;

    let dist = SkewNormal::new(LOCATION, SCALE, SHAPE).unwrap();
    fn pdf(x: f64) -> f64 { 
        // some code
    }
    histogram_test(N_SAMPLES, MIN_X, MAX_X, dist, pdf, "skew_normal");
}

where histogram_test takes the code repeated in both cases.

You see that the signature of histogram_test is not so clear for dist when testing discrete variables. And one would have to take into account the width of the bins of the histogram in that case too.

@saona-raimundo
Copy link
Contributor Author

How does the sampling work for infinite location? It looks like it would just always return the infinite value.

Yes, indeed! It will return infinite (which is consistent with Normal and the mathematical definition). I added tests for this case too.

@saona-raimundo
Copy link
Contributor Author

I wonder whether special is a better choice than statrs in this case. It also provides the error function, but has less dependencies.

I changed to special, but the Maybe nightly tests still fail.

@saona-raimundo
Copy link
Contributor Author

By the way, @vks, thank you for the review!! :D

Copy link
Collaborator

@vks vks left a comment

Choose a reason for hiding this comment

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

This looks good!

The nightly failures are unrelated (packed_simd_2 is broken on the current nightly, this happens from time to time).

I think this can be merged.

@vks vks merged commit cf00e37 into rust-random:master Sep 10, 2021
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