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

Added StochasticProcess and DiscreteMarkovChain #16981

Merged
merged 11 commits into from Jun 13, 2019

Conversation

Projects
None yet
6 participants
@czgdp1807
Copy link
Member

commented Jun 6, 2019

References to other Issues or PRs

[1] #16895

Brief description of what is fixed or changed

StochasticPSpace has been added to sympy.stats.stochastic_process, StochasticProcess and DiscreteMarkovChain has been added to sympy.stats.stochastic_process_types.
Associated tests have also been added.
Currently the DiscreteMarkovChain handles only probability queries More features, like expectation, joint_distribution will be done in other PRs so that it is easier to review.

Other comments

Following are the references for the additions,

  1. API - #16895 (comment)
  2. StochasticPSpace - #16895 (comment)
  3. RandomIndexedSymbol - #16852 (comment), https://github.com/sympy/sympy/pull/16866/files#r286833831

Release Notes

  • stats
    • StochasticProcess and DiscreteMarkovChain added to sympy.stats.
@sympy-bot

This comment has been minimized.

Copy link

commented Jun 6, 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.

Your release notes are in good order.

Here is what the release notes will look like:

  • stats
    • StochasticProcess and DiscreteMarkovChain added to sympy.stats. (#16981 by @czgdp1807)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.5.

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. -->
[1] https://github.com/sympy/sympy/issues/16895

#### Brief description of what is fixed or changed
`StochasticPSpace` has been added to `sympy.stats.stochastic_process`, `StochasticProcess` and `DiscreteMarkovChain` has been added to `sympy.stats.stochastic_process_types`.
Associated tests have also been added.
Currently the `DiscreteMarkovChain` handles only probability queries More features, like expectation, joint_distribution will be done in other PRs so that it is easier to review.

#### Other comments
Following are the references for the additions,

1. **API** - https://github.com/sympy/sympy/issues/16895#issuecomment-497649797
2. **StochasticPSpace** - https://github.com/sympy/sympy/issues/16895#issuecomment-498039663
3. **RandomIndexedSymbol** - https://github.com/sympy/sympy/pull/16852#issuecomment-493765433, https://github.com/sympy/sympy/pull/16866/files#r286833831
#### 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 -->
* stats
  * `StochasticProcess` and `DiscreteMarkovChain` added to `sympy.stats`.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@codecov

This comment has been minimized.

Copy link

commented Jun 6, 2019

Codecov Report

Merging #16981 into master will increase coverage by 0.148%.
The diff coverage is 70.175%.

@@              Coverage Diff              @@
##            master    #16981       +/-   ##
=============================================
+ Coverage   73.959%   74.108%   +0.148%     
=============================================
  Files          620       622        +2     
  Lines       160407    160741      +334     
  Branches     37637     37724       +87     
=============================================
+ Hits        118637    119123      +486     
+ Misses       36293     36187      -106     
+ Partials      5477      5431       -46

@divyanshu132 divyanshu132 added the stats label Jun 6, 2019

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

@czgdp1807 czgdp1807 added the GSoC label Jun 7, 2019

@czgdp1807 czgdp1807 changed the title [GSoC][WIP] Added StochasticProcess and DiscreteMarkovChain [WIP] Added StochasticProcess and DiscreteMarkovChain Jun 7, 2019

czgdp1807 added some commits Jun 7, 2019

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 8, 2019

@Upabjojr Please review the PR.

@czgdp1807 czgdp1807 changed the title [WIP] Added StochasticProcess and DiscreteMarkovChain Added StochasticProcess and DiscreteMarkovChain Jun 8, 2019

Show resolved Hide resolved sympy/stats/rv.py

czgdp1807 added some commits Jun 9, 2019

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

# extracting transition matrix and state space
trans_probs, state_space = self.trans_probs, self.state_space
if isinstance(given_condition, And):

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jun 10, 2019

Contributor

definitely a bad choice, you are limiting the expressions to And only. You should try to use "ask" in SymPy, which supports logical inference.

I'm not sure if this is simple. Can you have a look?

This comment has been minimized.

Copy link
@czgdp1807

czgdp1807 Jun 10, 2019

Author Member

I think that even if we use ask it will return either True or False. So to pass transition matrix or state space via given_condition, we will have to use And. Infact, if we try to make out a meaning of something like, Or(Eq(Probability(Eq(Y[0], 0)), S(1)/4), TransitionMatrix(Y, TO)), then, we will lead to a conflict that whether to use, Eq(Probability(Eq(Y[0], 0)), S(1)/4) or TransitionMatrix(Y, TO). And implies that we should use all the given_condition.
For making ask work, I will need to make a class that will inherit AssumptionKeys and then define extra keys in it like, transition_matrix and state_space. That also sounds good. However, which binary operator we should use if not And. I think using any other will just lead to conflicts. Isn't it?
Please let me know if you want something else.

This comment has been minimized.

Copy link
@czgdp1807

czgdp1807 Jun 10, 2019

Author Member

Or do you want something like this,

>>> X = DiscreteMarkovChain('X')
>>> TransitionMarix(X, matrix)
>>> ask(Q.transition_matrix(X, matrix))
True

But by the above way, transition probabilities aren't passed via given_condition, they have to be assumed separately, which conflicts with the API design.
Please give me an example to clearly understand your view.

This comment has been minimized.

Copy link
@Upabjojr

Upabjojr Jun 10, 2019

Contributor

Hi, maybe it's too complicated with "ask". Let's keep it the way you have written the code right now.

My concern is that there are expressions that are not really "And" objects, which may be more complicated. But let's ignore them for now.

This comment has been minimized.

Copy link
@oscarbenjamin

oscarbenjamin Jun 11, 2019

Contributor

The purpose of ask is to try to get a boolean value from the relations. This code is attempting to extract the transition probabilities from the relations so they need to be relations that don't evaluate to a boolean.

The code here will always look weird if if it needs to support this API where the transition probabilities can be provided as part of the query. There should be an independent API for that use case if it is useful.

czgdp1807 added some commits Jun 11, 2019

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2019

@Upabjojr I have made the changes. Please give reviews and if possible finalize it.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

I have made the suggested changes, please consider merging it , so that I can create another set of PRs for completing the other features of sotchastic processes. Thanks.

@Upabjojr Upabjojr merged commit f2e5233 into sympy:master Jun 13, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
sympy-bot/release-notes The release notes look OK
Details
@oscargus

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

Can you in upcoming PRs please increase the code coverage of this? Much of the non-covered code is exceptions (use raises(ExceptionName, lambda: code_to_raise_exception)), but those are also worthwhile. There is also a bit of other code which is currently not tested.

After trying to increase coverage in other parts of the code I can tell that it is not always easy for someone that has not written the code to reach all parts of the code (not even exceptions), and after a few iterations of other persons improving the code, some code (and exceptions) may be unreachable, which is even harder to confirm if there are not original tests for it.

@czgdp1807 czgdp1807 deleted the czgdp1807:stoch_process branch Jun 14, 2019

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

@oscargus Is there any way to run the codecov locally? I am unable to find them at https://docs.sympy.org/latest/modules/utilities/runtests.html

@oscarbenjamin

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

I do it with pytest like this:

$ pytest --cov=sympy.solvers.ode --cov-report=html sympy/solvers/tests/test_ode.py -m 'not slow'

That runs the not slow tests in test_ode.py and reports coverage for the ode module. The results in html are in a folder called htmlcov: open htmlcov/index.py in a web browser.

You need to pip install pytest, coverage and possibly pytest-cov first.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

Thanks.

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.