1. Georgia Tech Borg Lab Avatar Georgia Tech Borg Lab
  2. All
  3. gtsam
  4. Issues

Follow up on Eigen patches

Issue #10 resolved
Richard Roberts created an issue

http://eigen.tuxfamily.org/bz/show_bug.cgi?id=704 (Householder QR MKL selection) http://eigen.tuxfamily.org/bz/show_bug.cgi?id=705 (Fix MKL LLT return code) http://eigen.tuxfamily.org/bz/show_bug.cgi?id=716 (Improved comma initialization)

Comments (20)

  2. Paul Furgale

    We are starting to run in to this problem now as we interface existing code with GTSAM. I see no movement on 705 or 716. What do you suggest for other users of GTSAM?

  3. Richard Roberts reporter

    Sorry I haven't pushed on this lately. The Eigen team has been responsive to discussions about these patches in the past but I think has not had much time or motivation to apply them without further contact with us. I think with a few more messages and testing on our side we can get them merged. The next couple weeks are very busy for me since I'll be traveling but after that I'll have some time to work on this. I can also assist if someone else would like to push a bit on this, in what testing we'd need to do and then report to the Eigen team for them to be able to merge.

    In the mean time, Paul, what issue are you having specifically? In the past, these differences in Eigen have not affected interfacing with code that uses a stock Eigen install as long as the system-installed Eigen version is the same as the Eigen version bundled with GTSAM. We might be able to help you do what you need to do quickly.

  4. Paul Furgale

    Hi Richard, Our team had some compilation problems but we figured out a workaround that we think will work in the long term. I'll just keep an eye on this issue.

  5. Paul Furgale

    Just a ping for this issue. We have developers running in to problems like this:

    In file included from /usr/local/include/gtsam/nonlinear/NonlinearFactorGraph.h:24:0,
