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
[WIP] Removed ProductDiscretePSpace
and ProductContinuousPSpace
#14777
Conversation
Added classes `ProductDiscretePSpace`, `ProductDiscreteDomain` and `DiscreteDistributionHandmade` to allow expressions containing multiple random variables.
Replaced `DiracDelta` with `KroneckerDelta` functions for calculating probabilities with multiple discrete random variables. Added flag to probability to return unevaluated object. Added tests.
Modified test in test_discrete_rv.py to assert equality by comparing strings and made minor modification to drv.py
…probability_space
Added and removed some import statements in drv.py
Removed `compute_density` from the two classes and added a method to `ProductPSpace` to calculate the density of any given combination of continuous and/or discrete random variables.
Removed tests for `ProductDiscretePSpace` and `ProductContinuousPSpace` from core/tests/test_args.py, and added `conditional_space` method to `ProductPSpace`
sympy/stats/rv.py
Outdated
if any(pspace(rv).is_Continuous for rv in rvs): | ||
expr = self.integrate(DiracDelta(expr - z), | ||
**kwargs) | ||
elif any(pspace(rv).is_Discrete for rv in rvs): |
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.
Would else:
be sufficient here?
sympy/core/tests/test_args.py
Outdated
@@ -761,7 +760,6 @@ def test_sympy__stats__crv__ConditionalContinuousDomain(): | |||
D = SingleContinuousDomain(x, Interval(-oo, oo)) | |||
assert _test_args(ConditionalContinuousDomain(D, x > 0)) | |||
|
|||
|
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.
we usually leave 2 newlines on the base scope
What about mixed discrete and continuous product spaces? |
If the expression contains both discrete and continuous random variables, the product space is taken to be continuous, with a discrete component. The density is then calculated using |
OK, can you add a test for the mixed case? |
from sympy import symbols | ||
n = symbols('n') | ||
oo = S.Infinity | ||
assert str(P(X1 + X2 < 3, evaluate=False) == Sum(Piecewise((2**(X2 - n - 2)*(2/3)**(X2 - 1)/6, |
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.
this assert usage is wrong, you are evaluating a string.
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 suggest str(P( ... )) == """ string expression """
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.
Thanks! I was struggling with this.
Corrected tests is test_discrete_rv.py and added tests to test_mix.py
assert P(Eq(Y + E, 1)) == 0 | ||
assert P(Ne(Y + E, 2)) == 1 | ||
assert str(P(E + Y < 2, evaluate=False)) == """Integral(Sum(exp(-1)*Integral"""\ | ||
+"""(exp(-E)*DiracDelta(-_z + E + Y - 2), (E, 0, oo))/factorial(Y), (Y, 0, oo)), (_z, -oo, 0))""" |
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 believe this integral diverges to infinity.
References to other Issues or PR
Brief description of what is fixed or changed
Removed classes
ProductDiscretePSpace
andProductContinuousPSpace
and moved the methods toProductPSpace
. No new tests have been added, as tests already present check for the functionality of these classes.Other comments
closes #14730