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

Allow lambda zero for parametrization in Exp distribution. #972

Merged
merged 3 commits into from
Jul 3, 2020
Merged

Allow lambda zero for parametrization in Exp distribution. #972

merged 3 commits into from
Jul 3, 2020

Conversation

saona-raimundo
Copy link
Contributor

This allows the parameter lambda = 0 in the distribution Exp.

Implementation

Taking advantage that 1 / 0 = infinity, we change nothing in the implementation, i.e. a sample from Exp(lambda) is the result of a sample from Exp1 multiplied by 1 / lambda.

Performance

This makes the sampling from Exp(lambda = 0) lose time sampling from Exp1 when there is no need since the result is always infinity. This is okay since it is not the common case.

Documentation

The error LambdaTooSmall accounts now only for negative or NaN parameters lambda.
Also, In the documentation for Exp, there are two changes: the explanation of density and constructor new. Please check the style of this.

In the explanation of the density, we included the case lambda = 0, assuming that the user is using primitives (most of the cases).

In the constructor new, a remark was added. It seems a bit overly complicated, but this is because of the trait N: Float. Since Float has no infinity method, we rely on the behaviour of 1 / 0 of the custom type N. I made this explicit by explaining the implementation itself! I did not come up with a better explanation.

@saona-raimundo saona-raimundo mentioned this pull request May 1, 2020
Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Looks good other than that little nit.

rand_distr/src/exponential.rs Outdated Show resolved Hide resolved
@dhardy
Copy link
Member

dhardy commented May 2, 2020

Meanwhile: statrs-dev/statrs#102

This particular degenerate case (exponential with rate 0) is a clear extension of the regular model.

Updated description of LambdaTooSmall error.
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.

Looks good!

@dhardy
Copy link
Member

dhardy commented Jul 3, 2020

Well, lets merge this at least: this particular degenerate case is not problematic (provided one accepts that one cannot necessarily integrate over an infinite domain and get the expected result).

@dhardy dhardy merged commit c9f485a into rust-random:master Jul 3, 2020
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