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

Finished base functionality for plotting probability spaces. #1

Merged
merged 14 commits into from Jun 20, 2017

Conversation

ajboyd2
Copy link
Contributor

@ajboyd2 ajboyd2 commented Apr 10, 2017

No description provided.

@@ -26,6 +33,28 @@ def sim(self, n):
"""
return Results(self.draw() for _ in range(n))

def plot(self, type = None, alpha = None, xlim = None, **kwargs):
if (self.pf == None):
raise Exception("Probabiltiy Space is not supported for plotting probability distribution.")
Copy link
Owner

Choose a reason for hiding this comment

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

Check spelling

if (self.pf == None):
raise Exception("Probabiltiy Space is not supported for plotting probability distribution.")

if (xlim == None): # if no limits for x-axis are specified, then use the default from plt
Copy link
Owner

Choose a reason for hiding this comment

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

if xlim is None


from .probability_space import ProbabilitySpace

# Helper function for combinations
def ncr(n, r):
Copy link
Owner

Choose a reason for hiding this comment

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

Not necessary, see comment below.

@@ -93,7 +136,15 @@ class NegativeBinomial(ProbabilitySpace):
def __init__(self, r, p):
self.r = r
self.p = p
self.discrete = True
self.pf = lambda x: ncr(x-1, r-1)*((1-p)**(x-r))*(p**r)
Copy link
Owner

Choose a reason for hiding this comment

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

Use Scipy's pmf, but add r before plugging into formula.

"""

def __init__(self, draw):
def __init__(self, draw, pf = None, discrete = False):
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of adding pf and discrete to ProbabilitySpace class, create a Distribution subclass with these kwargs, and all the distributions will be instances of that. Only Distribution supports plotting, not ProbabilitySpace. I think you can revert probability_space.py to how it was.


def plot(self, type = None, alpha = None, xlim = None, **kwargs):
if (xlim == None): # if no limits for x-axis are specified, then use the default from plt
Copy link
Owner

Choose a reason for hiding this comment

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

Can you make these style changes throughout the file?


self.discrete = discrete

self.xlim = [
Copy link
Owner

Choose a reason for hiding this comment

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

This should be a tuple rather than a list.

]

def plot(self, type = None, alpha = None, xlim = None, **kwargs):
if xlim is None: # if no limits for x-axis are specified, then use the default from plt
Copy link
Owner

@dlsun dlsun Apr 22, 2017

Choose a reason for hiding this comment

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

xlower, xupper = xlim if xlim is not None else self.xlim

if self.discrete:
xlower = int(xlower)
xupper = int(xupper)
xvals = list(np.arange(xlower, xupper+1, 1))
Copy link
Owner

@dlsun dlsun Apr 22, 2017

Choose a reason for hiding this comment

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

Is there any reason you're casting this to a list instead of keeping it as a Numpy array?

Also, don't include the step when it's equal to 1. So just np.arange(xlower, xupper+1)

else:
xvals = list(np.linspace(xlower, xupper, 100))

yvals = list(map(self.pdf, xvals))
Copy link
Owner

Choose a reason for hiding this comment

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

Can you use vectorization here? You should be able to just do self.pdf(xvals), right?

pf (function): A function giving the exact probability
density (if continuous) or mass (if discrete) from the probability space
for a given input.
discrete (boolean): Describes if the probability space is discrete or not.
Copy link
Owner

@dlsun dlsun Apr 22, 2017

Choose a reason for hiding this comment

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

This addition should probably be reverted? Could you revert all changes to probability_space.py?

Copy link
Owner

@dlsun dlsun left a comment

Choose a reason for hiding this comment

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

This is looking very good! Thanks for your hard work on this, especially for catching other bugs, like the unnecessary import in plot.py. There are just a few minor issues before I merge this pull request.

@dlsun dlsun merged commit 5cbfe71 into dlsun:master Jun 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants