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

Update Spectra to v1.0.1 to fix C++20 compatibility issues on GCC/Clang #1871

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

valgur
Copy link
Contributor

@valgur valgur commented Oct 11, 2024

GTSAM currently unfortunately fails to build with C++20 or newer on GCC and Clang with the following errors in ShonanAveraging.cpp:

  [ 70%] Building CXX object gtsam/CMakeFiles/gtsam.dir/sfm/ShonanAveraging.cpp.o
  In file included from /home/martin/libs/gtsam/gtsam/3rdparty/Spectra/SymEigsBase.h:20,
                   from /home/martin/libs/gtsam/gtsam/3rdparty/Spectra/SymEigsSolver.h:12,
                   from /home/martin/libs/gtsam/gtsam/sfm/ShonanAveraging.cpp:19:
  /home/martin/libs/gtsam/gtsam/3rdparty/Spectra/MatOp/internal/ArnoldiOp.h:108:50: error: expected ‘)’ before ‘*’ token
    108 |     ArnoldiOp<Scalar, OpType, IdentityBOp>(OpType* op, IdentityBOp* /*Bop*/) :
        |                                           ~      ^
        |                                                  )
  /home/martin/libs/gtsam/gtsam/3rdparty/Spectra/SymEigsBase.h: In instantiation of ‘Spectra::SymEigsBase<Scalar, SelectionRule, OpType, BOpType>::SymEigsBase(OpType*, BOpType*, Index, Index) [with Scalar = double; int SelectionRule = 0; OpType = gtsam::MatrixProdFunctor; BOpType = Spectra::IdentityBOp; Index = long int]’:
  /home/martin/libs/gtsam/gtsam/3rdparty/Spectra/SymEigsSolver.h:165:83:   required from ‘Spectra::SymEigsSolver<Scalar, SelectionRule, OpType>::SymEigsSolver(OpType*, Index, Index) [with Scalar = double; int SelectionRule = 0; OpType = gtsam::MatrixProdFunctor; Index = long int]’
  /home/martin/libs/gtsam/gtsam/sfm/ShonanAveraging.cpp:641:79:   required from here
  /home/martin/libs/gtsam/gtsam/3rdparty/Spectra/SymEigsBase.h:252:15: error: no matching function for call to ‘Spectra::ArnoldiOp<double, gtsam::MatrixProdFunctor, Spectra::IdentityBOp>::ArnoldiOp(gtsam::MatrixProdFunctor*&, Spectra::IdentityBOp*&)’
    252 |         m_fac(ArnoldiOpType(op, Bop), m_ncv),
        |               ^~~~~~~~~~~~~~~~~~~~~~
  /home/martin/libs/gtsam/gtsam/3rdparty/Spectra/MatOp/internal/ArnoldiOp.h:99:7: note: candidate: ‘constexpr Spectra::ArnoldiOp<double, gtsam::MatrixProdFunctor, Spectra::IdentityBOp>::ArnoldiOp(const Spectra::ArnoldiOp<double, gtsam::MatrixProdFunctor, Spectra::IdentityBOp>&)’
     99 | class ArnoldiOp<Scalar, OpType, IdentityBOp>
        |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  /home/martin/libs/gtsam/gtsam/3rdparty/Spectra/MatOp/internal/ArnoldiOp.h:99:7: note:   candidate expects 1 argument, 2 provided
  /home/martin/libs/gtsam/gtsam/3rdparty/Spectra/MatOp/internal/ArnoldiOp.h:99:7: note: candidate: ‘constexpr Spectra::ArnoldiOp<double, gtsam::MatrixProdFunctor, Spectra::IdentityBOp>::ArnoldiOp(Spectra::ArnoldiOp<double, gtsam::MatrixProdFunctor, Spectra::IdentityBOp>&&)’
  /home/martin/libs/gtsam/gtsam/3rdparty/Spectra/MatOp/internal/ArnoldiOp.h:99:7: note:   candidate expects 1 argument, 2 provided
  gmake[2]: *** [gtsam/CMakeFiles/gtsam.dir/build.make:2050: gtsam/CMakeFiles/gtsam.dir/sfm/ShonanAveraging.cpp.o] Error 1
  gmake[1]: *** [CMakeFiles/Makefile2:1397: gtsam/CMakeFiles/gtsam.dir/all] Error 2
  gmake: *** [Makefile:166: all] Error 2

This PR updates the vendored Spectra from v0.9.0 to the latest v1.0.1 as a simple fix for these C++20 compatibility issues. I also moved the Spectra headers under a Spectra/... prefix to match the installed header paths since v0.7.0.

Change log: https://github.com/yixuan/spectra/blob/master/CHANGELOG.md

Since this is a major version bump, there are some breaking changes in the Spectra API, which are listed in the migration guide: https://spectralib.org/upgrade

Corresponding changes to ShonanAveraging.cpp:

diff --git a/gtsam/sfm/ShonanAveraging.cpp b/gtsam/sfm/ShonanAveraging.cpp
index 7c8b07f37c..151c816b21 100644
--- a/gtsam/sfm/ShonanAveraging.cpp
+++ b/gtsam/sfm/ShonanAveraging.cpp
@@ -16,7 +16,7 @@
  * @brief  Shonan Averaging algorithm
  */
 
-#include <SymEigsSolver.h>
+#include <Spectra/SymEigsSolver.h>
 #include <cmath>
 #include <gtsam/linear/PCGSolver.h>
 #include <gtsam/linear/SubgraphPreconditioner.h>
@@ -574,6 +574,8 @@ static bool PowerMinimumEigenValue(
  * nontrivial function, perform_op(x,y), that computes and returns the product
  * y = (A + sigma*I) x */
 struct MatrixProdFunctor {
+  using Scalar = double;
+
   // Const reference to an externally-held matrix whose minimum-eigenvalue we
   // want to compute
   const Sparse &A_;
@@ -634,13 +636,13 @@ static bool SparseMinimumEigenValue(
     Eigen::Index numLanczosVectors = 20) {
   // a. Estimate the largest-magnitude eigenvalue of this matrix using Lanczos
   MatrixProdFunctor lmOperator(A);
-  Spectra::SymEigsSolver<double, Spectra::SELECT_EIGENVALUE::LARGEST_MAGN,
-                         MatrixProdFunctor>
-      lmEigenValueSolver(&lmOperator, 1, std::min(numLanczosVectors, A.rows()));
+  Spectra::SymEigsSolver lmEigenValueSolver(
+      lmOperator, 1, std::min(numLanczosVectors, A.rows()));
   lmEigenValueSolver.init();
 
-  const int lmConverged = lmEigenValueSolver.compute(
-      maxIterations, 1e-4, Spectra::SELECT_EIGENVALUE::LARGEST_MAGN);
+  const int lmConverged =
+      lmEigenValueSolver.compute(Spectra::SortRule::LargestMagn, maxIterations,
+                                 1e-4, Spectra::SortRule::LargestMagn);
 
   // Check convergence and bail out if necessary
   if (lmConverged != 1) return false;
@@ -668,10 +670,8 @@ static bool SparseMinimumEigenValue(
 
   MatrixProdFunctor minShiftedOperator(A, -2 * lmEigenValue);
 
-  Spectra::SymEigsSolver<double, Spectra::SELECT_EIGENVALUE::LARGEST_MAGN,
-                         MatrixProdFunctor>
-      minEigenValueSolver(&minShiftedOperator, 1,
-                          std::min(numLanczosVectors, A.rows()));
+  Spectra::SymEigsSolver minEigenValueSolver(
+      minShiftedOperator, 1, std::min(numLanczosVectors, A.rows()));
 
   // If S is a critical point of F, then S^T is also in the null space of S -
   // Lambda(S) (cf. Lemma 6 of the tech report), and therefore its rows are
@@ -699,8 +699,9 @@ static bool SparseMinimumEigenValue(
   // order to be able to estimate the smallest eigenvalue within an *absolute*
   // tolerance of 'minEigenvalueNonnegativityTolerance'
   const int minConverged = minEigenValueSolver.compute(
-      maxIterations, minEigenvalueNonnegativityTolerance / lmEigenValue,
-      Spectra::SELECT_EIGENVALUE::LARGEST_MAGN);
+      Spectra::SortRule::LargestMagn, maxIterations,
+      minEigenvalueNonnegativityTolerance / lmEigenValue,
+      Spectra::SortRule::LargestMagn);
 
   if (minConverged != 1) return false;

To be honest, I would personally be hesitant to merge such an unwieldy PR, so you might want to commit these changes yourself or at least diff them against the official Spectra sources.

So the includes have a Spectra/ prefix, which was added in v0.7.0.
@ProfFan ProfFan requested a review from dellaert October 12, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant