-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
ENH: override dweibull distribution survival and inverse survival function #18232
Conversation
The import numpy as np
from scipy import stats
x, c = stats.uniform.rvs(size=(2, 10))
np.testing.assert_allclose(stats.dweibull(c).sf(x), stats.weibull_min(c).sf(abs(x))/2) To simplify, can you use the private functions of For tests, rather than comparing against reference values from |
scipy/stats/_continuous_distns.py
Outdated
def _sf(self, x, c): | ||
Cx1 = 0.5 * np.exp(-abs(x)**c) | ||
return np.where(x > 0, Cx1, 1 - Cx1) | ||
|
||
def _isf(self, q, c): | ||
fac = 2. * np.where(q <= 0.5, q, 1. - q) | ||
fac = np.power(-np.log(fac), 1.0 / c) | ||
return np.where(q > 0.5, -fac, fac) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def _sf(self, x, c): | |
Cx1 = 0.5 * np.exp(-abs(x)**c) | |
return np.where(x > 0, Cx1, 1 - Cx1) | |
def _isf(self, q, c): | |
fac = 2. * np.where(q <= 0.5, q, 1. - q) | |
fac = np.power(-np.log(fac), 1.0 / c) | |
return np.where(q > 0.5, -fac, fac) | |
def _sf(self, x, c): | |
p = weibull_min._sf(x, c) / 2 | |
return np.where(x > 0, p, 1 - p) | |
def _isf(self, p, c): | |
i = (p <= 0.5) | |
p = 2 * np.where(i, p, 1 - p) | |
q = weibull_min._isf(p, c) | |
return np.where(i, q, -q) |
or it's OK to follow the (unfortunate) naming conventions where _isf
accepts argument q
.
This fails tests until _sf
is overridden for weibull_min
. Following the style of its ppf
,
def _isf(self, q, c):
return pow(-np.log(q), 1.0/c)
or
def _isf(self, p, c):
return (-np.log(p))**(1.0/c)
would be fine, too. I don't know how the inverse cdf/sf methods started accepting q
as the input. p
would make more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not accept and silently revert changes in the future, please.
# reference values were computed with mpmath | ||
# from mpmath import mp | ||
# mp.dps = 50 | ||
# def sf_mpmath(x, c): | ||
# x = mp.mpf(x) | ||
# c = mp.mpf(c) | ||
# if x > 0: | ||
# return mp.mpf(0.5) * mp.exp(-x**c) | ||
# else: | ||
# return mp.one - mp.mpf(0.5) * mp.exp(-(-x)**c) | ||
# for the inverse survival function tests, swap ref and x | ||
|
||
@pytest.mark.parametrize('x, c, ref', | ||
[(1e20, 0.1, 1.8600379880103705e-44), | ||
(1e5, 0.5, 2.306726997904701e-138)]) | ||
def test_sf_isf(self, x, c, ref): | ||
assert_allclose(stats.dweibull.sf(x, c), ref, rtol=5e-14) | ||
assert_allclose(stats.dweibull.isf(ref, c), x, rtol=5e-14) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's convert this to a test for weibull_min.sf
/isf
. The test for dweibull.sf
/isf
can follow the style of test_entropy
. There the comment doesn't need to be repeated, though.
Co-authored-by: Matt Haberland <mhaberla@calpoly.edu>
…into dweibull_sf_isf
@pytest.mark.parametrize('x, c, ref', [(50, 1, 1.9287498479639178e-22), | ||
(1000, 0.8, | ||
8.131269637872743e-110)]) | ||
def test_sf_isf(self, x, c, ref): | ||
assert_allclose(stats.weibull_min.sf(x, c), ref, rtol=1e-14) | ||
assert_allclose(stats.weibull_min.isf(ref, c), x, rtol=1e-14) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests for the inverse survival function fail in main.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. You'll need to override weibull_min._sf
weibull_min._isf
with the same idea as you originally had for dweibull._sf
dweibull._isf
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I understand exactly: weibull_min
's sf
method is already overwritten and very accurate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. As you might have guessed, I meant _isf
, since we're talking about the inverse survival function. That is not overridden. (I tried it locally when I made the suggestions for what to change about dweibull
, and overriding it made the existing dweibull
tests pass.
@@ -6349,6 +6349,20 @@ def test_fit_min(self): | |||
ref = np.mean(rvs), stats.skew(rvs) | |||
assert_allclose(res, ref) | |||
|
|||
# reference values were computed via mpmath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed with
class WeibullMin(ReferenceDistribution):
def __init__(self, *, c):
super().__init__(c=c)
def _pdf(self, x, c):
return c*x**(c-mp.one)*mp.exp(-x**c)
Reference issue
Part of #17832
What does this implement/fix?
Overrides
sf
andisf
for the double Weibull distribution.Additional information
For both functions, the available range and precision improved for positive values.
Survival function
Inverse survival function