Feature/robust shonan #547

lucacarlone · 2020-09-30T20:16:50Z

Added option to use a Huber norm in Shonan Rotation Averaging.
The main changes were:

added a useHuber option in Shonan
added machinery to convert binaryMeasurements to use a robust noise model
adjusted the FrobeniusFactor-related code to cope with possibly robust costs
updated the example ShonanAveragingCLI.cpp

…ved hard-coded sigma in FrobeniusFactor

dellaert

Awesome! However, please refactor to pass noise model in constructor of binary measurements. Imperative code goes against GTSAM design guidelines.

gtsam/sfm/BinaryMeasurement.h

gtsam/sfm/ShonanAveraging.cpp

gtsam/sfm/ShonanAveraging.h

gtsam/slam/FrobeniusFactor.h

.cproject

varunagrawal · 2020-11-09T15:42:37Z

@dellaert @lucacarlone I went ahead and addressed the review comments while also removing the goto in the FrobeniusFactor.

varunagrawal · 2020-11-09T16:02:32Z

I'm pretty sure I'll never hear the end of it from Frank if I "screw good practice".

dellaert · 2020-11-09T17:32:58Z

I’ll review, but FWIW a goto can be the best choice within a function. It’s bad reputation stems from very long programs with lots of goto statements.

lucacarlone · 2020-11-10T02:49:29Z

@varunagrawal this is great!! it was still on my calendar, but had to push it back for a while. Is there anything else for which you need my help on this branch?

varunagrawal · 2020-11-10T03:23:35Z

@lucacarlone I think I've got this covered. The PR was already in great shape to begin with. 😊

I only need folks to review all my PRs now.

varunagrawal · 2020-11-28T00:44:09Z

@dellaert gentle reminder 🙂

dellaert

You asked for my review :-) Sorry about it being late.

dellaert · 2020-11-30T14:07:07Z

examples/ShonanAveragingCLI.cpp

    auto initial = shonan.initializeRandomly(rng);
-    auto result = shonan.run(initial);
+    auto result = shonan.run(initial,pMin);


spacing. Did you lint?

dellaert · 2020-11-30T14:07:47Z

examples/ShonanAveragingCLI.cpp

@@ -58,6 +61,10 @@ int main(int argc, char* argv[]) {
      "Write solution to the specified file")(
      "dimension,d", po::value<int>(&d)->default_value(3),
      "Optimize over 2D or 3D rotations")(
+      "useHuberLoss,h", po::value<bool>(&useHuberLoss)->default_value(false),
+	  "set True to use Huber loss")(
+	  "pMin,p", po::value<int>(&pMin)->default_value(3),


spacing. Please format (only parts that have been changed to avoid large diff) and also lint

dellaert · 2020-11-30T14:08:29Z

examples/ShonanAveragingCLI.cpp

    auto initial = shonan.initializeRandomly(rng);
-    auto result = shonan.run(initial);
+    auto result = shonan.run(initial,pMin);


dellaert · 2020-11-30T14:12:00Z

gtsam/sfm/BinaryMeasurement.h

-      : Factor(std::vector<Key>({key1, key2})), measured_(measured),
-        noiseModel_(model) {}
+                    const SharedNoiseModel &model = nullptr,
+                    bool useHuber = false)


I'm a bit puzzled about this flag. Why not just give BinaryMeasurement a robust error model as the input argument. I think the code should be re-factored to do just that, construct the noise model out of this class.

dellaert · 2020-11-30T14:13:04Z

gtsam/sfm/ShonanAveraging.h

