[ENH] Exponential distribution
#325
Conversation
skpro/distributions/exponential.py
Outdated
| def _get_scipy_object(self) -> rv_continuous: | ||
| return expon | ||
|
|
||
| def _get_scipy_param(self) -> dict: |
There was a problem hiding this comment.
return type here is a dict, but then you are returning a tuple composed of a list and a dict
There was a problem hiding this comment.
Indeed the return type must be changed.
skpro/distributions/exponential.py
Outdated
|
|
||
| Parameter | ||
| --------- | ||
| mu : float or array of float (1D or 2D) |
There was a problem hiding this comment.
I would give this a single parameter, rate. The scipy parameter scale is then the inverse of rate.
There was a problem hiding this comment.
Yes that is correct I misunderstood the distrbution's parameter in scipy. It needs to be rate itself.
But I do not seem to understand how it would be able to do the shifting and scaling using loc and scale as they have described in the docs if I am only using the rate parameter.
The probability density above is defined in the “standardized” form.
To shift and/or scale the distribution use the loc and scale parameters. Specifically, expon.pdf(x, loc, scale) is
identically equivalent to expon.pdf(y) / scale with y = (x - loc) / scale. Note that shifting the location of a distribution
does not make it a “noncentral” distribution; noncentral generalizations of some distributions are available in separate classes.
There was a problem hiding this comment.
I would suggest, let's use the same parameters as on wikipedia:
https://en.wikipedia.org/wiki/Exponential_distribution
We cannot use lambda as it is a reserved python keyword, so rate is the next best thing as it is consistent with Poisson process terminology.
skpro/distributions/__init__.py
Outdated
| @@ -21,12 +21,14 @@ | |||
| "TDistribution", | |||
| "Uniform", | |||
| "Weibull", | |||
| "Exponential", | |||
There was a problem hiding this comment.
can you put this to be alphabetically ordered?
| @@ -43,6 +43,7 @@ Continuous support | |||
| Normal | |||
| TDistribution | |||
| Weibull | |||
| Exponential | |||
There was a problem hiding this comment.
can you put this to be alphabetically ordered?
fkiraly
changed the title
Exponential distributionExponential distribution
|
I've made the changes accordingly please let me know if anything else needs to be done. I shall create PR with the updated |
What does this implement/fix? Explain your changes.
This interfaces the
Exponentialdistribution using the_ScipyAdapter.Does your contribution introduce a new dependency? If yes, which one?
No
Did you add any tests for the change?
Yes
PR checklist
For all contributions
See here for full badge reference
For new estimators
docs/source/api_reference/taskname.rst, follow the pattern.(https://www.sktime.net/en/latest/developer_guide/dependencies.html#adding-a-soft-dependency).