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

Update uniform.rs #251

Merged
merged 3 commits into from
Aug 4, 2024
Merged

Update uniform.rs #251

merged 3 commits into from
Aug 4, 2024

Conversation

avhz
Copy link
Contributor

@avhz avhz commented Jul 20, 2024

Add a standard() method to Uniform Distribution, following the same idea as the Normal::standard() method.

If you're happy with this change I can also add the same thing for other distributions that have an accepted 'standard' form.

Add a `standard()` method to Uniform Distribution.
@YeungOnion
Copy link
Contributor

In #228 we also implemented the Default trait since that would contribute to API, i.e. adds functionality without newtype, so I'd like this to also impl Default if we have a standard or unit construction method and if you added those trivial tests, I'd merge that.

After all, if most users had to write a new_standard_normal()->Normal and new_standard_uniform()->Uniform I don't think that would break their development experience.


So, wanting to ensure Default behaves expectedly (since most distributions don't have a T::new()->T), I am certain Normal and Uniform are unambiguous in their choice, I am less certain for others.

For example, Exponential distribution is parametrized by a single scale parameter, but I've not heard someone use the term, "standard exponential distribution"/"unit exponential distribution" even if I'd infer it to be $\lambda=1$, some arguments could be made for other distributions as well.

I suppose time and feedback will make this clear if other distributions have this, the author of #228 did not think so.

@avhz
Copy link
Contributor Author

avhz commented Aug 4, 2024

I've included Default and some basic unit tests as requested :)

@YeungOnion
Copy link
Contributor

Looks good. Thanks for the contribution; hope you continue to provide feedback!

@YeungOnion YeungOnion merged commit 53fc9d9 into statrs-dev:master Aug 4, 2024
5 checks passed
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