Make mEstimator variable names consistent with math #188

michaelbosse · 2019-12-10T21:53:24Z

Rename mEstimator residual to loss so that it is not confused
with lsq residual vector Ax-b.
Rename distance to squaredDistance when appropriate.
Removed erroneous 0.5 factor from NoiseModelFactor::weight()
Fix bug in mEstimator::Null::loss()

This change is

Rename mEstimator residual to loss so that it is not confused with lsq residual vector Ax-b. Rename distance to squaredDistance when appropriate. Removed erroneous 0.5 factor from NoiseModelFactor::weight() Fix bug in mEstimator::Null::loss()

dellaert · 2019-12-11T23:01:17Z

 91/175 Test  #91: testNoiseModel .....................***Failed    0.03 sec
/Users/travis/build/borglab/gtsam/gtsam/linear/tests/testNoiseModel.cpp:680: Failure: "expected 0.5 but was: 1" 
There were 1 failures

thduynguyen

@michaelbosse: The filename LossFunctions.* is very generic and obscure. Could we change it to MEstimators.*?

Reviewable status: 0 of 8 files reviewed, all discussions resolved

gtsam/linear/NoiseModel.h

varunagrawal · 2020-03-18T14:58:26Z

gtsam/nonlinear/NonlinearFactor.cpp

@@ -106,7 +106,7 @@ double NoiseModelFactor::weight(const Values& c) const {
    if (noiseModel_) {
      const Vector b = unwhitenedError(c);
      check(noiseModel_, b.size());
-      return 0.5 * noiseModel_->weight(b);


This might cause regressions. 🙁

So, this is very significant, and seems fishy. Too bad this stuff did not get resolved when it was fresh in our minds, but now we have to re-argue this, and why it was not in @yetongumich PR.

I agree. I believe the 0.5 should NOT be present, since if the noiseModel_ is not robust, the weight function should simply be 1 (which in NoiseModel.h it is). If the noise model is robust, then it should just return the weight value for that particular M-estimator given the residuals.

Interestingly, there isn't any test for this 😢

Interestingly, there isn't any reasonable usages for this function either...

Also, I think the 0.5 factor should not be there. @dellaert

The only usage of this function is in gtsam_unstable/partition, so I suggest this function to be moved to unstable as a standalone function there.

Plus, this change is correct.

dellaert · 2020-03-18T18:21:47Z

@varunagrawal, @mikebosse

@ProfFan and @yetongumich are currently examining/fixing all issues wrpt to Robust errors. We will get to this branch in due time but for now let's keep it dormant.

This reverts commit 4e3542a.

dellaert · 2020-07-14T01:15:17Z

@mikebosse you probably have given up on this, but we merged a very similar PR yesterday based on this PR (and a lot of conversation about robust models). If you have a minute please merge in develop and see what still remains? You might have to fix some conflicts, please resolve in favor of "distance".

yetongumich · 2020-07-14T03:12:10Z

Hi @mikebosse , In PR #269 , we have incorporated your changes by renaming residual to loss. In addition, we have also deprecated the "distance" function, and create mahalanobisDistance and squaredMahalanobisDistance, so that the code can better agree with math.

dellaert · 2020-08-17T12:11:04Z

@yetongumich Please create a branch off of this branch and resolve conflicts with develop, then close this Pr and submit other branch? I would like to see what was in this PR that we have not yet done in your PR, inspired by this one. Probably nothing, but let’s be sure and clean this up.

yetongumich · 2020-08-18T02:31:34Z

@yetongumich Please create a branch off of this branch and resolve conflicts with develop, then close this Pr and submit other branch? I would like to see what was in this PR that we have not yet done in your PR, inspired by this one. Probably nothing, but let’s be sure and clean this up.

@dellaert I think this PR merges directly into develop. May I know if we still need to create a new branch off this one?

As I went through "Files Changed" of this PR, the changes include

rename "residual" to "loss" (already incorporated in our PR)
rename "distance" to "squaredDistance" (in our PR, it's renamed as squaredMahalanobisDistance)
remove "distance_non_whitened" in robust noise model (this function is never called, and is also removed in the current develop branch)
resolve the issue that the "error" function in noisemodelfactor missed a 0.5 multiplier (which is already incorporated in our PR)
also add the 0.5 multiplier to the "weight" function in noisemodelfactor so that the robust noise model can perform similar to normal noise model when the error is small (this is already incorporated in our PR as well)

All the changes should be already incorporated in the current develop branch.

dellaert · 2020-08-18T14:49:00Z

@yetongumich it too more energy to reply above than it takes to create the PR :-)
We need a branch and PR because we cannot sync a fork. And while I believe you, I'd like to see a PR were develop has been merged in so we are sure everything is included before we merge.

gtsam/linear/LossFunctions.h

varunagrawal · 2021-01-05T16:36:42Z

I went ahead and synced the fork so we can easily check what is left. Moreover, I also uncommented and fixed some tests which should work given the updates to the robust losses.

varunagrawal · 2021-01-05T16:38:46Z

@dellaert @yetongumich looks like you guys got everything! 🎉

Can I get a review so we can (finally) merge this in?

dellaert

You'll have to update me on this one...

varunagrawal · 2021-01-05T19:49:08Z

I just merged in the latest develop into my local branch, as you asked in a previous comment. I resolved conflicts by picking squaredMahalanobisDistance instead of squaredDistance and distance instead of error.
Finally I updated the failing tests.

@michaelbosse's PR pretty much covered exactly the changes you, @yetongumich and @ProfFan had made previously. I saw that the changes were minimal so I pushed them up to this PR.

dellaert · 2021-01-11T17:07:04Z

tests/testNonlinearFactor.cpp

@@ -101,6 +101,82 @@ TEST( NonlinearFactor, NonlinearFactor )
  DOUBLES_EQUAL(expected,actual,0.00000001);
 }

+/* ************************************************************************* */
+TEST(NonlinearFactor, Weight) {


@yetongumich and @ProfFan
You guys did the deep dive into robust models. Can you please weigh in?

dellaert · 2021-01-18T20:33:55Z

@ProfFan @yetongumich "I see no problem" will not cut it :-) You guys did a deep dive into this a while ago, and should be able to decisively argue whether the 0.5 factor should be there, or not there. In either case, it will need a justification.

varunagrawal · 2021-03-22T04:10:53Z

Can we get this merged in soon please? 🙂

varunagrawal · 2021-08-01T14:22:15Z

@ProfFan @yetongumich let's get this one in?

ProfFan · 2021-10-07T18:18:44Z

@yetongumich Could you take a look at this? We need this one for 4.1 release.

yetongumich · 2021-10-08T04:42:22Z

@yetongumich Could you take a look at this? We need this one for 4.1 release.

Sorry for the late response. I went through this PR again, and we should be good with merging.

varunagrawal · 2021-10-08T07:25:36Z

@ProfFan @yetongumich "I see no problem" will not cut it :-) You guys did a deep dive into this a while ago, and should be able to decisively argue whether the 0.5 factor should be there, or not there. In either case, it will need a justification.

We still need a justification. 🙂

dellaert · 2021-10-08T18:28:06Z

Whatever the outcome is, it should be documented somewhere so we don’t have to go through this again

ProfFan

The weight function is not used anywhere other than gtsam_unstable, is untested, so I suggest it either be removed or moved to unstable as a standalone function. We can also merge this PR as-is. @dellaert

ProfFan · 2021-11-21T19:00:03Z

@dellaert @yetongumich @varunagrawal I am merging this if there are no objections. The weight in NoiseModelFactor should be the same as the one in the robust noise model.

varunagrawal · 2021-11-22T21:31:22Z

Yeah go for it. I don't know why @yetongumich isn't responding but we can't have him delay progress.

varunagrawal

LGTM

ProfFan · 2021-11-22T21:41:32Z

We need @dellaert to approve

dellaert · 2021-11-24T02:13:39Z

We need @dellaert to approve

@yetongumich needs to look at this. Please talk to him on slack/chat/whatever.

yetongumich · 2021-11-30T00:10:31Z

tests/testNonlinearFactor.cpp

@@ -101,6 +101,82 @@ TEST( NonlinearFactor, NonlinearFactor )
  DOUBLES_EQUAL(expected,actual,0.00000001);
 }

+/* ************************************************************************* */
+TEST(NonlinearFactor, Weight) {


varunagrawal · 2021-11-30T00:48:20Z

LGTM

@yetongumich you need to provide the answers for the above questions that Frank posed. This PR is basically waiting on you. 🙂

dellaert · 2021-11-30T02:15:49Z

LGTM

@yetongumich you need to provide the answers for the above questions that Frank posed. This PR is basically waiting on you. 🙂

Agreed!

yetongumich · 2021-11-30T03:15:47Z

Justification for why we should remove the 0.5:

The behavior of "weight" function in NoiseModelFactor should be the same as the "weight" function in NoiseModels. In robust NoiseModel, weights will change by values throughout iterations, while for gaussian NoiseModel, weight shall just stay 1. Therefore, the "weight" function should return the weight of the noisemodel, and should not have the 0.5 multiplier. Another justification is that when there's no noisemodel, the weight is 1, which is the same weight as Gaussian noise.

I don't know why there is a multiplier of 0.5 in the old code, maybe because of copy-paste from the error function, but I think we should have the 0.5 removed in the updated patch.

ProfFan · 2021-11-30T03:24:50Z

@dellaert I agree with @yetongumich 's answer

ProfFan · 2021-11-30T16:04:36Z

@dellaert We need your approval to merge.

dellaert

Phew, this took a while (Yetong!) but let's finally land.

Make variable names consistent with math

23f5c8c

Rename mEstimator residual to loss so that it is not confused with lsq residual vector Ax-b. Rename distance to squaredDistance when appropriate. Removed erroneous 0.5 factor from NoiseModelFactor::weight() Fix bug in mEstimator::Null::loss()

thduynguyen reviewed Dec 12, 2019

View reviewed changes

varunagrawal added the enhancement Improvement to GTSAM label Feb 23, 2020

varunagrawal self-requested a review March 10, 2020 11:18

varunagrawal reviewed Mar 18, 2020

View reviewed changes

yetongumich added a commit that referenced this pull request Apr 2, 2020

renamed residual to loss, follow PR #188

4e3542a

yetongumich added a commit that referenced this pull request Apr 2, 2020

Revert "renamed residual to loss, follow PR #188"

0acca9f

This reverts commit 4e3542a.

akshay-krishnan pushed a commit that referenced this pull request May 3, 2020

renamed residual to loss, follow PR #188

f4da76c

akshay-krishnan pushed a commit that referenced this pull request May 3, 2020

Revert "renamed residual to loss, follow PR #188"

886b874

This reverts commit 4e3542a.

varunagrawal added this to the GTSAM 4.1 milestone Jul 13, 2020

dellaert modified the milestones: GTSAM 4.1, GTSAM 4.1.1 Jul 14, 2020

varunagrawal reviewed Jan 5, 2021

View reviewed changes

gtsam/linear/LossFunctions.h Outdated Show resolved Hide resolved

varunagrawal reviewed Jan 5, 2021

View reviewed changes

gtsam/linear/LossFunctions.h Outdated Show resolved Hide resolved

Merge branch 'develop' into fix_bug_robust_residuals

102123c

varunagrawal requested a review from dellaert January 5, 2021 16:57

dellaert reviewed Jan 5, 2021

View reviewed changes

varunagrawal requested a review from dellaert January 5, 2021 19:49

varunagrawal added 2 commits January 6, 2021 11:46

update completed TODOs

a5ff750

add unit test for NonlinearFactor weight with different noise models

6e46b72

dellaert requested changes Jan 11, 2021

View reviewed changes

varunagrawal requested review from yetongumich and ProfFan January 11, 2021 18:52

varunagrawal mentioned this pull request Jul 25, 2021

Drafting 4.1 Release #824

Closed

14 tasks

varunagrawal self-assigned this Sep 18, 2021

ProfFan approved these changes Nov 19, 2021

View reviewed changes

ProfFan closed this Nov 21, 2021

ProfFan reopened this Nov 21, 2021

ProfFan approved these changes Nov 22, 2021

View reviewed changes

ProfFan requested a review from dellaert November 22, 2021 20:55

varunagrawal approved these changes Nov 22, 2021

View reviewed changes

yetongumich approved these changes Nov 30, 2021

View reviewed changes

dellaert approved these changes Nov 30, 2021

View reviewed changes

dellaert merged commit 613b161 into borglab:develop Nov 30, 2021

Make mEstimator variable names consistent with math #188

Make mEstimator variable names consistent with math #188

Conversation

michaelbosse commented Dec 10, 2019 • edited by dellaert Loading

dellaert commented Dec 11, 2019

thduynguyen 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

dellaert commented Mar 18, 2020

dellaert commented Jul 14, 2020

yetongumich commented Jul 14, 2020

dellaert commented Aug 17, 2020

yetongumich commented Aug 18, 2020

dellaert commented Aug 18, 2020

varunagrawal commented Jan 5, 2021

varunagrawal commented Jan 5, 2021

dellaert left a comment

Choose a reason for hiding this comment

varunagrawal commented Jan 5, 2021

Choose a reason for hiding this comment

dellaert commented Jan 18, 2021

varunagrawal commented Mar 22, 2021

varunagrawal commented Aug 1, 2021

ProfFan commented Oct 7, 2021

yetongumich commented Oct 8, 2021

varunagrawal commented Oct 8, 2021

dellaert commented Oct 8, 2021

ProfFan left a comment

Choose a reason for hiding this comment

ProfFan commented Nov 21, 2021

varunagrawal commented Nov 22, 2021

varunagrawal left a comment

Choose a reason for hiding this comment

ProfFan commented Nov 22, 2021

dellaert commented Nov 24, 2021

Choose a reason for hiding this comment

varunagrawal commented Nov 30, 2021

dellaert commented Nov 30, 2021

yetongumich commented Nov 30, 2021

ProfFan commented Nov 30, 2021

ProfFan commented Nov 30, 2021

dellaert left a comment

Choose a reason for hiding this comment

michaelbosse commented Dec 10, 2019 •

edited by dellaert

Loading