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

[WIP]Some general bug fixes for sympy.stats #16934

Open
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@czgdp1807
Copy link
Member

commented May 31, 2019

References to other Issues or PRs

N/A

Brief description of what is fixed or changed

Some trivial bug fixes have been made for sympy.stats.joint_rv

Other comments

I want to know what's the purpose of MarginalDistribution in sympy.stats.That part of the code is uncovered by the tests and it is also not being used in computing marginal_distribution.
It would be great if someone can clarify on this.
I have an idea of using MarginalDistribution for returning the marginal distribution of distributions with symbolic dimensions like, MultivariateEwens.

Release Notes

NO ENTRY

@sympy-bot

This comment has been minimized.

Copy link

commented May 31, 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
Some trivial bug fixes have been made for `sympy.stats.joint_rv`

#### Other comments
I want to know what's the purpose of `MarginalDistribution` in `sympy.stats`.That part of the code is uncovered by the tests and it is also not being used in computing `marginal_distribution`.
It would be great if someone can clarify on this.
I have an idea of using `MarginalDistribution` for returning the marginal distribution of distributions with symbolic dimensions like, `MultivariateEwens`.

#### 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 31, 2019

Codecov Report

Merging #16934 into master will increase coverage by 0.005%.
The diff coverage is 35.714%.

@@              Coverage Diff              @@
##            master    #16934       +/-   ##
=============================================
+ Coverage   74.408%   74.414%   +0.005%     
=============================================
  Files          622       622               
  Lines       160859    160857        -2     
  Branches     37758     37758               
=============================================
+ Hits        119693    119701        +8     
+ Misses       35851     35833       -18     
- Partials      5315      5323        +8

@czgdp1807 czgdp1807 changed the title [GSoC] Some general bug fixes for sympy.stats.joint_rv Some general bug fixes for sympy.stats.joint_rv Jun 1, 2019

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 1, 2019

ping @Upabjojr

@sidhantnagpal

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

Were the changed lines not covered by tests earlier? If not, test(s) should be added to make sure they show the intended behaviour.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2019

Yes. However, the changed lines were not covered even by the code inside SymPy. I am unable to understand its purpose. Though we can use it to return un-evaluated results.

@sidhantnagpal

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

It seems to have got added in #14764. CC @akash9712

@Upabjojr

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

It's possible that the current state for MarginalDistribution may be buggy. It hasn't been tested that well in 2018.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

Okay, I will try to use it in joint distributions of symbolic dimensions.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

I observed that MarginalDistribution is used by CompoundDistribution. However, some of the methods like, domain, set weren't tested and hence, errors like RecursionError: maximum recursion depth exceeded while calling a Python object remained without being noticed. I have tried to fix them.

@Upabjojr

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

Can you also add some tests?

@Upabjojr

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

Everything which did not work before this PR should be tested to prevent the bugs from reappearing.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

Can you also add some tests?

I think many bugs are removed, so, I will add tests to cover the code.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2019

I am observing there are still few bugs which needs to be corrected. I am working on it along with tests. Apologies for delayed response. I was working on other PRs during this time, so couldn't get back to this.

@Upabjojr

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

I am observing there are still few bugs which needs to be corrected.

That's the point of adding tests for everything. Developers usually detect and fix lots of bugs while writing the tests.

@czgdp1807 czgdp1807 changed the title Some general bug fixes for sympy.stats.joint_rv [WIP]Some general bug fixes for sympy.stats.joint_rv Jun 12, 2019

@czgdp1807 czgdp1807 changed the title [WIP]Some general bug fixes for sympy.stats.joint_rv [WIP]Some general bug fixes for sympy.stats Jun 12, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

@ritesh99rakesh Are you going to remove the MarginalDistribution and CompoundDistribution in your new implementations of CompoundPSpace. If yes, then I will stop my work of fixing those classes in this PR.
AFAICFO, the docs in the current CompoundDistribution is not good for maintaining the code, so I think they need to be re-implemented. It's pretty difficult for me to figure out which line of code does what. A simple change breaks many things.

@ritesh99rakesh

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

@ritesh99rakesh Are you going to remove the MarginalDistribution and CompoundDistribution in your new implementations of CompoundPSpace. If yes, then I will stop my work of fixing those classes in this PR.
AFAICFO, the docs in the current CompoundDistribution is not good for maintaining the code, so I think they need to be re-implemented. It's pretty difficult for me to figure out which line of code does what. A simple change breaks many things.

Yes, @czgdp1807 I am working on it. I too felt that some of your changes here were not necessary since they would anyways be changed.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

Yes, @czgdp1807 I am working on it. I too felt that some of your changes here were not necessary since they would anyways be changed.

Thank you. I will then stop my work on CompoundDistribution, MarginalDistribution. I will make some docs changes and then this PR will be complete.

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.