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

Added the No-U-Turn sampler #132

Open
wants to merge 22 commits into
base: advancedHMC
Choose a base branch
from

Conversation

Red-Portal
Copy link
Contributor

@Red-Portal Red-Portal commented Dec 21, 2019

Added NUTS as nuts based on AdvancedHMC.
In order to expose all of AdvancedHMC's options without exposing too much of GaussianProcesses.jl's internals, had to create a routine called nuts_hamiltonian as a workaround.
Please check if you are okay with nuts_hamiltonian's uglinesss.

Also, added effective sample size diagnostics to every MCMC routines.
Since running all the diagnostics introduces a little bit of overhead,
adding a verbosity flag might be necessary?

Copy link
Contributor

@maximerischard maximerischard left a comment

Choose a reason for hiding this comment

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

It's great to see that it was relatively straight-forward to write an interface between GaussianProcesses.jl and AdvancedHMC.jl. I think many people (myself included) will find this useful. What I would suggest is that once the PR is complete, the interface should be moved to a separate mini-package. This enables this functionality without adding a large dependency to either package. Ideally, GaussianProcesses.jl should be “inference-agnostic” while making it as easy as possible for users to use other packages (such as AdvancedHMC or DynamicHMC). The exception to this would be inference methods that are particular to Gaussian processes (such as slice sampling).

src/mcmc.jl Outdated
"""
function mcmc(gp::GPBase; nIter::Int=1000, burn::Int=1, thin::Int=1, ε::Float64=0.1,

function effective_sample_size(X::AbstractMatrix)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does compute_ess need to be defined inside of effective_sample_size?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name is indeed confusing. I'll change it.

@chris-nemeth
Copy link
Member

chris-nemeth commented Jan 20, 2020

Sorry for the slow response. I think this got lost in the pre-Christmas madness. Thanks, @Red-Portal for putting this together. From what I can tell the code is working and the tests pass. I think before we merge this it would be good to add a verbose flag, as you've mentioned. Given the amount of output per iteration, I'd set verbose=false as the default.

Given that we now have a few inference methods, I think we should create an inference folder, as we already have for kernels. We can move mcmc.jl, optimize.jl and vi.jl to this new folder. @thomaspinder and I are also working on something else which could later be added to the inference toolbox.

@maximerischard - you mentioned creating a separate inference package. I think this could be a good idea. In the short term, we could merge the AdvancedHMC implementation into the master until we create the separate smaller package. What do you think?

@maximerischard
Copy link
Contributor

@chris-nemeth I would think twice before adding the AdvancedHMC.jl dependency to master, even temporarily. @Red-Portal you mentioned you wanted to make a few more changes to this PR, is that still something you're planning to do?

@Red-Portal
Copy link
Contributor Author

@maximerischard Yes, I'll remove the effective sample size calculation part as soon as I have some time. I'm currently in front of a deadline, so sorry for the delay.

@Red-Portal
Copy link
Contributor Author

@maximerischard @chris-nemeth Hi! I've removed the ESS calculation related dependencies as promised. About introducing the AdvancedHMC.jl dependency, please let me know what is to be done.

Red-Portal and others added 2 commits February 10, 2020 16:39
…20-02-06-12-02-04-853-2561327094

CompatHelper: bump compat for "RecipesBase" to "0.8"
@chris-nemeth
Copy link
Member

@maximerischard are you still thinking that a separate package for the inference methods is the best way forward?

@chris-nemeth
Copy link
Member

Hi @Red-Portal, I was looking to integrate this PR and I did a quick check to see if it passes the tests. Could you please take a look at the deprecation error from StanHMCAdaptor()?

@Red-Portal
Copy link
Contributor Author

@chris-nemeth Hi!
I fixed the deprecation warning and bumped the master branch.

@maximerischard
Copy link
Contributor

@maximerischard are you still thinking that a separate package for the inference methods is the best way forward?

I've been thinking about this some more over the weekend. I still think we should avoid having AdvancedHMC as a requirement in Project.toml. But I now think it would be simpler to use Requires.jl instead of creating a separate package. @Red-Portal , do you think that would be doable?

@Red-Portal
Copy link
Contributor Author

Red-Portal commented Apr 9, 2020

@maximerischard I'm not very familiar with you're suggesting. Can you elaborate?

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.

7 participants