Skip to content

Add getters for distribution parameters #1233

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

Closed
AlessioZanga opened this issue May 27, 2022 · 7 comments
Closed

Add getters for distribution parameters #1233

AlessioZanga opened this issue May 27, 2022 · 7 comments
Labels
X-stale Outdated or abandoned work

Comments

@AlessioZanga
Copy link

Background

What is your motivation?

Think about the Beta distribution from rand_distr. It has two parameters, namely alpha and beta, both are not public. Adding getters for distribution parameters allow to generate new distributions given previous ones, without the need to store the parameters elsewhere.

What type of application is this? (E.g. cryptography, game, numerical simulation)

This feature request is a quality-of-life improvement that impact every field.

Feature request

Add getters for parameters of each distribution, similarly to the API defined in statrs distributions.

@dhardy
Copy link
Member

dhardy commented May 27, 2022

This has been discussed in the past in #560. Mainly, the concern is that we primarily care about sampling and not all distributions store their input parameters directly, and for those that do, we cannot be certain that the approach will not be changed in the future. For those that don't where parameters could be reconstructed but doing so might be lossy (limited precision) is this a good idea?

This in short is why we haven't done this already, though the argument is probably too general; we should consider individual cases.

For the Beta distribution, parameters are stored, possibly swapped. This should be easy to add getters for. Without further research I don't know whether their are alternative sampling methods which wouldn't directly store those parameters.

@AlessioZanga
Copy link
Author

Thank you for your reply! I looked for similar issues, but I missed #560 completely.

I know that the main focus of the library is sampling, but the QOL improvement deriving from the implementation of getters would be significant. If you think that adding getters in case of stored (possibly swapped) parameters could be a meaningful use case, I can take care of implementing them.

@vks
Copy link
Collaborator

vks commented May 27, 2022

We already have getters for Normal (see here), so I think it makes sense to add them for other distributions as well. PRs are very welcome!

@AlessioZanga
Copy link
Author

Current status

Evaluation criteria:

  1. Has parameters?
  2. Stores parameters as provided?
  3. Implements getters?

Summary of the evaluation criteria:

Distribution 1 2 3
Beta ✔️ ✔️1
Binomial ✔️ ✔️
Cauchy ✔️ ✔️
ChiSquared ✔️
Dirichlet ✔️ ✔️
Exp ✔️
Exp1
Fisher ✔️
Frechet ✔️ ✔️
Gamma ✔️
Geometric ✔️ ✔️
Gumbel ✔️ ✔️
Hypergeometric ✔️
InverseGaussian ✔️ ✔️
LogNormal ✔️ ✔️
NormalInverseGaussian ✔️
Normal ✔️ ✔️ ✔️
Pareto ✔️
Pert ✔️
Poisson ✔️ ✔️
SkewNormal ✔️ ✔️ ✔️
Student ✔️
Triangular ✔️ ✔️
UnitBall
UnitCircle
UnitDisc
UnitSphere
Weibull ✔️
WeightedAlias ✔️
Zeta ✔️
Zipf ✔️

Addressed issues

  • Distributions that do not satisfy (1) do not need getters, obviously,
  • Distributions that satisfy both (1) and (2) are able to return the parameters exactly, with a simple return statement.
  • Distributions that satisfy (1), (2) and (3) already return the parameters exactly.

Remaining issues

  • Distribution that satisfy only (1) are able to return the input parameters with some (possible) loss of precision.

Since the main focus of the library is sampling, we can:

  1. Simply ignore the non-stored parameters,
  2. Reverse compute the input parameters,
  3. Store a copy of the input parameters along with the precalculated values, e.g. the Poisson distribution.

Footnotes

  1. The Beta distribution may store the parameters swapped, but there is a flag that can be used to check that.

@dhardy
Copy link
Member

dhardy commented May 30, 2022

Concern: accuracy of result

Mentioned in a review comment of #1234, getter result may not equal constructor parameter. Should be mentioned in docs. Slight loss of precision is not usually an issue, but we should review for possible non-finite (inf/nan) values.

Concern: API lock-in of algorithm

It is potentially useful to allow internal algorithms to be changed. Such a change would already be considered a value-breaking change but not an API-breaking change. We should therefore be careful about exposing internal details (beyond minor loss of precision).

More significant would be cases where parameter calculation would no longer be possible with a new algorithm, without storing extra fields. Unlikely an issue.

Concern: maintenance burden

Extra work would now be required to replace algorithms in some cases due to computations for getters.


These are all minor concerns and not blockers.

We might consider only implementing getters in simpler cases.

Thanks for the PR, Alessio Zanga.

@dhardy
Copy link
Member

dhardy commented Aug 3, 2022

Sorry for the silence on this issue. I suggest we go ahead with PR #1234 with the following guidelines:

  • Methods reverse-compute input parameters where required (i.e. fn Exp::lambda, not lambda_inverse)
  • In any case where loss of accuracy or range is possible due to these calculations, we add a note in the method documentation
  • If any particular getter method is difficult to implement or has significant issues, simply remove that method (unless someone knows they need it). We don't need a comprehensive solution here.

Edit: review of #1234 brought up the case where Gamma::scale uses Exp::lambda_inverse: in this case we would have to take the reciprical twice if we had to use Exp::lambda to retrieve this parameter (Gamma::new(1.0, scale) already takes the reciprical twice). So perhaps we should keep Exp::lambda_inverse and add Exp::new_inverse — but this is probably a special case.

@dhardy dhardy added the X-stale Outdated or abandoned work label Jul 20, 2024
@dhardy
Copy link
Member

dhardy commented Jul 20, 2024

Closing as stale since it's not something I see as in need of solving. That said, if someone wishes to open a PR implementing some of the above, please go ahead.

@dhardy dhardy closed this as completed Jul 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
X-stale Outdated or abandoned work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants