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

Oracles + python #7

Closed
wants to merge 11 commits into from
Closed

Oracles + python #7

wants to merge 11 commits into from

Conversation

van51
Copy link

@van51 van51 commented Dec 31, 2018

Added membership and boundary oracles, as well as python support for them and HPolytopes.

@vissarion vissarion requested a review from TolisChal January 9, 2019 08:29
@vissarion vissarion self-assigned this Jan 9, 2019
@vissarion vissarion removed their assignment Jan 9, 2019
@vissarion vissarion self-requested a review January 9, 2019 08:34
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 the PR!

I tried to compiled it locally but with no success. After adding a few missing includes in the python\compile script by first adding the corresponding libraries to external i.e. -I../external/faiss -I../external/Eigen -I/usr/include/python2.7 -I../external/pybind11/include I got a linking error /usr/bin/ld: cannot find -lfaiss.

Is faiss needed to use the new functionality? When removing it from compile script (remove -DUSE_FAISS and -lfaiss from g++ command I got the following errors

volesti_pybind11.cpp:60:22: error: ‘Polytope {aka class HPolytope<point<Cartesian<float> > >}’ has no member named ‘contains_point’
             return p.contains_point((NT* ) point_info.ptr);
                      ^~~~~~~~~~~~~~
In file included from volesti_pybind11.cpp:7:0:
volume_approximation/include/convex_bodies/polytopes.h: In instantiation of ‘Point HPolytope<Point>::compute_boundary_intersection(Point&, Point&, float, int) [with Point = point<Cartesian<float> >]’:
volesti_pybind11.cpp:99:100:   required from here
volume_approximation/include/convex_bodies/polytopes.h:227:46: error: ‘get_nearest_facet’ was not declared in this scope
             long nn_index = get_nearest_facet(x0.data());

A clear way of checking if your new feature compiles/passes tests is to add some commands here: https://github.com/van51/volume_approximation/blob/R_volesti/.circleci/config.yml and see if it compiles/passes the tests. In my machine all old tests are passed so there are no regressions, which is good.

@van51
Copy link
Author

van51 commented Jan 11, 2019

FAISS is needed for this functionality. I didn't want to add it to the externals directory as it would make the repo much larger unnecessarily. So for now I just assumed that it would be installed in the user's system. Alternatives would be:

  • To use it as a submodule (same could be done for some other libs in externals as well)
  • To have a download script

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 the python2 support!

With the following diff I managed to compile and run the tests locally

diff --git a/python/compile b/python/compile
index 60fb188..33ec222 100755
--- a/python/compile
+++ b/python/compile
@@ -4,7 +4,8 @@ IFS=$'\n'
 names=($(readlink -f ../include/*/))
 # Restore IFS
 IFS=$SAVEIFS
-includes='-I../include -I../external/LPsolve_src/run_headers -I../external/boost -I../external '
+includes='-I../include -I../external/LPsolve_src/run_headers -I../external/boost -I../external 
+-I../external/Eigen '
 for (( i=0; i<${#names[@]}; i++ ))
 do
     includes+="-I${names[$i]} "
diff --git a/python/volesti_pybind11.cpp b/python/volesti_pybind11.cpp
index 998bf50..7e18fab 100644
--- a/python/volesti_pybind11.cpp
+++ b/python/volesti_pybind11.cpp
@@ -3,7 +3,7 @@
 #include "cartesian_kernel.h"
 #include "solve_lp.h"
 #include "pybind11/numpy.h"
-#include <eigen3/Eigen/Eigen>
+#include <Eigen/Eigen>
 #include "polytopes.h"
 #include <iostream>

Did you assume eigen3 installation? There is also a comment on the code.
I am ok with merging when those minor fixes are done. @TolisChal what do you think?

for ( ; rit != r.iter_end(); rit++, vit++, j++){
sum_nom -= A(i, j) * (*rit);
sum_denom += A(i, j) * (*vit);
for (uint j=0; j<dimension(); j++){
Copy link
Member

Choose a reason for hiding this comment

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

Why not iterators?

TolisChal added a commit that referenced this pull request Jan 31, 2019
Volume approximation for V-polytopes
@TolisChal TolisChal closed this Mar 19, 2020
vissarion pushed a commit that referenced this pull request Oct 26, 2020
Volume approximation for V-polytopes
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.

3 participants