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

No-U-Turn Sampler in HMC #216

Merged
merged 26 commits into from
Oct 17, 2023
Merged

No-U-Turn Sampler in HMC #216

merged 26 commits into from
Oct 17, 2023

Conversation

TolisChal
Copy link
Member

@TolisChal TolisChal commented Mar 10, 2022

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, namely benchmark_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

@vissarion vissarion linked an issue Mar 10, 2022 that may be closed by this pull request
@@ -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)
Copy link
Collaborator

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?

Copy link
Member Author

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.

@@ -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.}
Copy link
Collaborator

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?

Copy link
Member Author

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;
Copy link
Collaborator

@papachristoumarios papachristoumarios Mar 10, 2022

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.

hmc_wrong_1

Copy link
Collaborator

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?

Copy link
Member Author

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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove unused lines.

Copy link
Member Author

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++)
Copy link
Collaborator

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.

Copy link
Member Author

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);
Copy link
Collaborator

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.

Copy link
Member Author

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)
Copy link
Collaborator

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?

Copy link
Member Author

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.

Copy link
Collaborator

@papachristoumarios papachristoumarios left a 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!

Copy link
Collaborator

@papachristoumarios papachristoumarios left a 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!

Copy link
Member

@vissarion vissarion left a 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))
Copy link
Member

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?

Copy link
Member Author

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);
Copy link
Member

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?

@@ -101,24 +103,25 @@ struct LeapfrogODESolver {

void step(int k, bool accepted) {
num_steps++;

Copy link
Member

Choose a reason for hiding this comment

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

please remove trailing spaces

include/ode_solvers/leapfrog.hpp Outdated Show resolved Hide resolved
include/ode_solvers/leapfrog.hpp Show resolved Hide resolved
NT epsilon_=2)
{
epsilon = epsilon_;
if (F.params.L > 0){
Copy link
Member

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;

Copy link
Member Author

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});
Copy link
Member

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;
Copy link
Member

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 ?

Copy link
Member Author

@TolisChal TolisChal Oct 17, 2023

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,
Copy link
Member

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
Copy link
Member

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.

Copy link
Collaborator

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

Copy link
Member

@vissarion vissarion Apr 8, 2022

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

@vissarion
Copy link
Member

Hi @TolisChal what is the status of this PR? Should you merge it after fixing the conflicts?

@TolisChal TolisChal merged commit 074a562 into GeomScale:develop Oct 17, 2023
27 checks passed
iakoviid pushed a commit to iakoviid/volesti that referenced this pull request Nov 1, 2023
* 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]>
@TolisChal TolisChal deleted the nuts_hmc branch June 21, 2024 19:14
TolisChal added a commit that referenced this pull request Jul 7, 2024
* 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]>
TolisChal added a commit that referenced this pull request Jul 7, 2024
* 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]>
TolisChal added a commit to TolisChal/volume_approximation that referenced this pull request Jul 7, 2024
* 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]>
TolisChal added a commit to TolisChal/volume_approximation that referenced this pull request Jul 7, 2024
* 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]>
TolisChal added a commit to TolisChal/volume_approximation that referenced this pull request Jul 8, 2024
* 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]>
TolisChal added a commit to TolisChal/volume_approximation that referenced this pull request Jul 8, 2024
* 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]>
TolisChal added a commit to TolisChal/volume_approximation that referenced this pull request Jul 8, 2024
* 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]>
TolisChal added a commit that referenced this pull request Jul 17, 2024
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement NUTS
3 participants