Skip to content

Conversation

@aseyboldt
Copy link
Member

This should hopefully fix #1824 and #2307.

Copy link
Member

Choose a reason for hiding this comment

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

brief doc-string would be helpful.

Copy link
Member Author

Choose a reason for hiding this comment

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

coming up soon

Copy link
Member

Choose a reason for hiding this comment

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

why is that required?

Copy link
Member Author

Choose a reason for hiding this comment

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

draw_values used to return a list if it was passed more than one value and a single value otherwise. I didn't like that and change it to return always a list. I am playing around with sample_ppc a bit, and there it could be that the list of variable to draw is dynamic, which would make the previous behaviour somewhat strange. But I can change it back if you disagree.

Copy link
Member

Choose a reason for hiding this comment

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

I see, I don't disagree.

Copy link
Member

Choose a reason for hiding this comment

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

is it effect the speed? Since you commented in #2296 that draw_values might also cause slowdown in random method.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so

@aseyboldt
Copy link
Member Author

@hvasbath Can you verify that the speed issue is gone with this PR? It runs faster in a simple example I tried.

@hvasbath
Copy link
Contributor

I am not experienced enough with git to checkout this PR. Can you please post some command lines I would need to follow to try? Thanks!

@fonnesbeck
Copy link
Member

fonnesbeck commented Jun 16, 2017

@hvasbath The pattern is like this:

git fetch origin pull/<pr_number>/head:<local_name>
git checkout <local_name>

where local_name is whatever you want to call the PR on your machine.

@hvasbath
Copy link
Contributor

hvasbath commented Jun 17, 2017

Thanks @fonnesbeck!
With the test in #1824
Time on master:
('time for initialising population', 26.08741807937622)
Time with this PR:
('time for initialising population', 1.317948818206787)

So yeah almost as good as before! Good job @aseyboldt !
@junpenglao Now it doesnt compile a theano function anymore for each sample, which is why it is much faster again. It compiles the functions if there are theano.tensors involved- thats why it affects the speed.

@twiecki
Copy link
Member

twiecki commented Jun 17, 2017

Awesome, thanks for checking @hvasbath and sorry you went down that other rabbit hole. Will wait for the doc string and then merge.

@twiecki twiecki mentioned this pull request Jun 17, 2017
@aseyboldt
Copy link
Member Author

aseyboldt commented Jun 17, 2017

@twiecki I wrote a docstring and also made that function private, there really isn't any reason why someone should be using that instead of draw_values.

@hvasbath Good to hear and thanks for checking.

@twiecki
Copy link
Member

twiecki commented Jun 17, 2017

Awesome, can merge once tests pass.

@aseyboldt aseyboldt merged commit af2e68f into pymc-devs:master Jun 17, 2017
@aseyboldt aseyboldt deleted the fix-sample-ppc branch June 17, 2017 15:03
@hvasbath
Copy link
Contributor

Awesome that this is merged! It heavily affected SMC initialisation! @twiecki at least it was useful to draw @aseyboldt s attention. So that it is fixed in a much better and cleaner way now! No one can know everything right ;) . Thanks again for fixing @aseyboldt !

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.

major slowdown in distribution.random()

5 participants