Feature/noisemodel rename functions #269

yetongumich · 2020-04-03T18:09:17Z

Add document for the current implementation of robust noise model
Add additional test cases to verify the all robust models should behave the same as the quadratic function (except for L2WithDeadZone)
Get rid of the distance function in noisemodel, since we already have mahalanobisDistance function. Replace it with error function, which calculates the loss value given the measurement error vector
the noisemodel inside robust should be gaussian instead of a general noisemodel.
rename several variables so that it agrees with the equations in the document
incorporate PR#188 in renaming residual to loss.

This change is

This reverts commit 351c6f8.

dellaert

Cool. Still unclear why error is needed. When we talked I was suggesting to remove distance, not rename it.

dellaert · 2020-04-03T18:43:34Z

gtsam/linear/NoiseModel.cpp

  Vector w = Diagonal::whiten(v); // get noisemodel for constrained elements
  for (size_t i=0; i<dim_; ++i)  // add mu weights on constrained variables
    if (constrained(i)) // whiten makes constrained variables zero
      w[i] = v[i] * sqrt(mu_[i]); // TODO: may want to store sqrt rather than rebuild
-  return w.dot(w);
+  return 0.5 * w.dot(w);


Document change

Sorry I forgot to push. It's in the newest commit

dellaert · 2020-04-03T18:43:48Z

gtsam/linear/NoiseModel.cpp

@@ -654,6 +654,13 @@ void Robust::WhitenSystem(Matrix& A1, Matrix& A2, Matrix& A3, Vector& b) const{
  robust_->reweight(A1,A2,A3,b);
 }