@@ -53,6 +53,8 @@ struct GTSAM_EXPORT ShonanAveragingParameters {
  double alpha;                 // weight of anchor-based prior (default 0)
  double beta;                  // weight of Karcher-based prior (default 1)
  double gamma;                 // weight of gauge-fixing factors (default 0)
+  bool useHuber;  // if enabled, the Huber loss is used in the optimization


Spacing for comments is inconsistent. (Format changed part only)

This is the result of the formatter.

dellaert · 2020-11-30T14:15:01Z

gtsam/sfm/ShonanAveraging.h

+  Measurements makeNoiseModelRobust(const Measurements& measurements) const {
+    Measurements robustMeasurements = measurements;
+    for (auto &measurement : robustMeasurements) {
+      measurement = BinaryMeasurement<Rot>(measurement, true);


imperative, without any good reason. Please create a new Measurements variables, reserve space, and emplace_back :-)

dellaert · 2020-11-30T14:16:13Z

gtsam/slam/FrobeniusFactor.cpp

    size_t n = sigmas.size();
    if (n == 1) {
      sigma = sigmas(0); // Rot2
-      goto exit;
+      exit = true;


restore goto flow. It was good.

dellaert · 2020-11-30T14:17:30Z

gtsam/sfm/ShonanAveraging.cpp

+    const auto &robust = boost::dynamic_pointer_cast<noiseModel::Robust>(
+        measurement.noiseModel());
+    if (robust) {
+      std::cout << "Verification of optimality does not work with robust cost "


It should throw.

This causes the constructor to error out when using the huber noise model. Thoughts?

Try remove the autos?

I don't think that will help, because there are test cases where we specifically use the huber model and this if-case will succeed. Throwing an exception will cause the test to fail.

You can test an exception, no? You can't check whether something prints out. Anyway, printing a warning is not the way to handle a wrong use of the module.

I am not sure I understand. Shonan should still be able to work with a robust model, only losing the power to provide the certificate of optimality, right?

There is a ‘robust’ flag somewhere, right? If the flag is set, then robust should be allowed without a message. If it is not set it should throw. Printing out a message and then muddling on is not an option in my view.

Gotcha. I did misunderstand what you meant, sorry about that.

my 2 cents: it might be fishy to just disable the optimality verification when using robust loss, since the user might still think s/he is getting a certifiably-optimal solution. I suggest adding a flag "certifyOptimality" and throw an exception when certifyOptimality = true and we use a robust loss. The flag certifyOptimality might be useful also in the non-robust case since the user might want to disable the certification to save time/computation.

dellaert · 2020-11-30T14:17:44Z

gtsam/sfm/ShonanAveraging.cpp

@@ -793,6 +803,12 @@ std::pair<Values, double> ShonanAveraging<d>::run(const Values &initialEstimate,
  for (size_t p = pMin; p <= pMax; p++) {
    // Optimize until convergence at this level
    Qstar = tryOptimizingAt(p, initialSOp);
+    if(parameters_.useHuber){ // in this case, there is no optimality verification
+      if(pMin!=pMax)
+         std::cout << "When using robust norm, Shonan only tests a single rank" << std::endl;


dellaert · 2020-11-30T14:19:46Z

gtsam/sfm/ShonanAveraging.cpp

-    : ShonanAveraging<3>(parseMeasurements<Rot3>(g2oFile), parameters) {}
+    : ShonanAveraging<3>(
+          parameters.useHuber
+              ? makeNoiseModelRobust(parseMeasurements<Rot3>(g2oFile))


Create a parseMeasurements version that takes the flag rather than complicate the logic of these constructors

parseMeasurements is a part of dataset.cpp whereas makeNoiseModelRobust is of ShonanAveraging.h. This kind of specialization of parseMeasurements seems finicky to me.

Just add it here as a local helper method

I came up with something neater: maybeRobust(measurements, useHuber). If useHuber is false, just return the measurements.

This also make the constructor more functional, since we can use it across all constructors as simple as maybeRobust(measurements) or maybeRobust(parseMeasurements<Rot3>(g2oFile)).

…'s responsibility

…dels

lucacarlone · 2020-12-05T19:37:56Z

@varunagrawal : let me know if there are pending tasks on this PR that you want me to help with. It seems that except the "throw issue" above, everything else has been solved (thanks!).

varunagrawal · 2020-12-05T20:56:37Z

@lucacarlone I am going to need your review since I've made quite a few changes, and wanted to cross-check with you if the overall code is indeed what you expected.

lucacarlone · 2020-12-09T00:13:52Z

gtsam/sfm/ShonanAveraging.h

@@ -47,12 +47,16 @@ struct GTSAM_EXPORT ShonanAveragingParameters {
  using Anchor = std::pair<size_t, Rot>;

  // Paremeters themselves:


Paremeters -> Parameters

also: following GaussNewtonParams, LevenbergMarquardtParams, etc, shouldn't we call this class "ShonanAveragingParams"?

lucacarlone

I mostly looked at the "robust" aspects and everything seems to be in place.

varunagrawal · 2020-12-29T15:33:58Z

@dellaert gentle reminder.

gtsam/sfm/ShonanAveraging.h

lcarlone added 12 commits September 24, 2020 14:37

Merge branch 'develop' into feature/RobustShonan

baf1b99

adding robust cost function - version 1

6aed168

robust noise in place - test fails due to non-isotropic covariance?

001a55a

solving issue with robust model

73600c8

attempting robustification in Frobenius factor

564e623

added nice unit test

8be6d33

improved test

8cf3bc5

added check and unit test

3734039

added control over minimum rank in ShonanAveraging example, and resol…

6567422

…ved hard-coded sigma in FrobeniusFactor

reverted changes to cproject and language settings

455f81d

fixed typo

1506001

reverted changes to cproject

fa26cf8

ProfFan requested a review from dellaert September 30, 2020 22:59

dellaert requested changes Oct 1, 2020

View reviewed changes

varunagrawal added 3 commits November 9, 2020 10:38

Roustify BinaryMeasurements in a functional way, plus formatting

c99cb14

Remove goto, update docs, formatting

5762ba5

delete old, unused file

dd45797

Merge branch 'develop' into feature/RobustShonan

48699f9

varunagrawal requested a review from dellaert November 10, 2020 22:17

dellaert requested changes Nov 30, 2020

View reviewed changes

varunagrawal added 4 commits November 30, 2020 17:30

Removed unnecessary copy constructor and robust noise model is caller…

fba918c

…'s responsibility

throw runtime errors and explicitly form robust noise model

fd74ae9

makeNoiseModelRobust assumes responsibility for robustifying noise mo…

9d15afa

…dels

use goto flow

3e6efe3

varunagrawal added 4 commits November 30, 2020 17:32

formatting

7997886

fix tests

7391c10

Merge branch 'develop' into feature/RobustShonan

bf93527

Don't throw error for Kappa and test parameter print

a00d370

varunagrawal requested a review from dellaert December 2, 2020 22:17

varunagrawal assigned varunagrawal and lucacarlone Dec 3, 2020

varunagrawal added the feature New proposed feature label Dec 3, 2020

varunagrawal and others added 5 commits December 3, 2020 14:33

mark getters as const

b788fb1

Helper method to robustify measurements

636d5f4

throw error if robust model used but not specified in parameters

06aa4ef

fixed typo

5e82b72

added more explanation on throw

553f569

varunagrawal added 2 commits December 5, 2020 16:06

added flag to enable optimality certification, some formatting

de5adb4

correct flag checking

9a841a2

lucacarlone commented Dec 9, 2020

View reviewed changes

fixed small typos

0c079c6

lucacarlone commented Dec 9, 2020

View reviewed changes

dellaert approved these changes Jan 11, 2021

View reviewed changes

gtsam/sfm/ShonanAveraging.h Outdated Show resolved Hide resolved

follow correct doxygen format

d89dd7d

varunagrawal merged commit fb44e56 into develop Jan 12, 2021

varunagrawal deleted the feature/RobustShonan branch January 12, 2021 02:58

		@@ -47,12 +47,16 @@ struct GTSAM_EXPORT ShonanAveragingParameters {
		using Anchor = std::pair<size_t, Rot>;

		// Paremeters themselves:

Feature/robust shonan #547

Feature/robust shonan #547

Conversation

lucacarlone commented Sep 30, 2020

dellaert left a comment

Choose a reason for hiding this comment

varunagrawal commented Nov 9, 2020

varunagrawal commented Nov 9, 2020

dellaert commented Nov 9, 2020

lucacarlone commented Nov 10, 2020

varunagrawal commented Nov 10, 2020

varunagrawal commented Nov 28, 2020

dellaert 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucacarlone commented Dec 5, 2020

varunagrawal commented Dec 5, 2020

Choose a reason for hiding this comment

lucacarlone left a comment

Choose a reason for hiding this comment

varunagrawal commented Dec 29, 2020