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

Eigen Serialization Fix #944

Merged
merged 4 commits into from
Nov 30, 2021
Merged

Eigen Serialization Fix #944

merged 4 commits into from
Nov 30, 2021

Conversation

varunagrawal
Copy link
Collaborator

Eigen has backported some fixes which enable correct serialization when using GCC 8 and above.

The issue is detailed here:
https://gitlab.com/libeigen/eigen/-/issues/1676

Basically, boost serialization with Eigen throws an error because of incorrect template deduction handling by GCC. This issue is not seen in Clang, but the Eigen team went ahead and made a simple fix.

The only issue left to resolve is what happens when a user uses the system Eigen rather than the packaged version under 3rdparty. One potential solution is to specify the required Eigen version in CMake (cmake/HandleEigen.cmake). However, I wanted more discussion on this before pushing that change.

An example of the error thrown is when we try to serialize the Link object in GTDynamics, which has a Vector3 for the inertial matrix diagonal:

In file included from /usr/local/include/gtsam/3rdparty/Eigen/Eigen/Core:365,
                 from /usr/local/include/gtsam/inference/FactorGraph.h:29,
                 from /usr/local/include/gtsam/inference/MetisIndex.h:21,
                 from /usr/local/include/gtsam/inference/Ordering.h:25,
                 from /usr/local/include/gtsam/linear/VectorValues.h:21,
                 from /home/varun/borglab/GTDynamics/tests/testLink.cpp:18:
/usr/local/include/gtsam/3rdparty/Eigen/Eigen/src/Core/util/ForwardDeclarations.h: In instantiation of ‘struct Eigen::internal::accessors_level<boost::serialization::U>’:
/usr/local/include/boost/serialization/split_free.hpp:58:13:   required from ‘static void boost::serialization::free_loader<Archive, T>::invoke(Archive&, T&, unsigned int) [with Archive = boost::archive::xml_iarchive; T = Eigen::Matrix<double, 3, 3>]’
/usr/local/include/boost/serialization/split_free.hpp:74:18:   required from ‘void boost::serialization::split_free(Archive&, T&, unsigned int) [with Archive = boost::archive::xml_iarchive; T = Eigen::Matrix<double, 3, 3>]’
/usr/local/include/gtsam/base/Matrix.h:594:17:   required from ‘void boost::serialization::serialize(Archive&, Eigen::Matrix<Scalar_, Rows_, Cols_, Ops_, MaxRows_, MaxCols_>&, unsigned int) [with Archive = boost::archive::xml_iarchive; Scalar_ = double; int Rows_ = 3; int Cols_ = 3; int Ops_ = 0; int MaxRows_ = 3; int MaxCols_ = 3]’
/usr/local/include/boost/serialization/serialization.hpp:126:14:   required from ‘void boost::serialization::serialize_adl(Archive&, T&, unsigned int) [with Archive = boost::archive::xml_iarchive; T = Eigen::Matrix<double, 3, 3>]’
/usr/local/include/boost/archive/detail/iserializer.hpp:187:40:   [ skipping 22 instantiation contexts, use -ftemplate-backtrace-limit=0 to disable ]
/usr/local/include/boost/archive/detail/interface_iarchive.hpp:68:9:   required from ‘Archive& boost::archive::detail::interface_iarchive<Archive>::operator>>(T&) [with T = const boost::serialization::nvp<gtdynamics::Link>; Archive = boost::archive::xml_iarchive]’
/usr/local/include/gtsam/base/serialization.h:128:14:   required from ‘void gtsam::deserializeFromXMLStream(std::istream&, T&, const string&) [with T = gtdynamics::Link; std::istream = std::basic_istream<char>; std::string = std::__cxx11::basic_string<char>]’
/usr/local/include/gtsam/base/serialization.h:145:27:   required from ‘void gtsam::deserializeFromXMLString(const string&, T&, const string&) [with T = gtdynamics::Link; std::string = std::__cxx11::basic_string<char>]’
/usr/local/include/gtsam/base/serialization.h:181:27:   required from ‘void gtsam::deserializeXML(const string&, T&, const string&) [with T = gtdynamics::Link; std::string = std::__cxx11::basic_string<char>]’
/usr/local/include/gtsam/base/serializationTestHelpers.h:98:17:   required from ‘void gtsam::serializationTestHelpers::roundtripXML(const T&, T&) [with T = gtdynamics::Link]’
/usr/local/include/gtsam/base/serializationTestHelpers.h:122:18:   required from ‘bool gtsam::serializationTestHelpers::equalsXML(const T&) [with T = gtdynamics::Link]’
/home/varun/borglab/GTDynamics/tests/testLink.cpp:88:3:   required from here
/usr/local/include/gtsam/3rdparty/Eigen/Eigen/src/Core/util/ForwardDeclarations.h:32:54: error: incomplete type ‘Eigen::internal::traits<boost::serialization::U>’ used in nested name specifier
   32 |   enum { has_direct_access = (traits<Derived>::Flags & DirectAccessBit) ? 1 : 0,
      |                              ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
/usr/local/include/gtsam/3rdparty/Eigen/Eigen/src/Core/util/ForwardDeclarations.h:33:53: error: incomplete type ‘Eigen::internal::traits<boost::serialization::U>’ used in nested name specifier
   33 |          has_write_access = (traits<Derived>::Flags & LvalueBit) ? 1 : 0,
      |                             ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~

@varunagrawal varunagrawal added bugfix Fixes an issue or bug refactor Refactoring of code to remove rot labels Nov 29, 2021
@varunagrawal varunagrawal self-assigned this Nov 29, 2021
@varunagrawal varunagrawal linked an issue Nov 29, 2021 that may be closed by this pull request
@dellaert
Copy link
Member

Is there another way to side-step the issue in Link?

@ProfFan
Copy link
Collaborator

ProfFan commented Nov 29, 2021

@dellaert #940 (comment)

@varunagrawal
Copy link
Collaborator Author

Updated!

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.

I’m not following. I guess this PR is no longer an Eigen upgrade, but a workaround for serialization? Note this was not what I advised in the associated issue.

@varunagrawal varunagrawal changed the title Eigen update to 3.3.9 Eigen Serialization Fix Nov 29, 2021
@varunagrawal
Copy link
Collaborator Author

My understanding was that you preferred the workaround, so I went with that approach. Eigen is no longer touched (we're still at good ol' 3.3.7) but it resolves the serialization issue.

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.

Approved after Ci succeeds

@varunagrawal varunagrawal merged commit 29ccbe2 into develop Nov 30, 2021
@varunagrawal varunagrawal deleted the eigen-update branch November 30, 2021 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes an issue or bug refactor Refactoring of code to remove rot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Eigen compilation issue
3 participants