-
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
Support intersection between an Hpolytope and an ellipsoid #287
base: develop
Are you sure you want to change the base?
Support intersection between an Hpolytope and an ellipsoid #287
Conversation
* 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]>
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 have a few comments.
|
||
/// This class represents a polytope intersected with an ellipsoid | ||
/// \tparam Polytope Polytope Type | ||
/// \tparam CEllipsoid Ellipsoid Type |
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 you call it CEllipsoid instead of just Ellipsoid?
/// \tparam Polytope Polytope Type | ||
/// \tparam CEllipsoid Ellipsoid Type | ||
template <typename Polytope, typename CEllipsoid> | ||
class EllipsoidIntersectPolytope { |
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 not inherit from BallIntersectPolytope class? A lot of methods are the same. Also do we need this class at all or we can just use BallIntersectPolytope for both ball and ellipsoid intersections? The only difference I see is in the last 3 methods.
*grad_data = ((params.a_vec.coeff(i) - NT(1)) / x.getCoefficients().coeff(i)); | ||
grad_data++; | ||
} | ||
Point y(grad); |
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 are you creating a VT and then copy it to a Point instead of creating a Point and assign the desired values directly into it?
*grad_data = ((params.a_vec.coeff(i) - NT(1)) / x.getCoefficients().coeff(i)); | ||
grad_data++; | ||
} | ||
Point y(grad); |
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.
same here, is there a copy here? Can we avoid it?
template <typename NT> | ||
static NT compute(EllipsoidIntersectPolytope<Polytope, Ellipsoid<Point>> &P) | ||
{ | ||
return NT(1.5) * P.radius(); |
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.
Where 1.5 comes from?
parameters() : order(2), L(2), m(2), kappa(1) {}; | ||
|
||
parameters(unsigned int order_) : | ||
order(order), |
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 probably mean order(order_)
here
// Copyright (c) 2018-2023 Apostolos Chalkis | ||
// Copyright (c) 2020-2023 Elias Tsigaridas | ||
|
||
// Contributed and/or modified by Ioannis Iakovidis, as part of Google Summer of |
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.
Is this a typo?
906e119
to
f1abc36
Compare
This PR:
i) Provides a new C++ class that represents the intersection between an Hpolytope and an ellipsoid,
ii) Implements C++ examples on how to sample from the intersection between an Hpolytope and an ellipsoid,
iii) Exposes to R the new class using a new Rcpp module,
iv) Implements R example on how to sample from the intersection between an Hpolytope and an ellipsoid,
v) Provides the Dirichlet's distribution oracles in C++,
vi) Provides C++ examples to sample from the Dirichlet distribution,
vii) Implements R examples on how to sample from the Dirichlet distribution.