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

[WIP] Add discrete/continuous support+more samplers #229

Closed
wants to merge 36 commits into from
Closed

[WIP] Add discrete/continuous support+more samplers #229

wants to merge 36 commits into from

Conversation

rrkarim
Copy link
Contributor

@rrkarim rrkarim commented Feb 19, 2020

addr #187 #171
PR should include:

  • Support for more samplers (the design should be discussed)
  • Support for compound step sampling (maybe it is hard one to implement because of over-reliance on batching in tfp)
  • Support for discrete var sampling
  • Support auto_assign_method based on a set of certain criterias (like in pymc3)
  • More reach trace for compound step
  • More tests (they suck for now)

Current user design of compound step for user looks like this:

# the easiest one with automatic method assignment
# for each free var
model = ... # we define the model
pm.sample(model(), sampler_type="compound")
# user defines the methods for selected vars in model
from pymc4.mcmc.samplers import NUTS, RandomWalkM
pm.sample(model(), 
    sampler_type="compound", 
    sampler_methods=[
        ("var", RandomWalkM), 
        ("mean", NUTS)
    ], 
    xla=True)

There is currently support for categorical distribution mcmc sampling with custom state function, implemented in pymc4/distributions/state_functions.py, but more distributions should be tested and supported.

  • Categorical
  • **should be added as its tested and supported

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB.

@rrkarim rrkarim changed the title [WIP] Add discrete/continuous support [WIP] Add discrete/continuous support+more samplers Feb 19, 2020
@rrkarim
Copy link
Contributor Author

rrkarim commented Feb 19, 2020

There are some problems with sampling discrete distributions with xla enabled. I will look at the issue closer.

@sshkhr sshkhr mentioned this pull request Feb 20, 2020
@rrkarim
Copy link
Contributor Author

rrkarim commented Mar 7, 2020

@junpenglao can you please provide some feedback on the issues #233, I've implemented the sampling of the models with categorical distribution (with support of new_state_fn) but the implementation still relies on CompounStep. I've just putted the tfp implementation of compound step in this branch. There is a brief notebook that will test sampling.

Also tests are not passing because of the notebooks, now they should be edited too, but it is going to be messy here.

@rrkarim
Copy link
Contributor Author

rrkarim commented Mar 19, 2020

Sorry for some notebooks edited, tests do not pass without it. There are also some issues with notebook testing, the radon notebook doesn't work on my local machine.

@rrkarim
Copy link
Contributor Author

rrkarim commented Mar 21, 2020

So basically, I had two ways of designing sampler methods. This PR implementation is similar to pymc3, which for me is more clear from user perspective. Another idea was to have MCMCSampler class with all the ops like sample, run_chains etc. and sampler subclasses that defined the parameters like kernel, adaptive_kernel, trace_fn and etc. Then _assign_default_methods method should be defined in MCMCSampler class (it is defined now in CompoundStep) so that user doesn't pass sampler_type argument, but the samplers are assigned automatically to each free var according to certain criterias. Basically, all the sampler methods are now the type of compound step. What I don't like about this design choice is how the parametrization for each sampler method is handled.

For now there is a lack of support for clever auto_assign_method which could assign appropriate methods based on set of criterias. And also, the trace for compound step is returning only target_log_prob, but ideally it could return mapping of bootstrap_results for each kernel used in compound step. I'm planning to implement them after some feedback will be given on current status, since I'm not sure if the implemented design is good enough.

@fonnesbeck
Copy link
Member

I assume Unititled1.ipynb is not supposed to be there.

@rrkarim rrkarim closed this Jul 28, 2020
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.

2 participants