-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
WIP: Implement more continuous log CDF functions #2048
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
Conversation
The reasoning behind this is that a Flat distribution is a Uniform distribution with bounds stretching symmetrically towards negative and positive infinity. Or a Normal distribution with infinite standard deviation. Both of these give values of 1/2 for any finite point in the CDF. At negative infinity, the CDF is 0 and at positive infinity it is 1.
Since the normal CDF is used in both Wald and Normal it is refactored into a function but not exported
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.
Looks cool to me. Good contributions. If you could add a few tests for infinite values that'd be cool. Otherwise we can leave it for another commit.
pymc3/tests/test_distributions.py
Outdated
@@ -390,6 +390,8 @@ def test_discrete_unif(self): | |||
|
|||
def test_flat(self): | |||
self.pymc3_matches_scipy(Flat, Runif, {}, lambda value: 0) | |||
# TODO: Check infinite values? |
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.
I would check infinite values. Seems a good check.
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.
I've pushed a quick explicit couple of tests.
I guess we need to wait for Beta to get this to merge. But good work @domenzain :) |
pymc3/distributions/continuous.py
Outdated
lam = self.lam | ||
alpha = self.alpha | ||
|
||
value = value - alpha |
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.
use increment value += alpha
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.
Does it all work with tensors, not scalars, we need more tests for that
pymc3/distributions/continuous.py
Outdated
tt.log1p(-tt.erfc(z / tt.sqrt(2.)) / 2.) | ||
) | ||
|
||
# TODO: Decide where to put this, or inline it everywhere it is used... |
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.
You can move it to dist_math
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.
Done.
pymc3/distributions/continuous.py
Outdated
"Accurately computing log(1-exp(-|a|)) Assessed by the Rmpfr package" | ||
""" | ||
lam = self.lam | ||
a = lam * float(value) |
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.
What if value is tensor? Mb tt.as_tensor(value)
fits better there
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.
Changing this. I'm using floatX
and as_tensor
instead.
@@ -181,6 +182,17 @@ def random(self, point=None, size=None, repeat=None): | |||
def logp(self, value): | |||
return tt.zeros_like(value) | |||
|
|||
def logcdf(self, value): | |||
return tt.switch( |
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.
Do you check vector value for this op?
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.
I think all of these are element-wise operations.
When a tensor is passed as argument they would behave as expected:
>>> tt.eq(np.array([1, 2, np.nan, -np.inf, np.inf]), np.inf).eval()
array([False, False, False, False, True], dtype=bool)
pymc3/distributions/continuous.py
Outdated
) | ||
|
||
# TODO: Decide where to put this, or inline it everywhere it is used... | ||
def normallogcdf(value, mu=0, sd=1): |
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.
Does it work with ndimentional tensors?
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.
I'm unsure about broadcasting when the tensors will not be shaped identically.
But otherwise everything is element-wise ok.
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.
Latest review taken into account and integrated in commits where applicable.
pymc3/distributions/continuous.py
Outdated
tt.log1p(-tt.erfc(z / tt.sqrt(2.)) / 2.) | ||
) | ||
|
||
# TODO: Decide where to put this, or inline it everywhere it is used... |
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.
I don't know where to put this. I'm guessing there must be a more logical place than here.
pymc3/tests/test_distributions.py
Outdated
@@ -390,6 +390,8 @@ def test_discrete_unif(self): | |||
|
|||
def test_flat(self): | |||
self.pymc3_matches_scipy(Flat, Runif, {}, lambda value: 0) | |||
# TODO: Check infinite values? |
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.
I've pushed a quick explicit couple of tests.
@@ -181,6 +182,17 @@ def random(self, point=None, size=None, repeat=None): | |||
def logp(self, value): | |||
return tt.zeros_like(value) | |||
|
|||
def logcdf(self, value): | |||
return tt.switch( |
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.
I think all of these are element-wise operations.
When a tensor is passed as argument they would behave as expected:
>>> tt.eq(np.array([1, 2, np.nan, -np.inf, np.inf]), np.inf).eval()
array([False, False, False, False, True], dtype=bool)
pymc3/distributions/continuous.py
Outdated
tt.log1p(-tt.erfc(z / tt.sqrt(2.)) / 2.) | ||
) | ||
|
||
# TODO: Decide where to put this, or inline it everywhere it is used... |
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.
Done.
pymc3/distributions/continuous.py
Outdated
) | ||
|
||
# TODO: Decide where to put this, or inline it everywhere it is used... | ||
def normallogcdf(value, mu=0, sd=1): |
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.
I'm unsure about broadcasting when the tensors will not be shaped identically.
But otherwise everything is element-wise ok.
pymc3/distributions/continuous.py
Outdated
"Accurately computing log(1-exp(-|a|)) Assessed by the Rmpfr package" | ||
""" | ||
lam = self.lam | ||
a = lam * float(value) |
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.
Changing this. I'm using floatX
and as_tensor
instead.
I've removed |
@domenzain I was giving it a try, but have not finished yet. Keep in mind, everyone, that this is being merged into a feature branch, so there will be another opportunity to review it before it is merged into master. |
I will merge this into my feature branch. |
Implement several log CDF functions
The distributions concerned are:
Each implementation is then tested against SciPy's own implementation.