Add getters for distribution parameters, closes #1233 #1234
Conversation
| // 1 / mu^2 = alpha^2 - beta^2 | ||
| // 1 / mu^2 + beta^2 = alpha^2 | ||
| // sqrt(1 / mu^2 + beta^2) = alpha with alpha > 0 | ||
| F::sqrt(self.inverse_gaussian.mean().powi(2).recip() + self.beta.powi(2)) |
There was a problem hiding this comment.
I think this computation may produce slightly different alpha than one used for distribution initialization. Maybe it's worth to add a comment in the docs about it? Same for a number of other getters.
There was a problem hiding this comment.
Indeed, I've commented in #1233 about the issue, I've proposed a set of alternatives, that's why this PR is just a draft.
GitHub shows me that it is already linked to the original issue, but I'll edit the original comment and add it. |
| /// Returns the (inverse of the) shape parameter (`lambda`) of the distribution. | ||
| pub fn lambda_inverse(&self) -> F { | ||
| self.lambda_inverse | ||
| } |
There was a problem hiding this comment.
Storing 1 / λ was previously an implementation detail.
There was a problem hiding this comment.
Further to my comments below: we may want to keep this method (but maybe also add Exp::lambda)?
rand_distr/src/gamma.rs
Outdated
| if self.switched_params { | ||
| return self.b; | ||
| } | ||
|
|
||
| self.a |
There was a problem hiding this comment.
Nit: early return is poor code style where it's not used to skip further computations.
There was a problem hiding this comment.
Do you prefer to:
- Use an intermediate assignment upon checking the condition,
- Use a rust-like approach with a match,
- Other?
Is there any documentation about the coding standards that are used in this repository? I don't see any CONTRIBUTING.md guide.
There was a problem hiding this comment.
I would just use if .. { self.b } else { self.a }. Simple.
No, we don't have a style guide beyond make it neat. https://rust-random.github.io/book/contributing.html
There was a problem hiding this comment.
Sounds good! I'll update the getter!
| pub fn scale(&self) -> F { | ||
| match self.repr { | ||
| // By definition of `one`. | ||
| One(exp) => exp.lambda_inverse().recip(), |
There was a problem hiding this comment.
This is actually wrong: Gamma::One uses Exp::new(1 / scale) so lambda_inverse is scale, not its inverse.
It seems like it would be worth adding Exp::new_inverse and keeping lambda_inverse here.
| // Since `DoFAnythingElse` is computed using Gamma(shape = 0.5 * k, ...), | ||
| // we revert the operation k = (1. / 0.5) * shape = 2. * shape. |
There was a problem hiding this comment.
Comments like these are probably not useful.. reviewers can't rely on them and probably no one else has a use for them. So probably they should be removed?
| /// Returns the (inverse of the) shape parameter (`lambda`) of the distribution. | ||
| pub fn lambda_inverse(&self) -> F { | ||
| self.lambda_inverse | ||
| } |
There was a problem hiding this comment.
Further to my comments below: we may want to keep this method (but maybe also add Exp::lambda)?
| /// Returns the mean (`μ`) of the distribution. | ||
| pub fn mean(&self) -> F { | ||
| self.norm.mean() | ||
| } | ||
|
|
||
| /// Returns the standard deviation (`σ`) of the distribution. | ||
| pub fn std_dev(&self) -> F { | ||
| self.norm.std_dev() | ||
| } |
There was a problem hiding this comment.
The method names and descriptions are incorrect: these are the log-space mean and standard deviation.
I suggest calling the methods mu, sigma (copying the names of parameters to new).
The actual mean of a log-normal is exp(μ + σ²/2), but we probably don't need a method for that.
| pub fn n(&self) -> F { | ||
| match self.s != F::one() { | ||
| true => { | ||
| // By definition: | ||
| // t = (n^(1 - s) - s) * q | ||
| // Therefore: | ||
| // t / q = n^(1 - s) - s | ||
| // t / q + s = n^(1 - s) | ||
| // (t / q + s)^(1/(1 - s)) = n | ||
| F::powf(self.t / self.q + self.s, F::recip(F::one() - self.s)) | ||
| }, | ||
| false => { | ||
| // By definition: | ||
| // t = 1 - ln(n) | ||
| // Therefore: | ||
| // t - 1 = - ln(n) | ||
| // 1 - t = ln(n) | ||
| // e^(1 - t) = n | ||
| F::exp(F::one() - self.t) | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
Another complex case to check (or exclude)
Add getters for distributions that satisfy (1) and (2) criteria described in #1233, also provide a reference implementation for reverse-computing getters distribution that satisfy only (1), except for
Hypergeometric,PertandWeightedAlias.This is still a draft since we need to decide what to do for these distribution that satisfy only (1), eventually we can simply revert the commits with the reverse-computing getters and keep the exact getters.
Closes #1233.