search

STOR-i / GaussianProcesses.jl

A Julia package for Gaussian Processes
https://stor-i.github.io/GaussianProcesses.jl/latest/
Other
308 stars 53 forks source link

Added the No-U-Turn sampler #132

Open Red-Portal opened 4 years ago

Red-Portal commented 4 years ago

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?

chris-nemeth commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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.

chris-nemeth commented 4 years ago

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

chris-nemeth commented 4 years ago

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 commented 4 years ago

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

maximerischard commented 4 years ago

@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 commented 4 years ago

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