from /home/anderson/ethz-asl/src/continuous-trajectory-scanmatching/trajectory_maintainer/test/../include/TrajectoryMaintainer.hpp:13,
from /home/anderson/ethz-asl/src/continuous-trajectory-scanmatching/trajectory_maintainer/test/test_TrajectoryMaintainer.cpp:6:
/usr/local/include/gtsam/geometry/Point2.h: In static member function ‘static gtsam::Vector gtsam::Point2::Logmap(const gtsam::Point2&)’:
/usr/local/include/gtsam/geometry/Point2.h:175:86: error: could not convert ‘Eigen::DenseBase::operator<<(const Scalar&) with Derived = Eigen::Matrix, Eigen::DenseBase::Scalar = double.Eigen::CommaInitializer::operator, with XprType = Eigen::Matrix, Eigen::CommaInitializer = Eigen::CommaInitializer >, Eigen::CommaInitializer::Scalar = double’ from ‘Eigen::CommaInitializer >’ to ‘gtsam::Vector {aka Eigen::Matrix}’
/home/anderson/ethz-asl/src/continuous-trajectory-scanmatching/trajectory_maintainer/test/test_TrajectoryMaintainer.cpp: In member function ‘virtual void trajectoryMaintainerTestsuite_testTrajectoryMaintainer100000_1D_Odometry_Test::TestBody()’:
/home/anderson/ethz-asl/src/continuous-trajectory-scanmatching/trajectory_maintainer/test/test_TrajectoryMaintainer.cpp:237:42: error: no matching function for call to ‘gtsam::noiseModel::Diagonal::Sigmas(Eigen::CommaInitializer >)’
/home/anderson/ethz-asl/src/continuous-trajectory-scanmatching/trajectory_maintainer/test/test_TrajectoryMaintainer.cpp:237:42: note: candidate is:
/usr/local/include/gtsam/linear/NoiseModel.h:284:25: note: static gtsam::noiseModel::Diagonal::shared_ptr gtsam::noiseModel::Diagonal::Sigmas(const Vector&, bool)
/usr/local/include/gtsam/linear/NoiseModel.h:284:25: note: no known conversion for argument 1 from ‘Eigen::CommaInitializer >’ to ‘const Vector& {aka const Eigen::Matrix&}’
/home/anderson/ethz-asl/src/continuous-trajectory-scanmatching/trajectory_maintainer/test/test_TrajectoryMaintainer.cpp:239:39: error: no matching function for call to ‘gtsam::noiseModel::Diagonal::Sigmas(Eigen::CommaInitializer >)’
/home/anderson/ethz-asl/src/continuous-trajectory-scanmatching/trajectory_maintainer/test/test_TrajectoryMaintainer.cpp:239:39: note: candidate is:
/usr/local/include/gtsam/linear/NoiseModel.h:284:25: note: static gtsam::noiseModel::Diagonal::shared_ptr gtsam::noiseModel::Diagonal::Sigmas(const Vector&, bool)
/usr/local/include/gtsam/linear/NoiseModel.h:284:25: note: no known conversion for argument 1 from ‘Eigen::CommaInitializer >’ to ‘const Vector& {aka const Eigen::Matrix&}’
make[3]: *** [continuous-trajectory-scanmatching/trajectory_maintainer/CMakeFiles/trajectory_maintainer_tests.dir/test/test_TrajectoryMaintainer.cpp.o] Error 1
make[2]: *** [continuous-trajectory-scanmatching/trajectory_maintainer/CMakeFiles/trajectory_maintainer_tests.dir/all] Error 2
make[1]: *** [continuous-trajectory-scanmatching/trajectory_maintainer/CMakeFiles/run_tests_trajectory_maintainer.dir/rule] Error 2
make: *** [run_tests_trajectory_maintainer] Error 2

    I think their local build is picking up the system Eigen and running in to problems.

  6. Richard Roberts reporter

    I (finally) worked on that this weekend. It needed unit tests and documentation updates before I could submit a pull request, and I'm almost done with those.

    However, it's concerning that users get that error because having a different Eigen version installed in the system could then find some harder to diagnose runtime bugs if Eigen function behavior changes between versions.

  7. Frank Dellaert

    @lvzhaoyang was also having Eigen problems. It would be great if users could use whatever Eigen install as long as it is > some version, no?

  8. Frank Dellaert

    @lvzhaoyang Since Eigen is completely header, is there not a way to not patch Eigen such that we have a completely unmodified Eigen, but the gtsam eigen header overwrites the relevant eigen functionality? @mikebosse had to add a .finished() somewhere, presumably because he is using a system Eigen, but we use the new comma initializer all over the tests...

  9. Richard Roberts reporter

    Just an update on this - some changes in the latest Eigen code breaks the comma initializer patch and I am stumped by the problem so I asked the Eigen team for some help resolving it.

    One thing about this is that even after this patch is merged into Eigen, it will take a while to show up in an Eigen release, and even longer to show up as default in an Ubuntu install. Maybe we should consider not depending on this patch... It may have been a mistake in the first place! I this it's possible to write a regex find/replace that adds .finished() in all the unit tests.

  11. Richard Roberts reporter

    If you mean patching the system-installed Eigen, I don't think that's a good idea. Because of possible unintended consequences, maybe a user not wanting the system Eigen to be modified, maybe a system Eigen installed after GTSAM is installed, etc.

  13. Richard Roberts reporter

    Hi @dellaert (and @cbeall3), I don't think this is possible to do. I actually had already tried to do this way back when I first modified the comma initializer. I also took another look this morning to try to find a way, but I cannot think of anything that would work. I can fairly easily convert all comma initializer usages to vanilla Eigen using a regular expression. Do you think that is the best option for now? I'd hate to have the 4.0.0 release go out with this Eigen incompatibility problem. We can always go back and change it later if/when the Eigen patch is merged.

  18. Richard Roberts reporter

    Agree, 4.0 is going to be awesome, I'm excited to see it!!! Especially the autodiff and the speed improvements from fixed size matrices!!

  19. Chris Beall

    @richardroberts , you didn't revert the MKL return code patch, did you? Do we still need to follow up on that one, or should we just be careful to carry it forward as we upgrade Eigen?

  21. Log in to comment
Assignee
Type
bug
Priority
major
Status
resolved
Milestone
–
Version
4.0.0
Votes
1
Watchers
5
Jira: the preferred issue tracker for Bitbucket. Join the team!