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 dimensions can be passed to Multinomial, NegativeMultinomial #16834

Merged
merged 17 commits into from May 19, 2019

Conversation

Projects
None yet
4 participants
@czgdp1807
Copy link
Member

commented May 14, 2019

References

Enhances #16808
Discussion at https://gitter.im/sympy/stats

Brief description of what is fixed or changed

N/A

Other comments

@sidhantnagpal 's comment:
I have a couple of comments about the merged #16808 for Multinomial distribution. Firstly, there should be additional conditions to check if sum(x) = n(the PMF could be 0 if this is not the case) and sum(p) = 1. Secondly, as @Upabjojr mentioned we should allow symbolic number of trials n. This is currently what happens:

In [4]: n = Symbol('n')                                                        
In [5]: x, p = map(symbols, ('x:4', 'p:4'))                                    
In [6]: X = Multinomial('X', n, *p) 
ValueError: number of trials must be a positve integer

Release Notes

NO ENTRY

@sympy-bot

This comment has been minimized.

Copy link

commented May 14, 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
<!-- 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. -->
Enhances https://github.com/sympy/sympy/pull/16808
Discussion at https://gitter.im/sympy/stats

#### Brief description of what is fixed or changed
N/A

#### Other comments
@sidhantnagpal 's comment:
 I have a couple of comments about the merged sympy/sympy#16808 for Multinomial distribution. Firstly, there should be additional conditions to check if sum(x) = n(the PMF could be 0 if this is not the case) and sum(p) = 1. Secondly, as @Upabjojr mentioned we should allow symbolic number of trials n. This is currently what happens:
```
In [4]: n = Symbol('n')                                                        
In [5]: x, p = map(symbols, ('x:4', 'p:4'))                                    
In [6]: X = Multinomial('X', n, *p) 
ValueError: number of trials must be a positve integer
```

#### 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 -->

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

@sidhantnagpal @Upabjojr Please take a look and let me know if the changes are fine.

Show resolved Hide resolved sympy/stats/joint_rv_types.py Outdated
@codecov

This comment has been minimized.

Copy link

commented May 14, 2019

Codecov Report

Merging #16834 into master will decrease coverage by 0.014%.
The diff coverage is 77.777%.

@@              Coverage Diff              @@
##            master    #16834       +/-   ##
=============================================
- Coverage   73.871%   73.857%   -0.015%     
=============================================
  Files          619       619               
  Lines       159772    159776        +4     
  Branches     37494     37493        -1     
=============================================
- Hits        118026    118006       -20     
- Misses       36288     36308       +20     
- Partials      5458      5462        +4
@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented May 19, 2019

@sidhantnagpal @smichr Please take a look. Currently, some technical issues, don't allow me to use Range, hence I have tried to use imageset here. I am working on Range in #16838. That will take a long time to be merged and until that at least this PR shouldn't be stalled. If possible, please merge this PR. Thanks.

Show resolved Hide resolved sympy/stats/joint_rv_types.py Outdated
Update sympy/sets/handlers/intersection.py
Co-Authored-By: Christopher Smith <smichr@gmail.com>
Show resolved Hide resolved sympy/stats/joint_rv_types.py Outdated
Update sympy/stats/joint_rv_types.py
Co-Authored-By: Christopher Smith <smichr@gmail.com>
Show resolved Hide resolved sympy/stats/joint_rv_types.py Outdated
Show resolved Hide resolved sympy/stats/joint_rv_types.py Outdated
Show resolved Hide resolved sympy/stats/joint_rv_types.py Outdated
Show resolved Hide resolved sympy/stats/joint_rv_types.py Outdated

czgdp1807 and others added some commits May 19, 2019

Update sympy/stats/joint_rv_types.py
Co-Authored-By: Christopher Smith <smichr@gmail.com>
Update sympy/stats/joint_rv_types.py
Co-Authored-By: Christopher Smith <smichr@gmail.com>
Update sympy/stats/joint_rv_types.py
Co-Authored-By: Christopher Smith <smichr@gmail.com>
Show resolved Hide resolved sympy/stats/joint_rv_types.py Outdated

czgdp1807 and others added some commits May 19, 2019

Update sympy/stats/joint_rv_types.py
Co-Authored-By: Christopher Smith <smichr@gmail.com>

@czgdp1807 czgdp1807 force-pushed the czgdp1807:multivar_imprv branch from 708bc70 to f685659 May 19, 2019

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented May 19, 2019

@smichr I have made the changes as you suggested. Please check the diff and tell me if anything more is required?

czgdp1807 and others added some commits May 19, 2019

@smichr

This comment has been minimized.

Copy link
Member

commented May 19, 2019

anything more is required?

Get the popcorn and wait for the results! :-)

@smichr

This comment has been minimized.

Copy link
Member

commented May 19, 2019

Can SymPy do what is requested here?

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented May 19, 2019

Can SymPy do what is requested here?

@smichr @ritesh99rakesh will work on Compound Distributions in Phase 1. It is not related to the current PR.

@smichr smichr merged commit 8ee7fbd into sympy:master May 19, 2019

3 checks passed

codecov/project 73.857% (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

This comment has been minimized.

Copy link
Member Author

commented May 19, 2019

@smichr Thank you very much for helping me by quick reviews and suggestions, in this PR. Now I can work on Range without any worries.

@czgdp1807 czgdp1807 deleted the czgdp1807:multivar_imprv branch May 19, 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.