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

Remove operator^ from Matrix and SubgraphPreconditioner #1969

Conversation

calcmogul
Copy link
Contributor

Eigen recently added an operator^ for coefficient-wise bitwise xor, which conflicts with gtsam's operator^. Since gtsam's operator^ isn't xor, I opted to remove it.

Fixes #1951.

Eigen recently added an operator^ for coefficient-wise bitwise xor,
which conflicts with gtsam's operator^. Since gtsam's operator^ isn't
xor, I opted to remove it.

Fixes borglab#1951.
@mcm001
Copy link
Contributor

mcm001 commented Jan 10, 2025

Thanks! The patch was yours in the first place <3

gtsam/linear/iterative.h Show resolved Hide resolved
gtsam/linear/SubgraphPreconditioner.cpp Outdated Show resolved Hide resolved
gtsam/linear/SubgraphPreconditioner.h Outdated Show resolved Hide resolved
gtsam/linear/iterative.h Show resolved Hide resolved
@dellaert
Copy link
Member

Thanks for doing this ! The CI fails on testMatrix.cpp, though. You can check at least the unit tests on your platform with “make check”.

@dellaert
Copy link
Member

PS @calcmogul / Tyler, looking at your website, I’d gladly welcome your input on IP methods for GTSAM. @yetongumich did a PhD thesis on constrained optimization using “synthesized” manifolds, and he’s working on a few PRs that will include some IP methods, I think. He can comment more - but it’d be great to have an extra set of eyes on those :-)

@calcmogul
Copy link
Contributor Author

calcmogul commented Jan 10, 2025

Two of the test files don't compile for me with GCC 14, even on develop:

[tav@myriad build]$ ninja check -k 0
[1/5] Building CXX object gtsam/discrete/tests/CMakeFiles/testDecisionTreeFactor.dir/testDecisionTreeFactor.cpp.o
FAILED: gtsam/discrete/tests/CMakeFiles/testDecisionTreeFactor.dir/testDecisionTreeFactor.cpp.o 
ccache /usr/lib/ccache/bin/c++ -DNDEBUG -DTOPSRCDIR=\"/home/tav/git/gtsam\" -I/home/tav/git/gtsam -I/home/tav/git/gtsam/build -I/home/tav/git/gtsam/CppUnitLite -I/home/tav/git/gtsam/gtsam/3rdparty/metis/include -I/home/tav/git/gtsam/gtsam/3rdparty/metis/libmetis -I/home/tav/git/gtsam/gtsam/3rdparty/metis/GKlib -I/home/tav/git/gtsam/gtsam/3rdparty/cephes -isystem /home/tav/git/gtsam/gtsam/3rdparty/SuiteSparse_config -isystem /home/tav/git/gtsam/gtsam/3rdparty/Spectra -isystem /home/tav/git/gtsam/gtsam/3rdparty/CCOLAMD/Include -isystem /home/tav/git/gtsam/gtsam/3rdparty/Eigen -O3 -DNDEBUG -std=c++17 -fPIE -fdiagnostics-color=always -fdiagnostics-color=always -Wall -fPIC -Wreturn-local-addr -Werror=return-local-addr -Wreturn-type -Werror=return-type -Wformat -Werror=format-security -Wsuggest-override -O3 -Wno-unused-local-typedefs -MD -MT gtsam/discrete/tests/CMakeFiles/testDecisionTreeFactor.dir/testDecisionTreeFactor.cpp.o -MF gtsam/discrete/tests/CMakeFiles/testDecisionTreeFactor.dir/testDecisionTreeFactor.cpp.o.d -o gtsam/discrete/tests/CMakeFiles/testDecisionTreeFactor.dir/testDecisionTreeFactor.cpp.o -c /home/tav/git/gtsam/gtsam/discrete/tests/testDecisionTreeFactor.cpp
In file included from /home/tav/git/gtsam/CppUnitLite/TestHarness.h:23,
                 from /home/tav/git/gtsam/gtsam/discrete/tests/testDecisionTreeFactor.cpp:20:
