ReHMC support for spectrahedra and general convex bodies with refactored Spectrahedra classes #194
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## develop #194 +/- ##
===========================================
- Coverage 55.83% 55.44% -0.40%
===========================================
Files 64 85 +21
Lines 3845 4801 +956
Branches 1687 2107 +420
===========================================
+ Hits 2147 2662 +515
- Misses 732 889 +157
- Partials 966 1250 +284
|
vissarion
reviewed
Dec 14, 2021
There was a problem hiding this comment.
Thanks for this PR! I have some comments.
Regarding coverage I think you should ask more tests to cover your code, then codecov will not complain.
Also, do you agree to merge #190 before this PR? Then your examples here could be simplified.
|
|
||
| // get the minimum positive eigenvalue of At^2 + Bt + C | ||
| EigenvaluesProblems<NT, MT, VT> quadraticEigenvaluesProblem; | ||
| NT distance = quadraticEigenvaluesProblem.minPosQuadraticEigenvalue(precomputedValues.A, precomputedValues.B, | ||
| NT distance = EigenvaluesProblem.minPosQuadraticEigenvalue(precomputedValues.A, precomputedValues.B, | ||
| precomputedValues.C, precomputedValues.X, |
There was a problem hiding this comment.
indentation seems wrong after this
| struct PrecomputationOfValues { | ||
|
|
||
| /// These flags indicate whether the corresponding matrices are computed | ||
| /// if yes, we can use them and not compute them fro scratch |
There was a problem hiding this comment.
OK I understand that the use of flags is not the intent of this PR but why not including a TODO in the comments to avoid the use of flags in the future?
| @@ -28,6 +28,8 @@ void print_diagnostics(MT const& samples, unsigned int &min_ess, StreamType &str | |||
| VT ess = effective_sample_size<NT, VT, MT>(samples, min_ess); | |||
| VT intv_psrf = interval_psrf<VT, NT, MT>(samples); | |||
|
|
|||
| //std::cout << "interval_psrf = " << intv_psrf.maxCoeff() << "us" << std::endl; | |||
|
|
||
| #include <../../external/arpack++/include/arssym.h> | ||
| //#include <../../external/arpack++/include/arssym.h> |
| /// ARPACK++ standard eigenvalues solver | ||
| #define ARPACK_EIGENVALUES_SOLVER | ||
| //#define ARPACK_EIGENVALUES_SOLVER |
| /// Samples random points from the spectrahedron from the Boltzmann distribution | ||
| /// \param[in] spectrahedron A spectrahedron | ||
| /// \param[in] interiorPoint A point in the interior of the spectrahedron | ||
| /// \param[in] pointsNum The number of points to sample | ||
| /// \param[out] points The list of the sampled points | ||
| /// \tparam Point class Point with NT and VT as declared above in this class | ||
| template <typename Point> | ||
| void apply(SPECTRAHEDRON &spectrahedron, Point const & interiorPoint, const unsigned int pointsNum, | ||
| void apply(ConvexBody &spectrahedron, Point const & interiorPoint, const unsigned int pointsNum, |
There was a problem hiding this comment.
the parameter name should be renamed as well, right? From spectahedron to convexbody
| /// \tparam Point | ||
| template <typename Point> | ||
| void getNextPoint(SPECTRAHEDRON &spectrahedron, VT &p, PrecomputedValues &precomputedValues) { | ||
| void getNextPoint(ConvexBody &spectrahedron, VT &p) { |
test/LpSolve.cmake
Outdated
| @@ -0,0 +1,41 @@ | |||
| function(GetLpSolve) | |||
There was a problem hiding this comment.
is this lpsolve related change needed in the tests?
test/logconcave_sampling_test.cpp
Outdated
| typedef Eigen::Matrix <NT, Eigen::Dynamic, Eigen::Dynamic> MT; | ||
| typedef Cartesian<NT> Kernel; | ||
| typedef typename Kernel::Point Point; | ||
| typedef Spectrahedron <Point> SPECTRAHEDRON; |
There was a problem hiding this comment.
to inline with the current writing typedef Spectrahedron<Point> Spectrahedron
test/logconcave_sampling_test.cpp
Outdated
| void call_test_benchmark_spectrahedra_grid_search() { | ||
|
|
||
| typedef Eigen::Matrix<NT, Eigen::Dynamic, 1> VT; | ||
| typedef Eigen::Matrix <NT, Eigen::Dynamic, Eigen::Dynamic> MT; |
Resolve PR comments and update tests
Resolve cran test errors
TolisChal
approved these changes
Jan 4, 2022
vissarion
reviewed
Jan 4, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Combines the following 2 PRs on a new PR with a renamed branch: