-
Notifications
You must be signed in to change notification settings - Fork 125
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
Interior point methods for SOS optimization. #93
Conversation
ff68082
to
425f829
Compare
Final Status Google Summer of Code: Implemented
|
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 Bento for this PR! I am in general OK with merging but I have a few comments as a minimum effort to conform with existing code base.
return lhscb; | ||
} | ||
|
||
virtual ~LHSCB() {}; |
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.
please eliminate the use of virtual functions
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.
It is very natural to use virtual function. The Skajaa-Ye algorithm needs a Logarithmically-Homogeneous-Self-Concordant-Barrier (LHSCB) as input, which has to implement several functions. This could be the Interpolant Barrier for the dual SOS cone that we mostly use or any other barrier for the SOS cone or other conoes like the non-negative orthant or the PSD cone.
Can you please elaborate why we should implement this differently, considering that it is not a bottleneck in computation?
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.
It's a matter of design, it is not good practice to mix designs in a library. If it is difficult to change and since this part is not interacting a lot with the rest of the library I am OK to leave as is. In any case my main concern is performance.
include/sos/EnvelopeProblemSOS.h
Outdated
void initialize_problem(); | ||
}; | ||
|
||
#include "EnvelopeProblemSOS.tpp" |
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 just use hpp
for file format of all your header files as is in the library right now ?
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.
From what I've read it is better coding practice to use .tpp endings for the type of template classes that I am using. For consistency with the rest of code .cpp endings would be better, agreed.
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.
For consistency with the library as it is so far I recommend to use hpp
for header files too. In any case the parser does not care about file formats it is just a convention.
include/sos/EnvelopeProblemSOS.tpp
Outdated
@@ -0,0 +1,506 @@ | |||
// VolEsti (volume computation and sampling library) |
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.
this is an application of the SOS solver, right? Could it be an example instead (placed for instance in examples/EnvelopeProblemSOS
directory) since it has a lot of unfinished parts and TODOs ?
product_vector.segment(seg.first, seg.second - seg.first) = vec_seg; | ||
} | ||
return product_vector; | ||
} |
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.
add newline at the end of file
@@ -0,0 +1,2342 @@ | |||
#pragma once |
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.
see comments about external libraries
@@ -0,0 +1,157 @@ | |||
// VolEsti (volume computation and sampling library) |
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.
a better place for tests is in test
directory
include/sos/sos.cpp
Outdated
@@ -0,0 +1,90 @@ | |||
// VolEsti (volume computation and sampling library) |
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.
since this is a header only library maybe examples is a good place for this?
include/sos/CMakeLists.txt
Outdated
@@ -0,0 +1,52 @@ | |||
cmake_minimum_required(VERSION 3.15) |
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.
this file will not be able to generate a Makefile in other computers than yours, please remove explicit paths
include/sos/CMakeLists.txt
Outdated
add_executable(NonSymmetricConicOptimization ${TARGETS} utils.cpp) | ||
|
||
if (NOT BOOST_DIR) | ||
find_package(Boost COMPONENTS *boost usr/local/) |
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.
what version of boost do you require ?
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.
1.67 should be enough. Have not tracked exactly in which version the needed libraries where modified the last time.
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.
ok please at this info to the README
include/sos/CMakeLists.txt
Outdated
@@ -0,0 +1,52 @@ | |||
cmake_minimum_required(VERSION 3.15) |
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.
I have some comments/suggests for the CMakeLists.txt file to be more "useable".
I had to make some changes to make it work.
Besides Boost, the directory of Eigen should be defined
because, among others, you use the unsupported directory.
There is also a problem with openMP, some variables are not defined.
I suggest that we add something like the following in the CMakeLists.txt
###find_package(OpenMP REQUIRED)
OPTION (USE_OpenMP "Use OpenMP to enamble <omp.h>" ON)
set(OPENMP_LIBRARIES "/opt/local/lib")
set(OPENMP_INCLUDES "/opt/local/include")
Find OpenMP
if(APPLE AND USE_OpenMP)
if(CMAKE_C_COMPILER_ID MATCHES "Clang")
set(OpenMP_C "${CMAKE_C_COMPILER}")
set(OpenMP_C_FLAGS "-fopenmp=libomp -Wno-unused-command-line-argument")
set(OpenMP_C_LIB_NAMES "libomp" "libgomp" "libiomp5")
set(OpenMP_libomp_LIBRARY ${OpenMP_C_LIB_NAMES})
set(OpenMP_libgomp_LIBRARY ${OpenMP_C_LIB_NAMES})
set(OpenMP_libiomp5_LIBRARY ${OpenMP_C_LIB_NAMES})
endif()
if(CMAKE_CXX_COMPILER_ID MATCHES "Clang")
set(OpenMP_CXX "${CMAKE_CXX_COMPILER}")
set(OpenMP_CXX_FLAGS "-Wno-unused-command-line-argument")
set(OpenMP_CXX_LIB_NAMES "libomp" "libgomp" "libiomp5")
set(OpenMP_libomp_LIBRARY ${OpenMP_CXX_LIB_NAMES})
set(OpenMP_libgomp_LIBRARY ${OpenMP_CXX_LIB_NAMES})
set(OpenMP_libiomp5_LIBRARY ${OpenMP_CXX_LIB_NAMES})
endif()
endif()
if(USE_OpenMP)
find_package(OpenMP REQUIRED)
endif(USE_OpenMP)
if (OPENMP_FOUND)
include_directories("${OPENMP_INCLUDES}")
link_directories("${OPENMP_LIBRARIES}")
set (CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${OpenMP_C_FLAGS}")
set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${OpenMP_CXX_FLAGS}")
# set (CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${OpenMP_EXE_LINKER_FLAGS}")
endif(OPENMP_FOUND)
#######
Finally, we should variables to set the directories of
the intel compiler, python, and llvm
because they are not always live in the directories that you specify in the CMakeList.txt
Anyways, after these changes I managed to compile and run the code.
Added JSON parsing
* Add Boost Info
Google Summer of Code 2020
Added features, features for future improvements and weekly progress are listed in include/sos/README.md