-
Notifications
You must be signed in to change notification settings - Fork 122
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
Billiard walk #55
Conversation
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! 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, |
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.
whatever is not changed by the function should be const
, e.g. ConvexBody P
is not changed right?
include/samplers/samplers.h
Outdated
lambda_prev = T; | ||
return; | ||
} | ||
lambda_prev = 0.995 * pbpair.first; |
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.
0.995
is this for robustness? maybe define it as const
variable
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
R-proj/src/sample_points.cpp
Outdated
@@ -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; |
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.
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.
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
…ors in doxumentation and update Rd files
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! I am ok with merging!
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.