Generalize the rounding loop and support sparse computations in preprocessing routines #312

TolisChal · 2024-06-24T19:22:42Z

No description provided.

vfisikop

Great addition! I have a few comments.

vfisikop · 2024-06-25T09:21:41Z

examples/sampling-hpolytope-with-billiard-walks/sampler.cpp

+    MT E;
+    VT center;
+    bool converged;
+    std::tie(E, center, converged) = max_inscribed_ellipsoid<MT>(HP.get_mat(),


maybe better

std::tie(E, center, converged) = max_inscribed_ellipsoid<MT>(HP.get_mat(), HP.get_vec(), x0, max_iter, tol, reg);

to consume only 2 lines

Thanks, done

vfisikop · 2024-06-25T09:21:54Z

examples/sampling-hpolytope-with-billiard-walks/sampler.cpp

        throw std::runtime_error("max_inscribed_ellipsoid not converged");

-    MT A_ell = inscribed_ellipsoid_res.first.first.inverse();
+    MT A_ell = E;


can we avoid the copy here?

Thanks, done

vfisikop · 2024-06-25T09:23:00Z

include/convex_bodies/orderpolytope.h

@@ -638,6 +638,12 @@ class OrderPolytope {
        return false;
    }

+    // Apply linear transformation, of square matrix T^{-1}, in H-polytope P:= Ax<=b
+    // This is most of the times for testing reasons because it might destroy the sparsity
+    void linear_transformIt(MT const& T)


we should follow naming conventions here, so linear_transform_it or something similar

linear_transformIt is the name in every polytope class. This is why I used the same here.

vfisikop · 2024-06-25T09:25:17Z

include/preprocess/mat_computational_operator.h

+// Licensed under GNU LGPL.3, see LICENCE file
+
+
+#ifndef MATRIX_COMPUTATIONAL_OPERATOR_H


the header guard, the filename and the class is good to have the same name (to avoid confusion). Could you please change the name of the file?

I am wondering if it is simpler to just have two versions for each function (one for sparse and one for dense)
e.g.

inline static auto initialize_chol(MT const& M) { using DenseMT = Eigen::Matrix<NT, Eigen::Dynamic, Eigen::Dynamic>; using SparseMT = Eigen::SparseMatrix<NT>; if constexpr (std::is_same<MT, DenseMT>::value) { return std::unique_ptr<Eigen::LLT<MT>>(new Eigen::LLT<MT>()); } else if constexpr (std::is_same<MT, SparseMT>::value) { // avoid using new! std::unique_ptr<Eigen::SimplicialLLT<Eigen::SparseMatrix<NT>>> llt = std::unique_ptr<Eigen::SimplicialLLT<Eigen::SparseMatrix<NT>>>(new Eigen::SimplicialLLT<Eigen::SparseMatrix<NT>>()); llt->analyzePattern(mat); return llt; } else { \\...not supported\\ } }

This way there is not need to initialize structs with types and you just call the function.

I personally like more the style with structs :)

My main "concern" is that in other header files we use structs for compile time decisions so if we use if constexpr we introduce one more style in our code. Is there any particular reason for that?

vfisikop · 2024-06-25T09:26:04Z

include/preprocess/inscribed_ellipsoid_rounding.hpp

+    typename Point,
+    int ellipsopid_type = EllipsoidType::MAX_ELLIPSOID
+>
+std::tuple<MT, VT, NT> inscribed_ellipsoid_rounding(Polytope &P, 


does this function changes P ? Could it be a const&?

I didn't change anything here (remind you this was previously the max_inscribed_ellipsoid_roundin()). This function changes P as it rounds it by applying (T, shift) on it. It returns T, shift and the scalar value for the volume calculations.

It looks good to me I'd say. I don't see any reason to copy P and round it instead.

vfisikop · 2024-06-25T09:56:39Z

include/preprocess/mat_computational_operator.h

+        return std::unique_ptr<Spectra::DenseSymMatProd<NT>>(new Spectra::DenseSymMatProd<NT>(E));
+    }
+
+    inline static std::unique_ptr<Spectra::SymEigsSolver<NT, Spectra::SELECT_EIGENVALUE::BOTH_ENDS, 


we can use auto as return here

thanks, done

vfisikop · 2024-06-25T10:07:35Z

include/preprocess/mat_computational_operator.h

+
+    inline static std::unique_ptr<Spectra::DenseSymMatProd<NT>> get_mat_prod_op(MT const& E)
+    {
+        return std::unique_ptr<Spectra::DenseSymMatProd<NT>>(new Spectra::DenseSymMatProd<NT>(E));


please do not use new ;) You can use return std::make_unique<Spectra::DenseSymMatProd<NT>>(E)

thanks, done

vfisikop · 2024-06-25T10:08:25Z

include/preprocess/mat_computational_operator.h

+                                  Spectra::DenseSymMatProd<NT>>> get_eigs_solver(std::unique_ptr<Spectra::DenseSymMatProd<NT>> const& op,
+                                                                                 int const n)
+    {
+        return std::unique_ptr<Spectra::SymEigsSolver<NT, Spectra::SELECT_EIGENVALUE::BOTH_ENDS, 


this is too long.
What about

using SymEigsSolver = Spectra::SymEigsSolver < NT, Spectra::SELECT_EIGENVALUE::BOTH_ENDS, Spectra::DenseSymMatProd<NT> >; return std::unique_ptr<SymEigsSolver>(new SymEigsSolver(op.get(), 2, std::min(std::max(10, n/5), n)));

Thanks, done

vfisikop · 2024-06-25T10:09:26Z

include/preprocess/max_inscribed_ball.hpp

@@ -11,6 +11,8 @@
 #ifndef MAX_INNER_BALL
 #define MAX_INNER_BALL

+#include "mat_computational_operator.h"


please use the full path

thanks, done

vfisikop · 2024-06-25T10:32:40Z

test/CMakeLists.txt

@@ -296,6 +296,10 @@ add_test(NAME test_round_max_ellipsoid
          COMMAND new_rounding_test -tc=round_max_ellipsoid)
 add_test(NAME test_round_svd
          COMMAND new_rounding_test -tc=round_svd)
+add_test(NAME test_round_log_barrier_test


can we just call it rounding_test now?

…se_in_preprocessing

…rators

…ocessing routines (#312) * generalize rounding loop * support sparse cholesky operator * complete sparse support in max_inscribed_ball * complete sparse support in preprocesing * add sparse tests * change main rounding function name * improve explaining comments * resolve PR comments * changing the dates in copyrights * use if constexpr instead of SNIFAE * update the examples to cpp17 * update to cpp17 order polytope example * fix templating in mat_computational_operators * fix templating errors and change header file to mat_computational_operators --------- Authored-by: Apostolos Chalkis (TolisChal) <tolis.chal@gmail.com>

…ocessing routines (#312) * generalize rounding loop * support sparse cholesky operator * complete sparse support in max_inscribed_ball * complete sparse support in preprocesing * add sparse tests * change main rounding function name * improve explaining comments * resolve PR comments * changing the dates in copyrights * use if constexpr instead of SNIFAE * update the examples to cpp17 * update to cpp17 order polytope example * fix templating in mat_computational_operators * fix templating errors and change header file to mat_computational_operators

…ocessing routines (GeomScale#312) * generalize rounding loop * support sparse cholesky operator * complete sparse support in max_inscribed_ball * complete sparse support in preprocesing * add sparse tests * change main rounding function name * improve explaining comments * resolve PR comments * changing the dates in copyrights * use if constexpr instead of SNIFAE * update the examples to cpp17 * update to cpp17 order polytope example * fix templating in mat_computational_operators * fix templating errors and change header file to mat_computational_operators

…ocessing routines (#312) * generalize rounding loop * support sparse cholesky operator * complete sparse support in max_inscribed_ball * complete sparse support in preprocesing * add sparse tests * change main rounding function name * improve explaining comments * resolve PR comments * changing the dates in copyrights * use if constexpr instead of SNIFAE * update the examples to cpp17 * update to cpp17 order polytope example * fix templating in mat_computational_operators * fix templating errors and change header file to mat_computational_operators

Apostolos Chalkis added 7 commits June 21, 2024 16:06

generalize rounding loop

911e030

support sparse cholesky operator

85b1faf

complete sparse support in max_inscribed_ball

b3f75ba

complete sparse support in preprocesing

7e66e8c

add sparse tests

5df7093

change main rounding function name

d50b4cd

improve explaining comments

6051ebc

TolisChal requested a review from vfisikop June 24, 2024 19:24

vfisikop approved these changes Jun 25, 2024

View reviewed changes

Apostolos Chalkis added 8 commits June 25, 2024 07:57

Merge branch 'develop' into generalize_rounding_loop_and_support_spar…

e27854b

…se_in_preprocessing

resolve PR comments

25a39b1

changing the dates in copyrights

4168002

use if constexpr instead of SNIFAE

960969c

update the examples to cpp17

da7348d

update to cpp17 order polytope example

035cfe8

fix templating in mat_computational_operators

9b72f5f

fix templating errors and change header file to mat_computational_ope…

11b4948

…rators

vfisikop merged commit 5bc28d6 into GeomScale:develop Jun 27, 2024
25 of 27 checks passed

TolisChal deleted the generalize_rounding_loop_and_support_sparse_in_preprocessing branch July 7, 2024 03:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize the rounding loop and support sparse computations in preprocessing routines #312

Generalize the rounding loop and support sparse computations in preprocessing routines #312

TolisChal commented Jun 24, 2024

vfisikop left a comment

vfisikop Jun 25, 2024

TolisChal Jun 25, 2024

vfisikop Jun 25, 2024

TolisChal Jun 25, 2024

vfisikop Jun 25, 2024

TolisChal Jun 25, 2024

vfisikop Jun 25, 2024

vfisikop Jun 25, 2024

TolisChal Jun 25, 2024

vfisikop Jun 25, 2024

TolisChal Jun 25, 2024

vfisikop Jun 25, 2024

TolisChal Jun 25, 2024

vfisikop Jun 25, 2024

TolisChal Jun 25, 2024

vfisikop Jun 25, 2024

TolisChal Jun 25, 2024

vfisikop Jun 25, 2024

TolisChal Jun 25, 2024

vfisikop Jun 25, 2024

TolisChal Jun 25, 2024

		// Licensed under GNU LGPL.3, see LICENCE file


		#ifndef MATRIX_COMPUTATIONAL_OPERATOR_H

Generalize the rounding loop and support sparse computations in preprocessing routines #312

Generalize the rounding loop and support sparse computations in preprocessing routines #312

Conversation

TolisChal commented Jun 24, 2024

vfisikop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment