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

FEAT: Add jitted function for drawing #391

Merged
merged 2 commits into from
Mar 7, 2018

Conversation

QBatista
Copy link
Member

@QBatista QBatista commented Mar 7, 2018

Adds a jitted function for drawing discrete random variables. Note that DiscreteRV.draw no longer uses check_random_state as Numba supports top-level functions from the numpy.random module, but does not allow you to create individual RandomState instances.

Close #390

@oyamad
Copy link
Member

oyamad commented Mar 7, 2018

Some comments:

  • Why not keep the DiscreteRV class as is, just adding the new jitted discrete_rv function? A DiscreteRV instance cannot be passed to a jitted function anyway (unless it is implemented as a jitclass).

  • To me, discrete_rv sounds like returning some object representing a discrete random variable. What about random_choice? Note that this is similar to np.random.choice, where Numba does not support the p option (see this).



@jit(nopython=True)
def discrete_rv(p_vals, cum_sum=None, size=1000, seed=0):
Copy link
Member

Choose a reason for hiding this comment

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

The default value for seed must be None.

@QBatista
Copy link
Member Author

QBatista commented Mar 7, 2018

I agree with those comments. @jstac What do you think?

@coveralls
Copy link

coveralls commented Mar 7, 2018

Coverage Status

Coverage decreased (-0.02%) to 94.893% when pulling 081163f on QBatista:jitted_discreterv into 49ad385 on QuantEcon:master.

@QBatista QBatista force-pushed the jitted_discreterv branch 2 times, most recently from dfacbc9 to b87ed1a Compare March 7, 2018 03:39
@jstac
Copy link
Contributor

jstac commented Mar 7, 2018

@QBatista Many thanks.
@mmcky Do you mind if we merge this now? It looks good for me and I need it to finish what I'm doing in the master branch of the lectures...

@jstac jstac merged commit 931fd11 into QuantEcon:master Mar 7, 2018
@jstac
Copy link
Contributor

jstac commented Mar 7, 2018

@mmcky I'm going to go ahead and merge this, i hope that's OK.

@oyamad oyamad mentioned this pull request Mar 8, 2018
oyamad added a commit that referenced this pull request Mar 11, 2018
This reverts commit 931fd11, reversing
changes made to 49ad385.
@oyamad oyamad mentioned this pull request Mar 11, 2018
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.

4 participants