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

Hamiltonian Monte Carlo with Energy Conserving Subsampling #905

Merged
merged 100 commits into from
Feb 11, 2021

Conversation

OlaRonning
Copy link
Member

Introduces HMCECS to numpyro.

This PR includes:

  • Taylor proxy [section 4.2];
  • Difference estimation method [section 4.1].

@fehiepsi
Copy link
Member

fehiepsi commented Feb 6, 2021

Can you come with some suggestions for test cases?

I think we can test if taylor_proxy returns correct results and estimate_likelihood returns values that are close to the true loglikelihood with taylor_proxy (in the sense that variance is small if we change subsample indices and latent values). I think Normal-Normal model is a good test case but we can increase the latent dimensions to a few. What do you think?

@OlaRonning
Copy link
Member Author

@fehiepsi those tests sound good. I'll add them in.

@OlaRonning
Copy link
Member Author

@fehiepsi should the python version in CI be bumped to 3.9? looks like it's supported by Github Actions.

@fehiepsi
Copy link
Member

fehiepsi commented Feb 7, 2021

should the python version in CI be bumped to 3.9

It is not needed. We typically want to test for the oldest Python version that we support.

numpyro/infer/hmc_gibbs.py Outdated Show resolved Hide resolved
numpyro/infer/hmc_gibbs.py Outdated Show resolved Hide resolved
numpyro/infer/hmc_gibbs.py Outdated Show resolved Hide resolved
numpyro/infer/hmc_gibbs.py Outdated Show resolved Hide resolved
numpyro/primitives.py Outdated Show resolved Hide resolved
numpyro/infer/hmc_gibbs.py Show resolved Hide resolved
numpyro/examples/datasets.py Show resolved Hide resolved
numpyro/examples/datasets.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
numpyro/infer/hmc_gibbs.py Outdated Show resolved Hide resolved
numpyro/infer/hmc_gibbs.py Outdated Show resolved Hide resolved
numpyro/infer/hmc_gibbs.py Show resolved Hide resolved
numpyro/infer/hmc_gibbs.py Outdated Show resolved Hide resolved
numpyro/infer/hmc_gibbs.py Show resolved Hide resolved
Copy link
Member

@fehiepsi fehiepsi left a comment

Choose a reason for hiding this comment

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

Looks clean to me! Thanks for addressing my comments, @OlaRonning! Please let me know if you want to add more tests in another PR or if this is ready to merge when the following comments are addressed.

numpyro/infer/hmc_gibbs.py Outdated Show resolved Hide resolved
numpyro/infer/hmc_gibbs.py Outdated Show resolved Hide resolved
numpyro/infer/hmc_gibbs.py Outdated Show resolved Hide resolved
numpyro/infer/hmc_gibbs.py Outdated Show resolved Hide resolved
@OlaRonning
Copy link
Member Author

Hi @fehiepsi, thanks for the reviews!

I'll add the test cases (proxy result and low variance) you suggested and then it's ready to be merged.

@fehiepsi fehiepsi merged commit e5cd8db into pyro-ppl:master Feb 11, 2021
@OlaRonning OlaRonning deleted the feature/ecs branch February 11, 2021 08:27
@OlaRonning OlaRonning mentioned this pull request Feb 11, 2021
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.

3 participants