-
Notifications
You must be signed in to change notification settings - Fork 86
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
Add a way to easily get the standard normal distribution. #228
Conversation
I've wondered if this kind of thing would be useful and I thought to use Default, but I was unsure if it would be obvious for any other distributions, I thought perhaps the exponential and uniform would be guessable. The motive for default if useful for other distributions would be that the name is reusable. |
My typical experience of working with these distributions is that many (most?) distributions don't really do "standard" as much as the Normal Distribution. So it would be a property that is unique to the normal distribution. Personally, I would add it as something that is specific only to the normal distribution. But I do agree with you that the Default trait is more elegant as a solution. So if that's what we should go with, then I can open an updated PR. |
I would argue that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added two formatting nitpicks. CI does not enforce formatting rules yet, but it's planned (see #215) and following them now will avoid having to fix them later
src/distribution/normal.rs
Outdated
@@ -51,6 +51,26 @@ impl Normal { | |||
Ok(Normal { mean, std_dev }) | |||
} | |||
} | |||
|
|||
/// Constructs a new standard normal distribution with a mean of 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Constructs a new standard normal distribution with a mean of 0 | |
/// Constructs a new standard normal distribution with a mean of 0 |
src/distribution/normal.rs
Outdated
let mean: f64 = 0.0; | ||
let std_dev: f64 = 1.0; | ||
Normal { | ||
mean, | ||
std_dev | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let mean: f64 = 0.0; | |
let std_dev: f64 = 1.0; | |
Normal { | |
mean, | |
std_dev | |
} | |
Normal { | |
mean: 0.0, | |
std_dev: 1.0, | |
} |
Thanks! I've added in these changes in addition to implementing the default trait for the Normal distribution. Also added a test case. for it. Seems OK. Let me know if any further changes are needed! |
src/distribution/normal.rs
Outdated
n_mean = n.mean().unwrap(); | ||
n_std = n.std_dev().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two lines need a let
:
n_mean = n.mean().unwrap(); | |
n_std = n.std_dev().unwrap(); | |
let n_mean = n.mean().unwrap(); | |
let n_std = n.std_dev().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops not sure how I missed this one (cargo test ran??). Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you scroll up, you'll see that the entire tests module is only run when the nightly
feature flag is enabled (which requires a nightly compiler). So you'd need to run cargo +nightly test -F nightly
, which is easy to miss
Whoops, didn't realize I left this as a draft. Cool with keeping I wholeheartedly agree that Now that I've considered more on what I suggested, it may only add value if there's a use case for needing |
Thanks for reviewing @FreezyLemon! |
@RationalAsh Since you didn't notice the testing issue, do you have any feedback on making that more apparent that you'll need to run on nightly compiler with We have it in the Depending on workflow and context of edits, I'll target unit tests in specific module passing the module to cargo +nightly test -F nightly -- distribution::normal # tests in the module you contributed to in this PR |
The way I typically do it for my own projects that need non-standard testing / compile commands is to have a test.sh script that contains the commands needed to run all tests locally. Also, this might be a really dumb question because I didn't go to deep into rust / statrs internals -- Is there a need for the tests to run using the nightly compiler? Or in other words - is it possible to refactor so that most tests do not require the nightly compiler to run and instead use the latest stable version? |
Was hoping this exact question would be asked without my mentioning it. I don't see a need for it beyond how the test suite is written, but we'd have to change tests that use the I'll make an issue for it so we can merge and close this PR. Aside: I think the possibility of a |
I've been using statrs and I love the project. I find myself creating the standard normal distribution frequently. So I thought it might be convenient to add a way to get the standard normal in one function call rather than unwrapping. Hope this is useful for others too.