Expose Ceres Solver, Problem and Covariance Options as ROS parameters #78

efernandez · 2019-07-16T12:14:26Z

This allows to set all ceres::Solver::Options, ceres::Problem::Options and ceres::Covariance::Options using ROS parameters.

The motivation is to tune the solver and problem for better performance or accuracy, after looking at the tuning results shown in the plots at https://github.com/SteveMacenski/slam_toolbox , which was shared with me by @svwilliams . Unfortunately, it looks like the best configuration is almost the same as the default one 😆 , except for the preconditioner_type option, which defaults to JACOBI but it's set to SCHUR_JACOBI, which indeed make things a little faster for me (from 55 to 49% CPU with my fuse_rl configuration).

Either way, I think this is useful to tune things with other goals in mind, like accuracy, debugging, and still performance. TBH I haven't played a lot with the options/parameters, other than confirming things are being set and used internally.

I also suspect the enable_fast_removal option doesn't make a big different for the ceres::Problem because I think it's built from scratch every time.

I took the list of options from the latest version of Ceres, which will be 2.0.0, but also supported 1.13.x:

efernandez · 2019-07-16T13:01:26Z

fuse_core/include/fuse_core/macros.h

@@ -58,7 +58,7 @@
 #include <memory>
 #include <string>

-// Required by make_aligned_shared, that uses Eigen::aligned_allocator<T>().
+// Required by __MAKE_SHARED_ALIGNED_DEFINITION, that uses Eigen::aligned_allocator<T>().


Simply updating the comment, because we finally don't use make_aligned_shared, which doesn't exist.

efernandez · 2019-07-16T13:03:41Z

fuse_optimizers/include/fuse_optimizers/fixed_lag_smoother_params.h

   *
   * @param[in] node_handle - The node handle used to load the parameter
   * @param[in] parameter_name - The parameter name to load
   * @param[in] default_value - A default value to use if the provided parameter name does not exist
   * @return The loaded (or default) value
   */
-  double getPositiveParam(const ros::NodeHandle& node_handle, const std::string& parameter_name, double default_value)


At the end I don't really use getPositiveParam for integral types, but I think this change is for good, although it'd be great to have this function in a more generic place. Either way, if you don't like this, it's in its own commit, so it's easy drop it from this PR.

svwilliams · 2019-07-25T15:53:35Z

Sorry it is taking me so long to go through this one. I will eventually get some version of this merged, hopefully next week. It was on my ToDo list to support setting Ceres parameters, so thanks for beating me to it.

efernandez · 2019-07-25T18:03:01Z

I'd check the continuous integration results to fix whatever is failing (likely roslint issues), but I'm afraid this is requesting me too much: 🤔

I'm not 100% sure if that will grant it access to the organizations I'm in, but I wonder why is that needed or what it actually does.

efernandez · 2019-07-25T18:57:26Z

There shouldn't be any roslint issues now.

efernandez · 2019-08-14T16:56:00Z

I've just rebased this PR and added the changes from locusrobotics/fuse_rl#22 here.

FYI @ayrton04 @svwilliams

SteveMacenski · 2019-08-14T17:57:25Z

@efernandez on your slam toolbox link, trust me the fact that nearly the defaults were the best is not lost on me. I probably spent 2 weeks thinking how much smarter I was than the Ceres guys trying out combinations to come crawling back. I learned alot so shrug.

Anyhow reason I comment- there are some parameters I did not include as a "don't give them enough rope to hang themselves" I see that you've exposed parameters for both line search and trust region strategies, which for most applications (at least I'm imagining this being used for) line search seems to be a bad choice, and many of the parameters only do things in certain modes. Removing those would simplify the overwhelming number of choices to the things that people will actually want to focus in on.

efernandez · 2019-08-15T11:38:33Z

Thanks for you comment @SteveMacenski . I really appreciate your effort to analyse the different options. It's a shame though that defaults are almost the best combination, except fo the pre-conditioner, which is actually slightly faster with SCHUR_JACOBI. 👍

I decided to expose all options because I don't know if someone wants to play with any of them, but you're right that some options are only used for certain types of solvers. I'm happy to drop some options if that's always the case, or document which options are the most relevant ones. That way it'd be clear to the user what options are those by just looking at this package, although ceres already does a good job at documenting all options. 😃

SteveMacenski · 2019-08-15T15:20:54Z

@efernandez fair enough -- just thought I'd mention it. I'm always a fan of giving every single little dial they can turn, and especially since this is supposed to be general, it makes sense. I looked at the structure and think "huh this really seems designed towards localization/slam/filtering" which fit trust region over line search categorically so there's an opportunity to simplify since about 50% of the parameters don't do anything in trust region.

If nothing else Ceres docs exist and this thread 👍

efernandez · 2019-08-22T18:39:56Z

Not sure why the checks failed. I've rebased this on top of devel and run make roslint on all pkgs changed, and everything LGTM locally. Could you tell me what checks ares failing, please?

svwilliams · 2019-08-22T18:51:23Z

This is for the Xenial build. There were no errors in the Bionic build.

/home/ec2-user/workspace/test_rosdistro_fuse_PR-78/package/fuse_models/include/fuse_models/parameters/odometry_2d_publisher_params.h:135:26: error: ‘struct ceres::Covariance::Options’ has no member named ‘sparse_linear_algebra_library_type’

[2019-08-14T16:56:49.923Z]        covariance_options.sparse_linear_algebra_library_type = fuse_core::getParam(

I'm guessing the default version of Ceres in Xenial doesn't have that parameter?

efernandez · 2019-08-23T10:27:16Z

Thanks @svwilliams . I just fixed it. The sparse_linear_algebra_library_type options was added to ceres::Covariance::Options in version 1.13.0, so Ubuntu Xenial must be using and older version.

This commit introduced that changed: ceres-solver/ceres-solver@14d8297 (also mentioned in the code now)

svwilliams

Sorry this took so long to approve. And what I wouldn't give for a Ceres-maintained INI file reader.

svwilliams · 2019-08-26T21:15:43Z

fuse_optimizers/src/fixed_lag_smoother_node.cpp

+  ros::NodeHandle private_node_handle("~");
+  fuse_graphs::HashGraphParams hash_graph_params;
+  hash_graph_params.loadFromROS(private_node_handle);
+  fuse_optimizers::FixedLagSmoother optimizer(fuse_graphs::HashGraph::make_unique(hash_graph_params.problem_options));


Since you made a HashGraphParams object, it would probably make sense to pass the whole struct to the HashGraph constructor, rather than just the problem_options field.

Makes sense. Done.

ayrton04 · 2019-08-27T08:52:42Z

fuse_core/include/fuse_core/ceres_options.h

@@ -0,0 +1,188 @@
+/***************************************************************************
+ * Copyright (C) 2019 Clearpath Robotics. All rights reserved.


This license is probably going to have to change.

No problem. Would you guys change it before merging?

efernandez · 2019-08-30T15:34:25Z

fuse_core/include/fuse_core/ceres_options.h

+namespace ceres
+{
+
+bool StringtoLoggingType(std::string value, LoggingType* type)


Oops, it looks like I have this the other way around. Fixing it.

refs CORE-13949

refs CORE-13949 It accepts integral and floating point types only.

That use Eigen::aligned_allocator

refs CORE-13949 Instead of ceres::Problem::Options directly.

efernandez · 2019-08-31T09:12:03Z

Thanks, I just realized I removed the serialization headers by mistake while solving some conflicts after rebasing. 😃

efernandez mentioned this pull request Jul 16, 2019

Load ceres::Covariance::Options from param server locusrobotics/fuse_rl#22

Closed

efernandez commented Jul 16, 2019

View reviewed changes

efernandez force-pushed the CORE-13949-expose-ceres-solver-and-problem-options-as-ros-params branch from 0ed3490 to c2949ba Compare July 19, 2019 08:02

efernandez force-pushed the CORE-13949-expose-ceres-solver-and-problem-options-as-ros-params branch from c2949ba to ef1180f Compare July 25, 2019 18:52

efernandez force-pushed the CORE-13949-expose-ceres-solver-and-problem-options-as-ros-params branch from ef1180f to 96c8e41 Compare August 14, 2019 16:53

efernandez force-pushed the CORE-13949-expose-ceres-solver-and-problem-options-as-ros-params branch from 96c8e41 to cde1523 Compare August 23, 2019 10:25

efernandez changed the title ~~Expose Ceres Solver and Problem Options as ROS parameters~~ Expose Ceres Solver, Problem and Covariance Options as ROS parameters Aug 23, 2019

svwilliams approved these changes Aug 26, 2019

View reviewed changes

svwilliams requested a review from ayrton04 August 26, 2019 21:24

ayrton04 approved these changes Aug 27, 2019

View reviewed changes

efernandez commented Aug 30, 2019

View reviewed changes

efernandez force-pushed the CORE-13949-expose-ceres-solver-and-problem-options-as-ros-params branch 2 times, most recently from d337a74 to a5d4b0e Compare August 30, 2019 15:56

efernandez added 6 commits August 30, 2019 17:58

Load ceres::Solver::Options from parameter server

d9bd690

refs CORE-13949

Load ceres::Problem::Options from parameter server

3e00a25

refs CORE-13949

Load ceres::Covariance::Options from param server

fb0141e

refs CORE-13949

Make getPositiveParam a template function

123a7a8

refs CORE-13949 It accepts integral and floating point types only.

Update comment for macros

481b556

That use Eigen::aligned_allocator

Pass HashGraphParams to HashGraph constructor

e850e5e

refs CORE-13949 Instead of ceres::Problem::Options directly.

efernandez force-pushed the CORE-13949-expose-ceres-solver-and-problem-options-as-ros-params branch from a5d4b0e to e850e5e Compare August 30, 2019 15:59

svwilliams added 3 commits August 30, 2019 11:58

Updated license

96e10a5

Updated license

48b0a99

Added serialization headers back

67e3b5a

svwilliams merged commit 7d6c005 into locusrobotics:devel Aug 30, 2019

This was referenced Jul 13, 2021

fuse: 0.4.1-2 in 'melodic/distribution.yaml' [bloom] ros/rosdistro#30187

Merged

fuse: 0.4.1-1 in 'noetic/distribution.yaml' [bloom] ros/rosdistro#30198

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose Ceres Solver, Problem and Covariance Options as ROS parameters #78

Expose Ceres Solver, Problem and Covariance Options as ROS parameters #78

efernandez commented Jul 16, 2019 •

edited

Loading

efernandez Jul 16, 2019

efernandez Jul 16, 2019

svwilliams commented Jul 25, 2019

efernandez commented Jul 25, 2019 •

edited

Loading

efernandez commented Jul 25, 2019

efernandez commented Aug 14, 2019

SteveMacenski commented Aug 14, 2019

efernandez commented Aug 15, 2019 •

edited

Loading

SteveMacenski commented Aug 15, 2019

efernandez commented Aug 22, 2019

svwilliams commented Aug 22, 2019

efernandez commented Aug 23, 2019

svwilliams left a comment

svwilliams Aug 26, 2019

efernandez Aug 30, 2019

ayrton04 Aug 27, 2019

efernandez Aug 30, 2019

efernandez Aug 30, 2019

efernandez Aug 30, 2019

efernandez commented Aug 31, 2019

		@@ -0,0 +1,188 @@
		/***************************************************************************
		* Copyright (C) 2019 Clearpath Robotics. All rights reserved.

Expose Ceres Solver, Problem and Covariance Options as ROS parameters #78

Expose Ceres Solver, Problem and Covariance Options as ROS parameters #78

Conversation

efernandez commented Jul 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svwilliams commented Jul 25, 2019

efernandez commented Jul 25, 2019 • edited Loading

efernandez commented Jul 25, 2019

efernandez commented Aug 14, 2019

SteveMacenski commented Aug 14, 2019

efernandez commented Aug 15, 2019 • edited Loading

SteveMacenski commented Aug 15, 2019

efernandez commented Aug 22, 2019

svwilliams commented Aug 22, 2019

efernandez commented Aug 23, 2019

svwilliams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efernandez commented Aug 31, 2019

efernandez commented Jul 16, 2019 •

edited

Loading

efernandez commented Jul 25, 2019 •

edited

Loading

efernandez commented Aug 15, 2019 •

edited

Loading