/home/tav/git/gtsam/gtsam/discrete/tests/testDecisionTreeFactor.cpp: In member function ‘virtual void DecisionTreeFactorDivideTest::run(TestResult&)’:
/home/tav/git/gtsam/gtsam/discrete/tests/testDecisionTreeFactor.cpp:123:25: error: invalid initialization of reference of type ‘const gtsam::Vector&’ {aka ‘const Eigen::Matrix<double, -1, 1>&’} from expression of type ‘gtsam::DecisionTreeFactor’
  123 |   EXPECT(assert_inequal(pS, s));
      |                         ^~
/home/tav/git/gtsam/CppUnitLite/Test.h:151:9: note: in definition of macro ‘EXPECT’
  151 | { if (!(condition)) \
      |         ^~~~~~~~~
In file included from /home/tav/git/gtsam/gtsam/inference/DotWriter.h:22,
                 from /home/tav/git/gtsam/gtsam/inference/FactorGraph.h:25,
                 from /home/tav/git/gtsam/gtsam/inference/MetisIndex.h:21,
                 from /home/tav/git/gtsam/gtsam/inference/Ordering.h:25,
                 from /home/tav/git/gtsam/gtsam/discrete/DiscreteFactor.h:25,
                 from /home/tav/git/gtsam/gtsam/discrete/DecisionTreeFactor.h:22,
                 from /home/tav/git/gtsam/gtsam/discrete/tests/testDecisionTreeFactor.cpp:23:
/home/tav/git/gtsam/gtsam/base/Vector.h:163:48: note: in passing argument 1 of ‘bool gtsam::assert_inequal(const Vector&, const Vector&, double)’
  163 | GTSAM_EXPORT bool assert_inequal(const Vector& vec1, const Vector& vec2, double tol=1e-9);
      |                                  ~~~~~~~~~~~~~~^~~~
/home/tav/git/gtsam/gtsam/discrete/tests/testDecisionTreeFactor.cpp:131:25: error: invalid initialization of reference of type ‘const gtsam::Vector&’ {aka ‘const Eigen::Matrix<double, -1, 1>&’} from expression of type ‘gtsam::KeySet’ {aka ‘gtsam::FastSet<long unsigned int>’}
  131 |   EXPECT(assert_inequal(KeySet(pS.keys()), keys));
      |                         ^~~~~~~~~~~~~~~~~
/home/tav/git/gtsam/CppUnitLite/Test.h:151:9: note: in definition of macro ‘EXPECT’
  151 | { if (!(condition)) \
      |         ^~~~~~~~~
/home/tav/git/gtsam/gtsam/base/Vector.h:163:48: note: in passing argument 1 of ‘bool gtsam::assert_inequal(const Vector&, const Vector&, double)’
  163 | GTSAM_EXPORT bool assert_inequal(const Vector& vec1, const Vector& vec2, double tol=1e-9);
      |                                  ~~~~~~~~~~~~~~^~~~
[2/5] Building CXX object gtsam/discrete/tests/CMakeFiles/testTableFactor.dir/testTableFactor.cpp.o
FAILED: gtsam/discrete/tests/CMakeFiles/testTableFactor.dir/testTableFactor.cpp.o 
ccache /usr/lib/ccache/bin/c++ -DNDEBUG -DTOPSRCDIR=\"/home/tav/git/gtsam\" -I/home/tav/git/gtsam -I/home/tav/git/gtsam/build -I/home/tav/git/gtsam/CppUnitLite -I/home/tav/git/gtsam/gtsam/3rdparty/metis/include -I/home/tav/git/gtsam/gtsam/3rdparty/metis/libmetis -I/home/tav/git/gtsam/gtsam/3rdparty/metis/GKlib -I/home/tav/git/gtsam/gtsam/3rdparty/cephes -isystem /home/tav/git/gtsam/gtsam/3rdparty/SuiteSparse_config -isystem /home/tav/git/gtsam/gtsam/3rdparty/Spectra -isystem /home/tav/git/gtsam/gtsam/3rdparty/CCOLAMD/Include -isystem /home/tav/git/gtsam/gtsam/3rdparty/Eigen -O3 -DNDEBUG -std=c++17 -fPIE -fdiagnostics-color=always -fdiagnostics-color=always -Wall -fPIC -Wreturn-local-addr -Werror=return-local-addr -Wreturn-type -Werror=return-type -Wformat -Werror=format-security -Wsuggest-override -O3 -Wno-unused-local-typedefs -MD -MT gtsam/discrete/tests/CMakeFiles/testTableFactor.dir/testTableFactor.cpp.o -MF gtsam/discrete/tests/CMakeFiles/testTableFactor.dir/testTableFactor.cpp.o.d -o gtsam/discrete/tests/CMakeFiles/testTableFactor.dir/testTableFactor.cpp.o -c /home/tav/git/gtsam/gtsam/discrete/tests/testTableFactor.cpp
In file included from /home/tav/git/gtsam/CppUnitLite/TestHarness.h:23,
                 from /home/tav/git/gtsam/gtsam/discrete/tests/testTableFactor.cpp:19:
/home/tav/git/gtsam/gtsam/discrete/tests/testTableFactor.cpp: In member function ‘virtual void TableFactorconstructorsTest::run(TestResult&)’:
/home/tav/git/gtsam/gtsam/discrete/tests/testTableFactor.cpp:147:25: error: invalid initialization of reference of type ‘const gtsam::Vector&’ {aka ‘const Eigen::Matrix<double, -1, 1>&’} from expression of type ‘gtsam::TableFactor’
  147 |   EXPECT(assert_inequal(f5_with_wrong_keys, f5, 1e-9));
      |                         ^~~~~~~~~~~~~~~~~~
/home/tav/git/gtsam/CppUnitLite/Test.h:151:9: note: in definition of macro ‘EXPECT’
  151 | { if (!(condition)) \
      |         ^~~~~~~~~
In file included from /home/tav/git/gtsam/gtsam/inference/DotWriter.h:22,
                 from /home/tav/git/gtsam/gtsam/inference/FactorGraph.h:25,
                 from /home/tav/git/gtsam/gtsam/inference/MetisIndex.h:21,
                 from /home/tav/git/gtsam/gtsam/inference/Ordering.h:25,
                 from /home/tav/git/gtsam/gtsam/discrete/DiscreteFactor.h:25,
                 from /home/tav/git/gtsam/gtsam/discrete/DecisionTreeFactor.h:22,
                 from /home/tav/git/gtsam/gtsam/discrete/DiscreteConditional.h:21,
                 from /home/tav/git/gtsam/gtsam/discrete/tests/testTableFactor.cpp:22:
/home/tav/git/gtsam/gtsam/base/Vector.h:163:48: note: in passing argument 1 of ‘bool gtsam::assert_inequal(const Vector&, const Vector&, double)’
  163 | GTSAM_EXPORT bool assert_inequal(const Vector& vec1, const Vector& vec2, double tol=1e-9);
      |                                  ~~~~~~~~~~~~~~^~~~
ninja: build stopped: cannot make progress due to previous errors.

@calcmogul
Copy link
Contributor Author

calcmogul commented Jan 10, 2025

I’d gladly welcome your input on IP methods for GTSAM ... it’d be great to have an extra set of eyes on [a few PRs that will include some IP methods]

I'd be happy to. Some IPMs that properly deal with manifolds would be really good to have.

@mcm001
Copy link
Contributor

mcm001 commented Jan 11, 2025

I had to apply this diff, but otherwise works with newer Eigen for me! Why might CI here be missing cassert includes, but I'm hitting them locally with gcc 11.4.0-1ubuntu1~22.04?

diff --git a/gtsam/basis/Chebyshev2.cpp b/gtsam/basis/Chebyshev2.cpp
index 71c3db7f0..df7bf5f19 100644
--- a/gtsam/basis/Chebyshev2.cpp
+++ b/gtsam/basis/Chebyshev2.cpp
@@ -17,6 +17,7 @@
  */
 
 #include <gtsam/basis/Chebyshev2.h>
+#include <cassert>
 
 namespace gtsam {
 

@mcm001
Copy link
Contributor

mcm001 commented Jan 11, 2025

(I'm also seeing this in our CI as well)

@dellaert
Copy link
Member

Two of the test files don't compile for me with GCC 14, even on develop:

Might need to cast explicitly or use an explicit template.

Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

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

Thanks !!! I will merge as soon as last CI passes.

@dellaert dellaert merged commit 49c67d3 into borglab:develop Jan 11, 2025
33 checks passed
@calcmogul calcmogul deleted the remove-operator-from-matrix-and-subgraph-preconditioner branch January 11, 2025 19:57
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.

Ambiguous operator^ with new Eigen versions
3 participants