8000 Speed up random method for Multinominal · Issue #2811 · pymc-devs/pymc · GitHub
[go: up one dir, main page]
Skip to content

Speed up random method for Multinominal #2811

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.

Sign up for GitHub

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

Jump to bottom
Closed
junpenglao opened this issue Jan 21, 2018 · 7 comments
Closed

Speed up random method for Multinominal #2811

junpenglao opened this issue Jan 21, 2018 · 7 comments
Labels
beginner friendly enhancements

Comments

@junpenglao
Copy link
Member

Come across these two tweets, which might allow us to improve the speed of Multinominal random:
https://twitter.com/jeremyphoward/status/954871616277655552
https://twitter.com/RadimRehurek/status/928671225861296128
@ColCarroll
Copy link
Member

Or, if you have some experience with numpy, this PR has lost some steam...

numpy/numpy#9710

@junpenglao
Copy link
Member Author

Ah cool! Why is that PR not merged?

@ColCarroll
Copy link
Member

I think the ball is in numpy's court, though I really don't know enough Cython to reason about it. I'll ping that thread again and ask if there's something I could do.

@sharanry
Copy link
Contributor
sharanry commented Jan 24, 2018 •
edited
Loading

@junpenglao @ColCarroll Are we going to implement this at the pymc3 end?

@ColCarroll
Copy link
Member

I would vote to leave the code as is, unless the numpy implementation gets a speedup: I am not sure of a case where multinomial sampling is the bottleneck, such that the added complexity of implementing something on our side is worth it.

@springcoil
Copy link
Contributor

What is there to be done here? Replacing the np.random.choice with Radim's suggestion?

@ColCarroll
Copy link
Member

I am going to just close (feel free to reopen if disagree!)

I doubt this is the bottleneck for anyone's analysis, and implementing radim's makes the code less readable, and means we would not get automatic speedups if/when changes are merged upstream (though still no word on the numpy pr).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
beginner friendly enhancements
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@springcoil @ColCarroll @junpenglao @sharanry
0