Skip to content
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

Error with handmade Gamma Distribution (Easy Fix) #19959

Closed
Maelstrom6 opened this issue Aug 13, 2020 · 4 comments · Fixed by #20120
Closed

Error with handmade Gamma Distribution (Easy Fix) #19959

Maelstrom6 opened this issue Aug 13, 2020 · 4 comments · Fixed by #20120
Labels

Comments

@Maelstrom6
Copy link
Contributor

Problem

The following was working in sympy version 1.5. It does not work in 1.6.

The following code returns ValueError: The pdf on the given set is incorrect.:

from sympy.stats import *
from sympy import *

x, k, theta = symbols(r"x k theta", positive=True)
pdf = 1/(gamma(k)*theta**k)*x**(k-1)*exp(-x/theta)

X = ContinuousRV(x, pdf, set=Interval(0, oo))

However, this is exactly the pdf in the documentation.

Upon inspection, I found in line 154 in crv_types.py, we have that val = integrate(pdf(x), (x, set)) returns theta*theta**(-k)*theta**(k - 1). I assume integrate does not simplify the expression since this value is 1.

Proposed possible fixes:

  • Replace val = integrate(pdf(x), (x, set)) with val = simplify(integrate(pdf(x), (x, set))). (This may be inefficient for complex integrals that turn out to be incorrect).
  • Replace _value_check(val == S.One, "The pdf on the given set is incorrect.") with _value_check((val - S.One) == S.Zero, "The pdf on the given set is incorrect."). (This might also use the simplify method behind the scenes. But if it does not, it will be more efficient).

drv_types.py may have the same issue.

Unless I have done something horribly wrong, should I submit a PR for this? (I'll also add unit tests for this).

@Smit-create
Copy link
Member

Smit-create commented Aug 14, 2020

* `_value_check(val == S.One, "The pdf on the given set is incorrect.")`

Well, that's the issue with integrate I think that it doesn't evaluate completely always. But adding simplify can cost more so I avoided using it while I was adding the check. What do you say @czgdp1807
I had a similar issue in which I used Normal Distribution's pdf where even simplify didn't work, see #19645

@czgdp1807
Copy link
Member

The check happens only once, right i.e., during the object creation? So, may be simplify shouldn't create problem once the value checks are done. Other approach is to not do anything if val is an Expr and not a number i.e., is val is not guaranteed to be anything other than S.One just allow the PDF. Fail only if there is a surety of incorrectness. @Upabjojr Any thoughts?

@Smit-create
Copy link
Member

he check happens only once, right i.e., during the object creation?

Yes, only once.

Fail only if there is a surety of incorrectness

Removing the check may be more useful as it is always not sure even using simplify that we will get 1

@Maelstrom6
Copy link
Contributor Author

Removing the check may be more useful

Maybe log a warning instead of throw an error when the integral is not one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants