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

Symbolic conditions allowed in sympy.stats.frv #16908

Merged
merged 12 commits into from Jun 10, 2019

Conversation

Projects
None yet
7 participants
@czgdp1807
Copy link
Member

commented May 27, 2019

References to other Issues or PRs

N/A

Brief description of what is fixed or changed

The @XFAIL decorator has been removed from the function, test_dependent_finite in sympy.stats.tests.test_rv.

Other comments

I was just going through the tests in sympy.stats.tests.test_rv looking for some general improvements to be made in stats module and came across this @XFAIL test. As an attempt to solve this I just commented the part which didn't allow foreign symbols in ConditionalFiniteDomain. The notable point is that after removing the said part all the tests of sympy.stats are passing on my local system as shown below.
My question is if no test is failing then why, foreign symbols are not allowed in ConditionalFiniteDomain. Either there should be test checking this fact or foreign symbols should be allowed. I have few more approaches in my mind if you are willing to discuss. :)
Please DO NOT CLOSE this PR, rather discuss and make suggestions.

Test Results

Python 3.6.7 (default, Oct 22 2018, 11:32:17) 
[GCC 8.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sympy
>>> sympy.test('/stats/')
============================================================================================= test process starts =============================================================================================
executable:         /usr/bin/python3  (3.6.7-final-0) [CPython]
architecture:       64-bit
cache:              yes
ground types:       python 
numpy:              1.15.3
random seed:        54277752
hash randomization: on (PYTHONHASHSEED=1784891003)

sympy/stats/tests/test_continuous_rv.py[72] ...w..........................................f................ww.......                                                                                       [OK]
sympy/stats/tests/test_discrete_rv.py[15] .......w.......                                                                                                                                                  [OK]
sympy/stats/tests/test_error_prop.py[2] ..                                                                                                                                                                 [OK]
sympy/stats/tests/test_finite_rv.py[19] ...................                                                                                                                                                [OK]
sympy/stats/tests/test_joint_rv.py[13] ............f                                                                                                                                                       [OK]
sympy/stats/tests/test_mix.py[3] ...                                                                                                                                                                       [OK]
sympy/stats/tests/test_rv.py[24] ........................                                                                                                                                                  [OK]
sympy/stats/tests/test_symbolic_probability.py[2] ..                                                                                                                                                       [OK]

________________________________________________________________________________________________ slowest tests ________________________________________________________________________________________________
test_JointPSpace_marginal_distribution - Took 11.347 seconds
================================================================= tests finished: 144 passed, 4 skipped, 2 expected to fail, in 61.95 seconds =================================================================
True

Release Notes

NO ENTRY

@sympy-bot

This comment has been minimized.

Copy link

commented May 27, 2019

Hi, I am the SymPy bot (v147). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

  • No release notes entry will be added for this pull request.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->
N/A

#### Brief description of what is fixed or changed
The `@XFAIL` decorator has been removed from the function, `test_dependent_finite` in `sympy.stats.tests.test_rv`.

#### Other comments
I was just going through the tests in `sympy.stats.tests.test_rv` looking for some general improvements to be made in stats module and came across this `@XFAIL` test. As an attempt to solve this I just commented the part which didn't allow foreign symbols in `ConditionalFiniteDomain`. The notable point is that after removing the said part all the tests of `sympy.stats` are passing on my local system as shown below. 
My question is if no test is failing then why, foreign symbols are not allowed in `ConditionalFiniteDomain`. Either there should be test checking this fact or foreign symbols should be allowed. I have few more approaches in my mind if you are willing to discuss. :)
Please **DO NOT CLOSE** this PR, rather discuss and make suggestions. 

**Test Results**
```
Python 3.6.7 (default, Oct 22 2018, 11:32:17) 
[GCC 8.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sympy
>>> sympy.test('/stats/')
============================================================================================= test process starts =============================================================================================
executable:         /usr/bin/python3  (3.6.7-final-0) [CPython]
architecture:       64-bit
cache:              yes
ground types:       python 
numpy:              1.15.3
random seed:        54277752
hash randomization: on (PYTHONHASHSEED=1784891003)

sympy/stats/tests/test_continuous_rv.py[72] ...w..........................................f................ww.......                                                                                       [OK]
sympy/stats/tests/test_discrete_rv.py[15] .......w.......                                                                                                                                                  [OK]
sympy/stats/tests/test_error_prop.py[2] ..                                                                                                                                                                 [OK]
sympy/stats/tests/test_finite_rv.py[19] ...................                                                                                                                                                [OK]
sympy/stats/tests/test_joint_rv.py[13] ............f                                                                                                                                                       [OK]
sympy/stats/tests/test_mix.py[3] ...                                                                                                                                                                       [OK]
sympy/stats/tests/test_rv.py[24] ........................                                                                                                                                                  [OK]
sympy/stats/tests/test_symbolic_probability.py[2] ..                                                                                                                                                       [OK]

________________________________________________________________________________________________ slowest tests ________________________________________________________________________________________________
test_JointPSpace_marginal_distribution - Took 11.347 seconds
================================================================= tests finished: 144 passed, 4 skipped, 2 expected to fail, in 61.95 seconds =================================================================
True
```

#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
NO ENTRY
<!-- END RELEASE NOTES -->

@codecov

This comment has been minimized.

Copy link

commented May 27, 2019

Codecov Report

Merging #16908 into master will increase coverage by 0.048%.
The diff coverage is 87.5%.

@@              Coverage Diff              @@
##            master    #16908       +/-   ##
=============================================
+ Coverage   73.906%   73.954%   +0.048%     
=============================================
  Files          619       620        +1     
  Lines       160001    160527      +526     
  Branches     37554     37665      +111     
=============================================
+ Hits        118251    118717      +466     
- Misses       36269     36322       +53     
- Partials      5481      5488        +7
@smichr

This comment has been minimized.

Copy link
Member

commented May 27, 2019

if no test is failing then why, foreign symbols are not allowed in ConditionalFiniteDomain.

If this passed at the time it was written, then a test demonstrating that it allows an invalid result should be added. If it didn't pass then, then a check for the commit that allowed it to pass should be made to see if it should be allowed to pass now. Either way, a test demonstrating that a valid conclusion still holds when the code is commented out would be good.

@czgdp1807 czgdp1807 changed the title [GSoC] Removed XFAIL from test_rv.py Removed XFAIL from test_rv.py May 28, 2019

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented May 28, 2019

ping @Upabjojr

Show resolved Hide resolved sympy/stats/frv.py Outdated
@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

@Abdullahjavednesar I have made the update. Please remove author's turn label.

@smichr smichr changed the title Removed XFAIL from test_rv.py add foreign symbol test in test_rv.py May 31, 2019

@smichr

This comment has been minimized.

Copy link
Member

commented May 31, 2019

Is there any way that you can show that the result would be wrong if the symbol is allowed to remain and the error is not raised?

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 1, 2019

As, can be seen in the new diff that, I have used one approach of allowing symbolic conditions with foreign symbols in finite random variables. The other approach is given below,
We can use Probability to return un-evaluated results, however, the problem with Probability is the absence of doit method in it, which doesn't allow things like, Probability(D > x).subs(x, 2).doit(). An example is shown below,
Code

D = Die('D', 6)
Z = Probability(D > x)
print(Z)
Z = Z.subs(x, 2)
print(Z)
print(Z.doit())

Output

Probability(D > x)
Probability(D > 2)
Probability(D > 2) # remains un-evaluated even when the symbol is replaced with number and doit is called

If the approach in the diff is not acceptable then my proposal is to add doit() method in Probability that can evaluate probabilities for numeric conditions.

@smichr

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

Can you give an example of the Piecewise output?

(Also 'Undeciable' -> 'Undecidable' at line 170 of frv.)

czgdp1807 added some commits Jun 2, 2019

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2019

Example

>>> from sympy.stats import P, Die, Bernoulli
>>> from sympy import symbols, Eq
>>> D = Die('D', 6)
>>> x = symbols('x')
>>> Z = P(D > x)
>>> Z
Piecewise((1/6, x < 1), (0, True)) + Piecewise((1/6, x < 2), (0, True)) + Piecewise((1/6, x < 3), (0, True)) + Piecewise((1/6, x < 4), (0, True)) + Piecewise((1/6, x < 5), (0, True)) + Piecewise((1/6, x < 6), (0, True))
>>> Z.subs(x, 2)
2/3
>>> B = Bernoulli('B', 1/4)
>>> Y = P(B > x)
>>> Y
Piecewise((0.25, x < 1), (0, True)) + Piecewise((0.75, x < 0), (0, True))
>>> Y.subs(x, 0)
0.250000000000000
>>> X = P(Eq(B, x))
>>> X
Piecewise((0.25, Eq(x, 1)), (0, True)) + Piecewise((0.75, Eq(x, 0)), (0, True))
>>> X.subs(x, 0)
0.750000000000000
@smichr

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

Would it be better to piecewise_fold the sum? e.g.

Piecewise((1, x < 1), (5/6, x < 2), (2/3, x < 3), (1/2, x < 4), (1/3, x < 5), (1/6, x < 6), (0, True))
@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2019

I will work on it but I am waiting for @Upabjojr 's view on the approach which I have used in the diff to allow symbolic conditions. The representation can be improved after that. Thanks for the suggestion.

@Abdullahjavednesar Abdullahjavednesar requested a review from Upabjojr Jun 3, 2019

@Upabjojr

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

The set module has ways to expand a set into conditions of inequalities. My concern is about inequalities that are highly intertwined, and maybe cannot be solved, how do you handle them?

Make sure that the expressions that are not raising exceptions anymore return symbolic probability (in the tests!).

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

how do you handle them?

My approach doesn't solve the inequalities. It just searches the random expression in the inequality/equality and replaces those random expression with the elements in the domain of the expression. Currently this PR covers the basic case of having Relational conditions and more work(may be in a different PR) is needed to cover complex conditions like, those with &, etc.

Make sure that the expressions that are not raising exceptions anymore return symbolic probability

Yes, I am running tests locally on my system to see if any tests are failing due to my changes and correcting the code or the tests, whatever is necessary.

From your comment I assume that the approach in the diff is better than the one given in this comment. Thanks for the clarification. I will make more changes to make this PR good. If you have any comment to share by tonight then please do so.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

Would it be better to piecewise_fold the sum?

I will try it today.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

Would it be better to piecewise_fold the sum?

I observed that it's very slow. And if user wants to simplify the result then he/she can directly do so by using simplify.

czgdp1807 added some commits Jun 7, 2019

Show resolved Hide resolved sympy/stats/frv_types.py Outdated

@czgdp1807 czgdp1807 changed the title add foreign symbol test in test_rv.py Symbolic conditions allowed in sympy.stats.frv Jun 9, 2019

@Upabjojr Upabjojr merged commit 0533158 into sympy:master Jun 10, 2019

3 checks passed

codecov/project 73.954% (target 0%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sympy-bot/release-notes The release notes look OK
Details

@czgdp1807 czgdp1807 deleted the czgdp1807:xfail_rv branch Jun 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.