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] DynamicHMC support #549

Merged
merged 13 commits into from
Sep 23, 2018
Merged

[WIP] DynamicHMC support #549

merged 13 commits into from
Sep 23, 2018

Conversation

xukai92
Copy link
Member

@xukai92 xukai92 commented Sep 18, 2018

TODOs

  • figure out how to pass target accept rate and number of adaptations to DynamicHMC
  • clean up style
  • figure our how to handle unregistered dependencies
  • use Requires.jl to resolve the unregistered dependencies
  • change the keyword argument way to a new type for DynamicHMC

Summary
The way I'm current supporting DynamicHMC is through a keyword argument called implementation in sample, e.g. sample(model_func, DynamicNUTS(...)). See the test file for a complete example.

@yebai
Copy link
Member

yebai commented Sep 19, 2018

figure out how to pass target accept rate and number of adaptations to DynamicHMC

I think passing in number of adaptions is probably enough here. We can leave target accept rate as default.

@xukai92
Copy link
Member Author

xukai92 commented Sep 21, 2018

This PR is almost finished. One question on testing: for functionality which using Requires.jl, what's the best way to put them in our test system? Do we actually want to manually set up travis to install, say, DynamicHMC.jl related packages and do the tests?

@yebai
Copy link
Member

yebai commented Sep 21, 2018

This PR is almost finished. One question on testing: for functionality which using Requires.jl, what's the best way to put them in our test system? Do we actually want to manually set up travis to install, say, DynamicHMC.jl related packages and do the tests?

Perhaps we can install them from travis script for now?

@xukai92
Copy link
Member Author

xukai92 commented Sep 21, 2018

I wonder if the new environment thing can be used? If not we will do that manually.

@yebai
Copy link
Member

yebai commented Sep 21, 2018

I wonder if the new environment thing can be used? If not we will do that manually.

Let’s do this manually for now.

@trappmartin
Copy link
Member

I’m using runtest.jl to install packages only required for tests in MCMCChain. Adding Pkg.add to Travis didn’t work for me.

@yebai
Copy link
Member

yebai commented Sep 21, 2018

Adding Pkg.add to Travis didn’t work for me.

Hmm, strange. Any idea why?

@yebai
Copy link
Member

yebai commented Sep 21, 2018

Seems the issue is not on the Turing side -- something weird is going on with Stats.jl.

@mohamed82008
Copy link
Member

I’m using runtest.jl to install packages only required for tests in MCMCChain. Adding Pkg.add to Travis didn’t work for me.

The proper way to do this is using a test/REQUIRE file.

@trappmartin
Copy link
Member

I’m using runtest.jl to install packages only required for tests in MCMCChain. Adding Pkg.add to Travis didn’t work for me.

The proper way to do this is using a test/REQUIRE file.

Can you refer to unregistered packages in the REQUIRE file?

@mohamed82008
Copy link
Member

If the unregistered package has a Project.toml, you can use add its UUID to the Manifest.toml file. The best example is probably in this link https://discourse.julialang.org/t/how-to-install-non-registered-dependencies-in-julia-0-7-1-0/13971/27

@trappmartin
Copy link
Member

Cool, thanks @mohamed82008!

@mohamed82008
Copy link
Member

Seems the issue is not on the Turing side -- something weird is going on with Stats.jl.

Seems related to the recent automatic capping https://discourse.julialang.org/t/package-compatibility-caps/15301. I think Stats was in the blind spot of the heuristic used: updated before July, does not use Compat, and has Julia 0.6 lower limit.

@mohamed82008
Copy link
Member

Closing and reopening again to trigger CI.

@xukai92 xukai92 closed this Sep 23, 2018
@xukai92 xukai92 reopened this Sep 23, 2018
@xukai92
Copy link
Member Author

xukai92 commented Sep 23, 2018

The vec_assume_mat.jl test fails (https://travis-ci.org/TuringLang/Turing.jl/jobs/431989506#L897-L949) on both the Linux Travis and AppVeyor (but is OK for macOS Travis).

@yebai yebai merged commit 26dea1e into master Sep 23, 2018
@yebai yebai deleted the dynamichmc-support branch September 23, 2018 22:39
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.

4 participants