-
Notifications
You must be signed in to change notification settings - Fork 116
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
No-U-Turn Sampler in HMC #216
Conversation
@@ -29,10 +29,10 @@ dimension <- 50 | |||
facets <- 200 | |||
|
|||
# Create domain of truncation | |||
H <- gen_rand_hpoly(dimension, facets) | |||
H <- gen_rand_hpoly(dimension, facets, seed = 15) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a specific seed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some seeds, the polytope is not bounded and the script fails.
So, I fixed the seed to test a specific instance and check if something changes/fails
after a commit.
R-proj/src/sample_points.cpp
Outdated
@@ -242,7 +263,7 @@ void sample_from_polytope(Polytope &P, int type, RNGType &rng, PointList &randPo | |||
//' @param n The number of points that the function is going to sample from the convex polytope. | |||
//' @param random_walk Optional. A list that declares the random walk and some related parameters as follows: | |||
//' \itemize{ | |||
//' \item{\code{walk} }{ A string to declare the random walk: i) \code{'CDHR'} for Coordinate Directions Hit-and-Run, ii) \code{'RDHR'} for Random Directions Hit-and-Run, iii) \code{'BaW'} for Ball Walk, iv) \code{'BiW'} for Billiard walk, v) \code{'dikin'} for dikin walk, vi) \code{'vaidya'} for vaidya walk, vii) \code{'john'} for john walk, viii) \code{'BCDHR'} boundary sampling by keeping the extreme points of CDHR or ix) \code{'BRDHR'} boundary sampling by keeping the extreme points of RDHR x) \code{'HMC'} for Hamiltonian Monte Carlo (logconcave densities) xi) \code{'ULD'} for Underdamped Langevin Dynamics using the Randomized Midpoint Method xii) \code{'ExactHMC'} for exact Hamiltonian Monte Carlo with reflections (spherical Gaussian or exponential distribution). The default walk is \code{'aBiW'} for the uniform distribution or \code{'CDHR'} for the Gaussian distribution and H-polytopes and \code{'BiW'} or \code{'RDHR'} for the same distributions and V-polytopes and zonotopes.} | |||
//' \item{\code{walk} }{ A string to declare the random walk: i) \code{'CDHR'} for Coordinate Directions Hit-and-Run, ii) \code{'RDHR'} for Random Directions Hit-and-Run, iii) \code{'BaW'} for Ball Walk, iv) \code{'BiW'} for Billiard walk, v) \code{'dikin'} for dikin walk, vi) \code{'vaidya'} for vaidya walk, vii) \code{'john'} for john walk, viii) \code{'BCDHR'} boundary sampling by keeping the extreme points of CDHR or ix) \code{'BRDHR'} boundary sampling by keeping the extreme points of RDHR x) \code{'NUTS'} for NUTS Hamiltonian Monte Carlo sampler (logconcave densities) xi) \code{'HMC'} for Hamiltonian Monte Carlo (logconcave densities) xii) \code{'ULD'} for Underdamped Langevin Dynamics using the Randomized Midpoint Method (logconcave densities) xiii) \code{'ExactHMC'} for exact Hamiltonian Monte Carlo with reflections (spherical Gaussian or exponential distribution). The default walk is \code{'aBiW'} for the uniform distribution, \code{'CDHR'} for the Gaussian distribution and H-polytopes and \code{'BiW'} or \code{'RDHR'} for the same distributions and V-polytopes and zonotopes. \code{'NUTS'} is the default sampler for logconcave densities.} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is too long. Can we split it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will do so. Thank you.
@@ -56,6 +56,8 @@ struct LeapfrogODESolver { | |||
pts xs; | |||
pts xs_prev; | |||
|
|||
Point grad_x; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is probably something going on with the solver. I run one of the examples at examples/logconcave/simple_hmc.cpp
(I also added NUTS example which I am going to push in a PR).
While NUTS seemed to return the correct marginals (in 2D), HMC returned the following wrong marginal.
The density is π(x) \propto exp(-f(x)) with f(x) = 2 x^T x + x.sum() for x belonging to a 2D-cube.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, have your changes affected how eta is set on vanilla hmc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this comment.
I think the current commit works properly considering HMC.
I implemented the same optimization as in NUTS for the number of evaluations of the gradient.
In particular, the second evaluation in the leapfrog's step is used for the first momenta update in the next leapfrog step.
I did not change anything in HMC parameterization.
randPoints[0] = p; | ||
|
||
listOfPoints.push_back(randPoints); | ||
//std::cout<<(listOfPoints[i])[0].getCoefficients().transpose()<<std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove unused lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I did so.
//std::cout<<"length = "<<listOfPoints.size()<<std::endl; | ||
NT L = std::numeric_limits<NT>::lowest(), Ltemp; | ||
|
||
for (int i=0; i<rnum-1; i++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you just try subsequent points, ignoring the times the sampler stays at a place. This way you would also not need to store the points.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but what about not subsequent points?
@@ -125,8 +125,10 @@ struct HamiltonianMonteCarloWalk { | |||
// Pick a random velocity | |||
v = GetDirection<Point>::apply(dim, rng, false); | |||
|
|||
solver->set_state(0, x); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above. HMC returns wrong marginals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the current commit fixes this issue.
@@ -0,0 +1,384 @@ | |||
// VolEsti (volume computation and sampling library) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you also tried to involve the average number of reflections on the burn-in method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I did not. I added a TODO comment to generalize Nesterov's algorithm in the truncated setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TolisChal Thank you for the PR! NUTS seems functional from the simple examples I have tried it on (we should also try more examples)!
However, the HMC walk seems broken and returns the wrong marginals (see more on the corresponding comment), which I think is due to the changes in the leapfrog integrator.
Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi all,
The functionality of vanilla HMC seems to be restored.
Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR. That's a really cool feature! I have a few comments on the code.
# Sample points | ||
n_samples <- 20000 | ||
|
||
samples <- sample_points(P, n = n_samples, random_walk = list("walk" = "NUTS", "solver" = "leapfrog", "starting_point" = warm_start[,1]), distribution = list("density" = "logconcave", "negative_logprob" = f, "negative_logprob_gradient" = grad_f)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you reduce the length here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. Thanks
@@ -79,6 +79,7 @@ class point | |||
void set_dimension(const unsigned int dim) | |||
{ | |||
d = dim; | |||
coeffs.setZero(d); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now this is not just setting a new dimension but also reset the whole vector/point. Should this be renamed to resize_point
or something?
include/ode_solvers/leapfrog.hpp
Outdated
@@ -101,24 +103,25 @@ struct LeapfrogODESolver { | |||
|
|||
void step(int k, bool accepted) { | |||
num_steps++; | |||
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove trailing spaces
NT epsilon_=2) | ||
{ | ||
epsilon = epsilon_; | ||
if (F.params.L > 0){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or simpler eta = F.params.L > 0 ? 10.0 / (dim * sqrt(F.params.L)) : 0.005;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
accepted = false; | ||
|
||
// Initialize solver | ||
solver = new Solver(0, params.eta, pts{x, x}, F, bounds{P, NULL}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use a smart pointer here to avoid memory leaks
|
||
NT uu = std::log(rng.sample_urdist()) - h1; | ||
int j = -1; | ||
bool s = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is s
, does it make sense to give a more descriptive name ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comes from the paper. In general, I use the variable names from the nuts paper
> | ||
static void apply(Polytope &P, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is an fix, unrelated to NUTS.
@@ -298,6 +298,11 @@ else () | |||
COMMAND logconcave_sampling_test -tc=uld) | |||
add_test(NAME logconcave_sampling_test_exponential_biomass_sampling | |||
COMMAND logconcave_sampling_test -tc=exponential_biomass_sampling) | |||
add_test(NAME logconcave_sampling_test_nuts_hmc_truncated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be helpful to also add a C++ example of NUTS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vissarion @TolisChal I have already opened this PR to Tolis' fork with a NUTS example: TolisChal#29
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! It seems that you've resolved the mkl
linking issue in examples ;-)
e.g. as described here #214
Hi @TolisChal what is the status of this PR? Should you merge it after fixing the conflicts? |
* Enable github actions to build examples. Avoid passing a polytope as a const reference. * Fix ambiguous call to fix function by renaming volesti's diagnostic function. (GeomScale#263) * Updating documentation (GeomScale#261) Adding WSL and MKL build instructions. * disable an R sampling test for windows * Fix the warning message in R Mac's cran test (GeomScale#285) * copy and replace lp_rlp.h * remove re-initialization of eta * update ubuntu version from 18 to 20 in R cran tests * minor improvements (explanatory comments) --------- Co-authored-by: Apostolos Chalkis <[email protected]> * delete commented out code * No-U-Turn Sampler in HMC (GeomScale#216) * initialize nuts sampler class * implement the burnin of nuts sampler * add tests and resolve bugs * implement e0 estimation in burnin of nuts * optimize leapfrog * integrate nuts into the R interface * document random walk in sample_points.cpp in R interface * fix burnin for the non-truncated case * resolve bugs in hmc and nuts pipelines * improve the preprocess in burin step of nuts * split large lines in sample_points.cpp * Add NUTS C++ example and update CMake (GeomScale#29) * resolve PR comments * fix minor bug * fix compiler bug * fix error in building the C++ examples * resolve warnings in sample_points * fix lpsolve cran warning * fix cran warning on mac * improve lpsolve cmake for cran check * fix R warning in mac test * remove lpsolve header * resolve PR comments --------- Co-authored-by: Marios Papachristou <[email protected]> Co-authored-by: Apostolos Chalkis <[email protected]> --------- Co-authored-by: Vissarion Fisikopoulos <[email protected]> Co-authored-by: Soumya Tarafder <[email protected]> Co-authored-by: Apostolos Chalkis <[email protected]> Co-authored-by: Marios Papachristou <[email protected]>
* initialize nuts sampler class * implement the burnin of nuts sampler * add tests and resolve bugs * implement e0 estimation in burnin of nuts * optimize leapfrog * integrate nuts into the R interface * document random walk in sample_points.cpp in R interface * fix burnin for the non-truncated case * resolve bugs in hmc and nuts pipelines * improve the preprocess in burin step of nuts * split large lines in sample_points.cpp * Add NUTS C++ example and update CMake (#29) * resolve PR comments * fix minor bug * fix compiler bug * fix error in building the C++ examples * resolve warnings in sample_points * fix lpsolve cran warning * fix cran warning on mac * improve lpsolve cmake for cran check * fix R warning in mac test * remove lpsolve header * resolve PR comments --------- Co-authored-by: Marios Papachristou <[email protected]> Co-authored-by: Apostolos Chalkis (TolisChal) <[email protected]>
* initialize nuts sampler class * implement the burnin of nuts sampler * add tests and resolve bugs * implement e0 estimation in burnin of nuts * optimize leapfrog * integrate nuts into the R interface * document random walk in sample_points.cpp in R interface * fix burnin for the non-truncated case * resolve bugs in hmc and nuts pipelines * improve the preprocess in burin step of nuts * split large lines in sample_points.cpp * Add NUTS C++ example and update CMake (#29) * resolve PR comments * fix minor bug * fix compiler bug * fix error in building the C++ examples * resolve warnings in sample_points * fix lpsolve cran warning * fix cran warning on mac * improve lpsolve cmake for cran check * fix R warning in mac test * remove lpsolve header * resolve PR comments --------- Co-authored-by: Marios Papachristou <[email protected]> Co-authored-by: Apostolos Chalkis (TolisChal) <[email protected]>
* initialize nuts sampler class * implement the burnin of nuts sampler * add tests and resolve bugs * implement e0 estimation in burnin of nuts * optimize leapfrog * integrate nuts into the R interface * document random walk in sample_points.cpp in R interface * fix burnin for the non-truncated case * resolve bugs in hmc and nuts pipelines * improve the preprocess in burin step of nuts * split large lines in sample_points.cpp * Add NUTS C++ example and update CMake (#29) * resolve PR comments * fix minor bug * fix compiler bug * fix error in building the C++ examples * resolve warnings in sample_points * fix lpsolve cran warning * fix cran warning on mac * improve lpsolve cmake for cran check * fix R warning in mac test * remove lpsolve header * resolve PR comments --------- Co-authored-by: Marios Papachristou <[email protected]> Co-authored-by: Apostolos Chalkis <[email protected]>
* initialize nuts sampler class * implement the burnin of nuts sampler * add tests and resolve bugs * implement e0 estimation in burnin of nuts * optimize leapfrog * integrate nuts into the R interface * document random walk in sample_points.cpp in R interface * fix burnin for the non-truncated case * resolve bugs in hmc and nuts pipelines * improve the preprocess in burin step of nuts * split large lines in sample_points.cpp * Add NUTS C++ example and update CMake (#29) * resolve PR comments * fix minor bug * fix compiler bug * fix error in building the C++ examples * resolve warnings in sample_points * fix lpsolve cran warning * fix cran warning on mac * improve lpsolve cmake for cran check * fix R warning in mac test * remove lpsolve header * resolve PR comments --------- Co-authored-by: Marios Papachristou <[email protected]> Co-authored-by: Apostolos Chalkis (TolisChal) <[email protected]>
* initialize nuts sampler class * implement the burnin of nuts sampler * add tests and resolve bugs * implement e0 estimation in burnin of nuts * optimize leapfrog * integrate nuts into the R interface * document random walk in sample_points.cpp in R interface * fix burnin for the non-truncated case * resolve bugs in hmc and nuts pipelines * improve the preprocess in burin step of nuts * split large lines in sample_points.cpp * Add NUTS C++ example and update CMake (#29) * resolve PR comments * fix minor bug * fix compiler bug * fix error in building the C++ examples * resolve warnings in sample_points * fix lpsolve cran warning * fix cran warning on mac * improve lpsolve cmake for cran check * fix R warning in mac test * remove lpsolve header * resolve PR comments --------- Co-authored-by: Marios Papachristou <[email protected]> Co-authored-by: Apostolos Chalkis (TolisChal) <[email protected]>
* initialize nuts sampler class * implement the burnin of nuts sampler * add tests and resolve bugs * implement e0 estimation in burnin of nuts * optimize leapfrog * integrate nuts into the R interface * document random walk in sample_points.cpp in R interface * fix burnin for the non-truncated case * resolve bugs in hmc and nuts pipelines * improve the preprocess in burin step of nuts * split large lines in sample_points.cpp * Add NUTS C++ example and update CMake (#29) * resolve PR comments * fix minor bug * fix compiler bug * fix error in building the C++ examples * resolve warnings in sample_points * fix lpsolve cran warning * fix cran warning on mac * improve lpsolve cmake for cran check * fix R warning in mac test * remove lpsolve header * resolve PR comments --------- Co-authored-by: Marios Papachristou <[email protected]> Co-authored-by: Apostolos Chalkis (TolisChal) <[email protected]>
* initialize nuts sampler class * implement the burnin of nuts sampler * add tests and resolve bugs * implement e0 estimation in burnin of nuts * optimize leapfrog * integrate nuts into the R interface * document random walk in sample_points.cpp in R interface * fix burnin for the non-truncated case * resolve bugs in hmc and nuts pipelines * improve the preprocess in burin step of nuts * split large lines in sample_points.cpp * Add NUTS C++ example and update CMake (#29) * resolve PR comments * fix minor bug * fix compiler bug * fix error in building the C++ examples * resolve warnings in sample_points * fix lpsolve cran warning * fix cran warning on mac * improve lpsolve cmake for cran check * fix R warning in mac test * remove lpsolve header * resolve PR comments --------- Co-authored-by: Marios Papachristou <[email protected]> Co-authored-by: Apostolos Chalkis (TolisChal) <[email protected]>
* initialize nuts sampler class * implement the burnin of nuts sampler * add tests and resolve bugs * implement e0 estimation in burnin of nuts * optimize leapfrog * integrate nuts into the R interface * document random walk in sample_points.cpp in R interface * fix burnin for the non-truncated case * resolve bugs in hmc and nuts pipelines * improve the preprocess in burin step of nuts * split large lines in sample_points.cpp * Add NUTS C++ example and update CMake (#29) * resolve PR comments * fix minor bug * fix compiler bug * fix error in building the C++ examples * resolve warnings in sample_points * fix lpsolve cran warning * fix cran warning on mac * improve lpsolve cmake for cran check * fix R warning in mac test * remove lpsolve header * resolve PR comments --------- Co-authored-by: Marios Papachristou <[email protected]> Co-authored-by: Apostolos Chalkis (TolisChal) <[email protected]>
This PR implements the No-U-Turn Sampler (NUTS) in HMC:
-- Implements a new structure for the random walk.
-- Optimizes the number of gradient computations in both HMC and NUTS; it reduces this number to half per step.
-- Implements a new C++ test for NUTS in
./test/logconcave_sampling_test.cpp
, namelybenchmark_nuts_hmc
(for both truncated and non-truncated cases).-- Extends the R interface to expose the NUTS sampler in R.
-- Implements a new R example in
./R-proj/examples/logconcave/nuts_rand_poly.R
.This PR resolves the issue #123