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

Closed
wants to merge 4 commits into from
Closed

Added BernoulliProcess #15058

wants to merge 4 commits into from

Conversation

akash9712
Copy link
Contributor

@akash9712 akash9712 commented Aug 8, 2018

References to other Issues or PRs

Brief description of what is fixed or changed

Modified BernoulliProcess to take in a name(Symbol/str), a probability
of success, and success(int), failure(int) as input. This returns an
object of StochasticProces, which can be indexed to return a single
distribution or a joint distribution depending on the number of indices.

Other comments

Release Notes

  • stats
    • Added BernoulliProcess

akash9712 and others added 3 commits August 8, 2018 21:15
Added support for naming of Indexed types and avoid passing unnecessary kw_args
Modified `BernoulliProcess` to take in a name(symbol/str), a probability
of success, and success(int), failure(int) as input. This returns an
object of `StochasticProces`, which can be indexed to return a single
distribution or a joint distribution depending on the number of indices.
@sympy-bot
Copy link

sympy-bot commented Aug 8, 2018

Hi, I am the SymPy bot (v125). I'm here to make sure this pull request has a release notes entry. Please read the guide on how to write release notes.

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


#### Brief description of what is fixed or changed
Modified `BernoulliProcess` to take in a name(`Symbol`/`str`), a probability
of success, and success(`int`), failure(`int`) as input. This returns an
object of `StochasticProces`, which can be indexed to return a single
distribution or a joint distribution depending on the number of indices.


#### Other comments


#### 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. If there is no release notes entry for this PR,
write "NO ENTRY". The bot will check your release notes automatically to see
if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* stats
    * Added `BernoulliProcess`
<!-- END RELEASE NOTES -->

Your release notes are in good order.

Here is what the release notes will look like:

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

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.

@akash9712
Copy link
Contributor Author

Ping @Upabjojr @jksuom

@@ -159,6 +159,10 @@ def __new__(cls, base, *args, **kw_args):

return Expr.__new__(cls, base, *args, **kw_args)

@property
def name(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this edit? I would not edit other modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just cherry pick one of the other commits that were already merged into the master, because it was needed in the PR. Should I remove this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better not edit other modules

@@ -330,3 +330,12 @@ def marginalise_out(self, expr, rv):

def __call__(self, *args):
return self.pdf(*args)

class StochasticProcess(Basic, NamedArgsMixin):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's an expression, should inherit Expr

Copy link
Contributor Author

@akash9712 akash9712 Aug 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inheriting from Expr would mean allowing arithmatic operations directly on the stochastic process instead of the indexed object. I am not sure if that would be correct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

StochasticProcess)
from sympy.functions.elementary.piecewise import Piecewise

class BernoulliProcess(StochasticProcess):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we need to follow the same approach of JointRandomSymbol. Maybe RandomProcess should be equivalent to JointRandomSymbol as a class?

I would also consider a probability space for random processes, that is, I would maintain the same existing structure/logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had considered the StochasticProcess class as a collection of random variables, and any finite number of indexed elements from could be used to make a joint distribution. Each indexed element itself would a random variable.
Can you please explain as to how the StochasticProcess class could be made similar to JointRandomSymbol, i.e, how to seperate the distibution and the probability space?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One way would be to represent it as a collection of distributions instead of random symbols.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we extract the collection of probability spaces when needed. For example the expression E(X[t], X[t-2]) would probably extract the probability spaces at t and t-2, thus creating the collection.

I suggest to keep the API as close as possible to the existing one, as it would avoid editing too much later.

@Abdullahjavednesar Abdullahjavednesar added stats PR: author's turn The PR has been reviewed and the author needs to submit more changes. labels Aug 27, 2018
@czgdp1807 czgdp1807 closed this Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: author's turn The PR has been reviewed and the author needs to submit more changes. stats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants