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

ShonanAveraging compile error? #689

Closed
ToniRV opened this issue Jan 29, 2021 · 9 comments
Closed

ShonanAveraging compile error? #689

ToniRV opened this issue Jan 29, 2021 · 9 comments

Comments

@ToniRV
Copy link
Contributor

ToniRV commented Jan 29, 2021

Sorry for the badly formatted issue.
I think I got an error on the ShonanAveraging.cpp side

error: no match for ‘operator-’ (operand types are ‘const ScalarMultipleReturnType {aka const Eigen::CwiseUnaryOp<Eigen::internal::scalar_multiple_op<double>, const Eigen::CwiseNullaryOp<Eigen::internal::scalar_identity_op<double>, Eigen::Matrix<double, -1, -1> > >}’ and ‘const Sparse {aka const Eigen::SparseMatrix<double>}’)
   const Sparse C = pmEigenValue * Matrix::Identity(A.rows(), A.cols()) - A;

That's probably from Frank's latest paper?

/home/tonirv/Code/gtsam/gtsam/sfm/ShonanAveraging.cpp: In function ‘bool gtsam::PowerMinimumEigenValue(const Sparse&, const Matrix&, double&, gtsam::Vector*, std::size_t*, std::size_t, double)’:
/home/tonirv/Code/gtsam/gtsam/sfm/ShonanAveraging.cpp:556:72: error: no match for ‘operator-’ (operand types are ‘const ScalarMultipleReturnType {aka const Eigen::CwiseUnaryOp<Eigen::internal::scalar_multiple_op<double>, const Eigen::CwiseNullaryOp<Eigen::internal::scalar_identity_op<double>, Eigen::Matrix<double, -1, -1> > >}’ and ‘const Sparse {aka const Eigen::SparseMatrix<double>}’)
   const Sparse C = pmEigenValue * Matrix::Identity(A.rows(), A.cols()) - A;
                                                                        ^
In file included from /usr/include/eigen3/Eigen/src/Core/MatrixBase.h:126:0,
                 from /usr/include/eigen3/Eigen/Core:345,
                 from /home/tonirv/Code/gtsam/gtsam/3rdparty/Spectra/SymEigsSolver.h:10,
                 from /home/tonirv/Code/gtsam/gtsam/sfm/ShonanAveraging.cpp:19:
/usr/include/eigen3/Eigen/src/plugins/CommonCwiseUnaryOps.h:50:1: note: candidate: const NegativeReturnType Eigen::MatrixBase<Derived>::operator-() const [with Derived = Eigen::CwiseUnaryOp<Eigen::internal::scalar_multiple_op<double>, const Eigen::CwiseNullaryOp<Eigen::internal::scalar_identity_op<double>, Eigen::Matrix<double, -1, -1> > >; Eigen::MatrixBase<Derived>::NegativeReturnType = Eigen::CwiseUnaryOp<Eigen::internal::scalar_opposite_op<double>, const Eigen::CwiseUnaryOp<Eigen::internal::scalar_multiple_op<double>, const Eigen::CwiseNullaryOp<Eigen::internal::scalar_identity_op<double>, Eigen::Matrix<double, -1, -1> > > >; typename Eigen::internal::traits<T>::Scalar = double]
 operator-() const { return NegativeReturnType(derived()); }

Removing all Shonan* files makes gtsam compile just fine. @dellaert
Using Ubuntu 16.04 🤷‍♂️ if that helps

@dellaert
Copy link
Member

Hmmm, this should be possible, and this compiles fine on my Mac. It also survived 23 CI runs.

BTW, @jingwuOUO, I hope we're sure here that we do not create a very large dense matrix, right? I think Eigen does the right thing with expression templates, but we better be sure :-)

@ToniRV, the expression templates might also be the issue for compilation. No good idea what to do here except try a different expression template: try

const Sparse C = Matrix::Identity(A.rows(), A.cols()) * pmEigenValue- A;

?

@ToniRV
Copy link
Contributor Author

ToniRV commented Jan 29, 2021

Ah, cleaning and re-building solves the issue...
Sorry for the noise!

@ToniRV ToniRV closed this as completed Jan 29, 2021
@ToniRV
Copy link
Contributor Author

ToniRV commented Feb 2, 2021

Cleaning and re-building resets the Eigen used to the one bundled in GTSAM (3.3.7)... but when using my system-wide Eigen (3.2.92) it breaks with:

[ 18%] Building CXX object gtsam/CMakeFiles/gtsam.dir/sfm/ShonanAveraging.cpp.o
/home/tonirv/Code/gtsam/gtsam/sfm/ShonanAveraging.cpp: In function ‘bool gtsam::PowerMinimumEigenValue(const Sparse&, const Matrix&, double&, gtsam::Vector*, std::size_t*, std::size_t, double)’:
/home/tonirv/Code/gtsam/gtsam/sfm/ShonanAveraging.cpp:557:71: error: no match for ‘operator-’ (operand types are ‘const ScalarMultipleReturnType {aka const Eigen::CwiseUnaryOp<Eigen::internal::scalar_multiple_op<double>, const Eigen::CwiseNullaryOp<Eigen::internal::scalar_identity_op<double>, Eigen::Matrix<double, -1, -1> > >}’ and ‘const Sparse {aka const Eigen::SparseMatrix<double>}’)
   const Sparse C = Matrix::Identity(A.rows(), A.cols()) * pmEigenValue- A;
   
gtsam/CMakeFiles/gtsam.dir/build.make:2726: recipe for target 'gtsam/CMakeFiles/gtsam.dir/sfm/ShonanAveraging.cpp.o' failed
make[2]: *** [gtsam/CMakeFiles/gtsam.dir/sfm/ShonanAveraging.cpp.o] Error 1
Use System Eigen                                 : ON (Using version: 3.2.92) <-- This doesn't work!
Use System Eigen                                 : OFF (Using version: 3.3.7) <-- This works.

I think this could be avoided without the user having to update Eigen?
Removing all Shonan* files makes gtsam compile just fine.

@ToniRV ToniRV reopened this Feb 2, 2021
@dellaert
Copy link
Member

dellaert commented Feb 3, 2021

Ah. How could it be avoided? Did you try the other order I suggested?

@ToniRV
Copy link
Contributor Author

ToniRV commented Feb 3, 2021

The change of order didn't work. It seems more like an incompatibility between Dense and Sparse matrices...

ToniRV added a commit to ToniRV/gtsam-1 that referenced this issue Feb 3, 2021
@ToniRV
Copy link
Contributor Author

ToniRV commented Feb 3, 2021

Ok, as trivial as it may sound, I had to tell Eigen that the identity matrix is sparse 😄
const Sparse C = pmEigenValue * Matrix::Identity(A.rows(), A.cols()).sparseView() - A;
Let's see if CI doesn't complain.

@dellaert
Copy link
Member

dellaert commented Feb 3, 2021

Nice!

@ToniRV
Copy link
Contributor Author

ToniRV commented Feb 4, 2021

#690 it works!

@ToniRV ToniRV closed this as completed Feb 4, 2021
@shmily53
Copy link

shmily53 commented Apr 3, 2024

@ToniRV This kind of fix doesn't seem to cover all scenarios. When the eigen library globally defines EIGEN_DEFAULT_TO_ROW_MAJOR will compile error "error: ambiguous partial specializations of 'cwie_promote_storage_order<Eigen::Sparse, Eigen::Sparse, 1 , 0>'"

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

No branches or pull requests

3 participants