search

statrs-dev / statrs

Statistical computation library for Rust
https://docs.rs/statrs/latest/statrs/
MIT License
582 stars 83 forks source link

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

Closed RationalAsh closed 4 months ago

RationalAsh commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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().

RationalAsh commented 4 months ago

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!

YeungOnion commented 4 months ago

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 commented 4 months ago

Thanks for reviewing @FreezyLemon!

YeungOnion commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

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