-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Particle samplers and traces for Emcee #1689
Particle samplers and traces for Emcee #1689
Conversation
An example:
|
There was a previous proposal about this: #659 |
pymc3/sampling.py
Outdated
@@ -83,7 +85,7 @@ def assign_step_methods(model, step=None, methods=(NUTS, HamiltonianMC, Metropol | |||
|
|||
def sample(draws, step=None, init='advi', n_init=200000, start=None, | |||
trace=None, chain=0, njobs=1, tune=None, progressbar=True, | |||
model=None, random_seed=-1): | |||
model=None, random_seed=-1, ): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing comma
pymc3/step_methods/particle.py
Outdated
return bij.rmap(apoint) | ||
|
||
|
||
class EmceeSamplerStep(ParticleStep): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scratch the last comment (deleted), it is using emcee directly. In that case, I think this should be in pymc3.external
, just as the Edward extension is.
Thanks for the PR; I had a quick peek at the code and will look at it in depth later. I'm delighted to see the addition of particle samplers, in general. As I commented above, my own preference would be to put the |
pymc3/step_methods/particle.py
Outdated
return bij.rmap(apoint) | ||
|
||
|
||
class EmceeSamplerStep(ParticleStep): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our convention is not to include Step
in the name of user-facing sampler classes, so I suggest renaming to simply Emcee
.
pymc3/step_methods/particle.py
Outdated
|
||
f = theano.function([inarray0], logp0) | ||
f.trust_input = True | ||
return f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line
I'll get onto those suggestions listed above, but I'm thinking now that there could be a factory function to build a step around an external sampler. This would remove the dependency on emcee and allow easy addition of other samplers by user, no? |
@philastrophist Very cool. Did you look at #1569? |
Very cool stuff @philastrophist. A PhD student I mentored at a previous job was very upset that particle samplers weren't in PyMC3 - I'll let him know. |
@philastrophist also needs tests and an example NB for the docs. |
Now the particle samplers can use the Not read #1569 yet but plan to see how it works. Tests and the NB are underway. For now I'll update the example above. There is currently somewhat more lines than strictly necessary in All in all it's basically done and works. What I'm currently doing is making sure it can do all the things current samplers can whilst incorporating some flexibility for people to add other P.S. This started out as something I did for myself and I'm doing it when I have time. So it may take time for me to finish everything! |
@philastrophist Seems like you need to rebase. |
Great, just remove the WIP from the tag and title when you want a pre-merge review. |
@philastrophist Any progress? |
Currently the way this method works is quite restrictive on how the sampler works. So it only works for 1 particle sampler at a time. I think this is fine for now since any further development will need the multindtrace. |
b6007e6
to
6d18ecf
Compare
I have made some subtle Since this is a minor problem with a change in api, I think now is the time to remove WIP (since it works for any of my cases) and start some discussion... PS. A notebook for an intro into how to use |
pymc3/sampling.py
Outdated
return start, step | ||
def transform_start_particles(start, nparticles, model=None): | ||
""" | ||
:param start: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These doc-strings need to be made numpy-style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's my editor's default. Will change
pymc3/sampling.py
Outdated
start = start[0] | ||
else: | ||
raise NotImplemented('Initializer {} is not supported.'.format(init)) | ||
def get_random_starters(nwalkers, model=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this only be used with a particle sampler? Seems like the function is more general than that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I'll move it outside the particlestep block
pymc3/sampling.py
Outdated
|
||
return start, step | ||
def transform_start_particles(start, nparticles, model=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be moved to external/emcee?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this should stay here since it's applicable to specifying jobs and other particle steps not just emcee
I think this looks good overall. I'm a bit concerned of the added code complexity but the particle stuff should be usable by other samples except emcee too. The changes to the API make sense. It does need tests however. |
Thanks for the feedback. I've made some comments, but what in particular are you concerned about with added code complexity? The additions to With respect to Tests will be implemented of course, I just wanted to get the api out of the way first. |
---------- | ||
varname : str | ||
burn : int | ||
thin : int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing particles doc
c5294ac
to
a03a6f8
Compare
There is a test:
I'm not quite sure what to make of this. Other than that I think I have, or can easily, fix the errors raised in testing for this branch. Now I'll just put in a |
@philastrophist Any progress? |
Still going... Rectifying errors |
`transform_start_positions` now accounts for njobs
Tests just underway |
Just wondering - what's the status with the Emcee sampler? Excited to test this out on our data. |
Good question, would need to ask @philastrophist. @reac2 is your model non-differentiable? |
Thanks - will do! |
Have you tried the new ATMCMC sampler? |
Ah, no I was looking for ATMCMC and thought it had been removed. That's great - I will try this one out! Thanks very much. |
Is there an example for the ATMCMC code you can point me to? Our model is similar in form to the arbitrary deterministic disasters example so i'm starting there but running into trouble with redefining the likelihood_name parameter in smc.SMC. |
I'd check out the test case in |
Great, found it! Thanks. I'll see if I can get this going with the arbitrary deterministic disasters model. |
@reac2 you can also find an example here fitting a mixed-effect model https://github.com/junpenglao/GLMM-in-Python/blob/master/pymc3_different_sampling.py#L86-L113 |
Please see #2253 for the up-to-date refactored version. I consider this pull request outdated and will close it. |
So one of the major gripes I have with pymc3 is that I couldn't use the wonderful emcee from @dfm's emcee sampler very easily with the flexible model specification that pymc3 has. So I've added a generic particle sampler and an emcee sampler along with a trace to be used for particles.
This is somewhat hacky, but it works. There are obvious improvements to be made and a modified emcee sampler could be built in to avoid those pesky reshapes!
However, I have a limited knowledge of the workings of pymc3 despite writing this and so I'm not sure how to incorporate initialisations correctly for the walkers. I want to either use the advi and randomise the result for the walkers, or generate samples from the specified priors (currently doing this).
It'll be great if someone else could look at this!
Thanks
EDIT: Implemented some suggestions and improvements. Still a bit hacky, but I have some ideas