+Robust::shared_ptr Robust::Create(const RobustModel::shared_ptr& robust,


Why are we adding this?

Theoretically, the noisemodel inside robust should be Gaussian. However, the old implementation uses a general noisemodel. After changing it to Gaussian, I added this constructor to fit the old API.

I don't understand, if it is not Gaussian then this conversion will 100% fail.

I think this complicated matters rather than simplifies them. You can’t change the old API, anyway without a version bump. Please undo the change and throw an exception if not (subclass of) Gaussian - and remove this new function.

It must be Gaussian @ProfFan
I've revised by Frank's suggestion in the new commit.

dellaert · 2020-04-03T18:47:09Z

gtsam/linear/JacobianFactor.cpp

@@ -515,7 +515,7 @@ Vector JacobianFactor::error_vector(const VectorValues& c) const {
 double JacobianFactor::error(const VectorValues& c) const {
  Vector e = unweighted_error(c);
  // Use the noise model distance function to get the correct error if available.
-  if (model_) return 0.5 * model_->distance(e);
+  if (model_) return model_->error(e);


Why do we need error? I don’t see it in your document. Also, please rename m to d in your document?

I rethought about it, and I think we still need the error function. Because the way error is calculated varies in different noise models (Gaussian, Robust, Constrained). If we do not have an "error" function inside the noise models, we'll need to implement it in the "error" function of "NoiseModelFactor".

In the document, we already used d to represent the new reweighted Mahalanobis distance

Yetong - we agreed the code would reflect the math. What symbol does “error” correspond to? If there is no corresponding symbol you have two choices: add a symbol in math, or remove this extra function :-)

Got it, will add symbol to document

Actually, I much prefer if you took a hard look at the document and how the robust (and quadratic) models are used in a factor (which does have an error) and my favorite outcome would be : "wow, we don't need this function at all!"

I'm not sure about this. How about discuss about this in our meeting?

ProfFan · 2020-04-05T20:53:17Z

gtsam/linear/JacobianFactor.cpp

@@ -515,7 +515,7 @@ Vector JacobianFactor::error_vector(const VectorValues& c) const {
 double JacobianFactor::error(const VectorValues& c) const {
  Vector e = unweighted_error(c);
  // Use the noise model distance function to get the correct error if available.
-  if (model_) return 0.5 * model_->distance(e);
+  if (model_) return model_->error(e);


ProfFan · 2020-04-05T20:56:42Z

gtsam/linear/NoiseModel.cpp

@@ -654,6 +654,13 @@ void Robust::WhitenSystem(Matrix& A1, Matrix& A2, Matrix& A3, Vector& b) const{
  robust_->reweight(A1,A2,A3,b);
 }

+Robust::shared_ptr Robust::Create(const RobustModel::shared_ptr& robust,


I don't understand, if it is not Gaussian then this conversion will 100% fail.

gtsam/linear/NoiseModel.cpp

ProfFan · 2020-04-05T20:58:50Z

gtsam/linear/NoiseModel.h

@@ -718,15 +724,18 @@ namespace gtsam {
      virtual void WhitenSystem(Matrix& A1, Matrix& A2, Matrix& A3, Vector& b) const;

      virtual Vector unweightedWhiten(const Vector& v) const {
-        return noise_->unweightedWhiten(v);
+        return noise_->whiten(v);


So many things going on. Why this change? I think the PR should be more focused on the new math write-up. Is this related?

After changing the noise_ to gaussian, we can directly use the whiten method.

Here is how things are going on:
In order to agree with the new math, we need to calculate the Mahalanobis distance for the robust noise model. Since mahalanobisDistance is only implmented for Gaussian, I changed the NoiseModel inside robust to Gaussian (previously was general noisemodel), which makes sense since it must be Gaussian.
These changes follow the change of the NoiseModel to Gaussian.

Yes, but unweightedWhiten was not used, right? why is this a thing in this PR? I think we should talk to @mikebosse whether unweightedWhiten is needed at all and have a PR just to remove them, if so?

Sounds good, I'll change it back.

dellaert

Not a satisfactory answer on “error”, see my comment.

dellaert · 2020-04-05T21:02:56Z

gtsam/linear/JacobianFactor.cpp

@@ -515,7 +515,7 @@ Vector JacobianFactor::error_vector(const VectorValues& c) const {
 double JacobianFactor::error(const VectorValues& c) const {
  Vector e = unweighted_error(c);
  // Use the noise model distance function to get the correct error if available.
-  if (model_) return 0.5 * model_->distance(e);
+  if (model_) return model_->error(e);


Yetong - we agreed the code would reflect the math. What symbol does “error” correspond to? If there is no corresponding symbol you have two choices: add a symbol in math, or remove this extra function :-)

dellaert · 2020-04-05T21:03:31Z

gtsam/linear/NoiseModel.cpp

  Vector w = Diagonal::whiten(v); // get noisemodel for constrained elements
  for (size_t i=0; i<dim_; ++i)  // add mu weights on constrained variables
    if (constrained(i)) // whiten makes constrained variables zero
      w[i] = v[i] * sqrt(mu_[i]); // TODO: may want to store sqrt rather than rebuild
-  return w.dot(w);
+  return 0.5 * w.dot(w);


dellaert · 2020-04-05T21:07:02Z

gtsam/linear/NoiseModel.cpp

@@ -654,6 +654,13 @@ void Robust::WhitenSystem(Matrix& A1, Matrix& A2, Matrix& A3, Vector& b) const{
  robust_->reweight(A1,A2,A3,b);
 }

+Robust::shared_ptr Robust::Create(const RobustModel::shared_ptr& robust,


I think this complicated matters rather than simplifies them. You can’t change the old API, anyway without a version bump. Please undo the change and throw an exception if not (subclass of) Gaussian - and remove this new function.

dellaert · 2020-04-05T21:09:23Z

gtsam/linear/NoiseModel.h

@@ -718,15 +724,18 @@ namespace gtsam {
      virtual void WhitenSystem(Matrix& A1, Matrix& A2, Matrix& A3, Vector& b) const;

      virtual Vector unweightedWhiten(const Vector& v) const {
-        return noise_->unweightedWhiten(v);
+        return noise_->whiten(v);


So many things going on. Why this change? I think the PR should be more focused on the new math write-up. Is this related?

dellaert · 2020-04-13T19:25:47Z

I just checked the updated pdf, and so I still think there is no need for "error" in the noise models, only "loss". Which will take the Mahalanobis distance.

yetongumich · 2020-04-13T20:00:13Z

I just checked the updated pdf, and so I still think there is no need for "error" in the noise models, only "loss". Which will take the Mahalanobis distance.

I see. Currently loss is only defined for robust noise models. Should we make it a general function in noiseModel::Base?

dellaert · 2020-04-13T20:05:28Z

That would solve it, right?

yetongumich · 2020-04-13T20:06:41Z

Yes

…_rename_functions

ProfFan · 2020-07-09T18:29:21Z

@yetongumich This is blocking 4.0.3 now, could you take a hard look at this PR ASAP?

yetongumich · 2020-07-10T03:09:08Z

@yetongumich This is blocking 4.0.3 now, could you take a hard look at this PR ASAP?

Yes, will take a close look at it this weekend!

yetongumich · 2020-07-12T18:46:38Z

The previous PR was previously blocked by not passing a test in testNonlinearOptimzier.cpp (RobustMeanCalculation).

The modification is reducing the relative error threshold in the Dogleg optimizer parameters. This modification is required because the error of robust noise model increases more slowly as the error becomes larger. Thus, the relative error decrease tend to be small. This issue is not exposed since there was another issue that the error of robust noise model is falsely multiplied by 0.5.

In this PR, since the error of noise model is corrected, the test is also modified to make it pass.

ProfFan · 2020-07-13T02:58:20Z

@yetongumich The Cython wrapper is broken:

[ 91%] Building CXX object gtsam_unstable/CMakeFiles/gtsam_unstable.dir/nonlinear/ConcurrentFilteringAndSmoothing.cpp.o
/home/travis/build/borglab/gtsam/build/cython/gtsam/gtsam.cpp: In function ‘int __pyx_pf_5gtsam_5gtsam_17noiseModel_Robust___init__(__pyx_obj_5gtsam_5gtsam_noiseModel_Robust*, PyObject*, PyObject*)’:
/home/travis/build/borglab/gtsam/build/cython/gtsam/gtsam.cpp:148509:130: error: no matching function for call to ‘gtsam::noiseModel::Robust::Robust(boost::shared_ptr<gtsam::noiseModel::mEstimator::Base>&, boost::shared_ptr<gtsam::noiseModel::Base>&)’
         __pyx_t_10 = new gtsam::noiseModel::Robust(__pyx_v_robust->CnoiseModel_mEstimator_Base_, __pyx_v_noise->CnoiseModel_Base_);
                                                                                                                                  ^
In file included from /home/travis/build/borglab/gtsam/build/cython/gtsam/gtsam.cpp:684:0:
/home/travis/build/borglab/gtsam/gtsam/linear/NoiseModel.h:696:7: note: candidate: gtsam::noiseModel::Robust::Robust(gtsam::noiseModel::mEstimator::Base::shared_ptr, gtsam::noiseModel::Gaussian::shared_ptr)
       Robust(const RobustModel::shared_ptr robust, const NoiseModel::shared_ptr noise)
       ^
/home/travis/build/borglab/gtsam/gtsam/linear/NoiseModel.h:696:7: note:   no known conversion for argument 2 from ‘boost::shared_ptr<gtsam::noiseModel::Base>’ to ‘gtsam::noiseModel::Gaussian::shared_ptr {aka boost::shared_ptr<gtsam::noiseModel::Gaussian>}’
/home/travis/build/borglab/gtsam/gtsam/linear/NoiseModel.h:693:7: note: candidate: gtsam::noiseModel::Robust::Robust()
       Robust() {};
       ^
/home/travis/build/borglab/gtsam/gtsam/linear/NoiseModel.h:693:7: note:   candidate expects 0 arguments, 2 provided
/home/travis/build/borglab/gtsam/gtsam/linear/NoiseModel.h:679:11: note: candidate: gtsam::noiseModel::Robust::Robust(const gtsam::noiseModel::Robust&)
     class GTSAM_EXPORT Robust : public Base {
           ^
/home/travis/build/borglab/gtsam/gtsam/linear/NoiseModel.h:679:11: note:   candidate expects 1 argument, 2 provided
cython/CMakeFiles/cythonize_gtsam.dir/build.make:72: recipe for target 'cython/CMakeFiles/cythonize_gtsam.dir/gtsam/gtsam.cpp.o' failed
make[2]: *** [cython/CMakeFiles/cythonize_gtsam.dir/gtsam/gtsam.cpp.o] Error 1
CMakeFiles/Makefile2:7967: recipe for target 'cython/CMakeFiles/cythonize_gtsam.dir/all' failed
make[1]: *** [cython/CMakeFiles/cythonize_gtsam.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....

ProfFan · 2020-07-13T02:58:48Z

I think this is due to a breaking API change

…override

yetongumich · 2020-07-13T06:03:22Z

Yes. I noticed that, and resolved it in the newest commit. Should be good for the travis check.

dellaert

Awesome. Can you also take a look at @mikebosse 's PR after this was merged, and see whether that is completely subsumed in here? Whatever the outcome, please comment in that PR with your take and let @mikebosse close the PR, if needed, or comment on what in that PR still needs to be merged. I'd propose @mikebosse should first merge in develop to see the updated diff.

Looked over and seems all comments addressed, don't want to distract you from 4.1

dellaert · 2020-07-13T17:16:47Z

@yetongumich please delete the branch yourself, I don't have the option it seems. It's weird but there seems to only be a commit.

yetongumich · 2020-07-14T03:26:27Z

@yetongumich please delete the branch yourself, I don't have the option it seems. It's weird but there seems to only be a commit.

I cannot delete the branch. It says can't delete because it has an open PR. Looks like some bug with github.

yetongumich added 8 commits April 1, 2020 20:07

remove distance in noisemodel, replace with error

2015396

add test on robust loss functions to behave like quadratic

3b183e2

Revert "add implementation for deprecated Mahalanobis"

227bff6

This reverts commit 351c6f8.

merge with mahalanobis renaming

9d60c50

re-add implemntation for deprecated Mahalanobis

d86bab0

renamed mahalanobisDistance

12b0267

add document for robust noise model

9e4be90

noise in robust should be gaussian, change variable names

12930ac

yetongumich requested a review from dellaert April 3, 2020 18:09

Merge branch 'develop' into feature/noisemodel_rename_functions

a176962

yetongumich requested a review from ProfFan April 3, 2020 18:25

dellaert requested changes Apr 3, 2020

View reviewed changes

yetongumich added 2 commits April 5, 2020 14:20

revised comments

efc264d

change test cases to use the updated names, remove 2nd Create in robust

90b286f

ProfFan previously requested changes Apr 5, 2020

View reviewed changes

dellaert requested changes Apr 5, 2020

View reviewed changes

yetongumich added 5 commits April 5, 2020 18:44

modified document, remove commented function, add deprecated distance

3f8bb10

check if noisemodel is gaussian, if not throw exception

99761a1

change unweightedwhiten back

646a4b7

rename residual to loss

efcc5c9

modified document

c54346c

dellaert mentioned this pull request Jul 7, 2020

4.0.3 Release Tracking Issue #357

Closed

26 tasks

Merge remote-tracking branch 'origin/develop' into feature/noisemodel…

3d8641c

…_rename_functions

modified test

eb1a2b8

ProfFan requested review from dellaert and ProfFan July 13, 2020 02:57

yetongumich added 4 commits July 12, 2020 23:05

deprecate error in noisemodel, use loss instead; revise virtual with …

7db7455

…override

deprecate error in noisemodel, use loss instead; revise virtual with …

ec99349

…override

modify testNoiseModel to use loss instead of error

52927c1

change doc

529f609

dellaert approved these changes Jul 13, 2020

View reviewed changes

dellaert merged commit 73209e6 into develop Jul 13, 2020

yetongumich mentioned this pull request Jul 14, 2020

Make mEstimator variable names consistent with math #188

Merged

varunagrawal deleted the feature/noisemodel_rename_functions branch October 22, 2023 19:54

Feature/noisemodel rename functions #269

Feature/noisemodel rename functions #269

Conversation

yetongumich commented Apr 3, 2020 • edited Loading

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

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

dellaert commented Apr 13, 2020

yetongumich commented Apr 13, 2020

dellaert commented Apr 13, 2020

yetongumich commented Apr 13, 2020

ProfFan commented Jul 9, 2020

yetongumich commented Jul 10, 2020

yetongumich commented Jul 12, 2020

ProfFan commented Jul 13, 2020

ProfFan commented Jul 13, 2020

yetongumich commented Jul 13, 2020

dellaert left a comment

Choose a reason for hiding this comment

dellaert commented Jul 13, 2020

yetongumich commented Jul 14, 2020

yetongumich commented Apr 3, 2020 •

edited

Loading