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

Billiard walk #55

Merged
merged 14 commits into from
Feb 13, 2020
Merged

Billiard walk #55

merged 14 commits into from
Feb 13, 2020

Conversation

TolisChal
Copy link
Member

Add the billiard walk for uniform sampling and volume computation:

i) The walk is available for all the representations and provides a speed up for volume computation.
ii) Its mixing time is better than the existing random walks' mixing time and hence, it is recommended as the default random walk for uniform sampling.

@vissarion vissarion self-requested a review February 12, 2020 11:29
@vissarion vissarion added this to the 1.1.0 milestone Feb 12, 2020
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! I am ok with merging after addressing minor comments.

@@ -425,4 +439,51 @@ void hit_and_run_coord_update(Point &p,
p.set_coord(rand_coord, p[rand_coord] + bpair.first + kapa * (bpair.second - bpair.first));
}


template <class ConvexBody, class Point, class Parameters, typename NT>
void billiard_walk(ConvexBody &P, Point &p, NT diameter, std::vector<NT> &Ar, std::vector<NT> &Av, NT &lambda_prev,
Copy link
Member

Choose a reason for hiding this comment

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

whatever is not changed by the function should be const, e.g. ConvexBody P is not changed right?

include/convex_bodies/vpolytope.h Show resolved Hide resolved
lambda_prev = T;
return;
}
lambda_prev = 0.995 * pbpair.first;
Copy link
Member

Choose a reason for hiding this comment

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

0.995 is this for robustness? maybe define it as const variable

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

@@ -236,6 +246,7 @@ Rcpp::NumericMatrix sample_points(Rcpp::Nullable<Rcpp::Reference> P = R_NilValue
InnerBall = HP.ComputeInnerBall();
if (!set_mean_point) MeanPoint = InnerBall.first;
}
if (diam < 0.0) diam = 2.0 * std::sqrt(NT(dim)) * InnerBall.second;
Copy link
Member

Choose a reason for hiding this comment

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

you set this std::sqrt(NT(dim)) * InnerBall.second in various places in the code. It is better if you use everywhere comp_diam() So this is set only in one place in the code. That is, if we want to change it in the future we should only change it there. This applies to all types of convex bodies.

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

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! I am ok with merging!

@TolisChal TolisChal merged commit 9502fec into GeomScale:develop Feb 13, 2020
@TolisChal TolisChal deleted the billiard_sampling branch February 13, 2020 13:07
vissarion pushed a commit that referenced this pull request Oct 26, 2020
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.

2 participants