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

Add a way to easily get the standard normal distribution. #228

Merged
merged 6 commits into from
May 25, 2024

Conversation

RationalAsh
Copy link
Contributor

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.

@YeungOnion
Copy link
Contributor

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.

@RationalAsh
Copy link
Contributor Author

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.

@FreezyLemon
Copy link
Contributor

I would argue that Normal::standard() is more obvious at first glance than Normal::default(). That said, you can also just implement both and have Normal::default() return the result of Normal::standard().

Copy link
Contributor

@FreezyLemon FreezyLemon left a 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

@@ -51,6 +51,26 @@ impl Normal {
Ok(Normal { mean, std_dev })
}
}

/// Constructs a new standard normal distribution with a mean of 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Constructs a new standard normal distribution with a mean of 0
/// Constructs a new standard normal distribution with a mean of 0

Comment on lines 67 to 72
let mean: f64 = 0.0;
let std_dev: f64 = 1.0;
Normal {
mean,
std_dev
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let mean: f64 = 0.0;
let std_dev: f64 = 1.0;
Normal {
mean,
std_dev
}
Normal {
mean: 0.0,
std_dev: 1.0,
}

@RationalAsh
Copy link
Contributor Author

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

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!

Comment on lines 543 to 544
n_mean = n.mean().unwrap();
n_std = n.std_dev().unwrap();
Copy link
Contributor

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:

Suggested change
n_mean = n.mean().unwrap();
n_std = n.std_dev().unwrap();
let n_mean = n.mean().unwrap();
let n_std = n.std_dev().unwrap();

Copy link
Contributor Author

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.

Copy link
Contributor

@FreezyLemon FreezyLemon May 24, 2024

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

@YeungOnion
Copy link
Contributor

Whoops, didn't realize I left this as a draft. Cool with keeping impl Default.


I wholeheartedly agree that Normal::standard() likely only means one thing.

Now that I've considered more on what I suggested, it may only add value if there's a use case for needing Default as a trait bound.

@YeungOnion
Copy link
Contributor

Thanks for reviewing @FreezyLemon!

@YeungOnion
Copy link
Contributor

@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 -Fnightly?

We have it in the Contributing section of the README, but it's not obvious if something you add isn't run unless you pay close attention to the number of tests and how it changes as you add them and I'm wondering if you've got suggestions?

Depending on workflow and context of edits, I'll target unit tests in specific module passing the module to libtest like so

cargo +nightly test -F nightly -- distribution::normal # tests in the module you contributed to in this PR

@RationalAsh
Copy link
Contributor Author

@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 -Fnightly?

We have it in the Contributing section of the README, but it's not obvious if something you add isn't run unless you pay close attention to the number of tests and how it changes as you add them and I'm wondering if you've got suggestions?

Depending on workflow and context of edits, I'll target unit tests in specific module passing the module to libtest like so

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?

@YeungOnion
Copy link
Contributor

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 testing_boiler macro or find a way to write the macro differently. Personally, I don't like the way testing_boiler is used as failed assertions don't clearly provide the line where the test is written.

I'll make an issue for it so we can merge and close this PR.

Aside: I think the possibility of a nightly feature is useful for things we're figuring out. I'm expecting the multivariate API will start as nightly for the crate, but not rustc

@YeungOnion YeungOnion merged commit 358aafb into statrs-dev:master May 25, 2024
2 checks passed
@YeungOnion YeungOnion mentioned this pull request Jul 26, 2024
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.

3 participants