Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rethink gradient values for Constrained Factors #34

Open
varunagrawal opened this issue May 31, 2019 · 9 comments
Open

Rethink gradient values for Constrained Factors #34

varunagrawal opened this issue May 31, 2019 · 9 comments
Assignees
Labels
bug Bug report

Comments

@varunagrawal
Copy link
Collaborator

The behavior of gradients, specifically gradientAtZero methods for constrained factors/noise models needs to be re-evaluated. Concretely, gradients for constrained factors should be undefined since they are point functions with 0 covariance.

This behavior is not respected in the BayesTree data structure since while the constrained factors set the gradient to zero, the BayesTree aggregates the gradients from other factors on a variable without considering the constraints.

@dellaert
Copy link
Member

Note: this is a bug as it makes iSAM2 with Dogleg do the wrong thing for constrained factors.

@ProfFan
Copy link
Collaborator

ProfFan commented Sep 23, 2019

Should this be in 4.1? @dellaert

@dellaert
Copy link
Member

Let's discuss w @varunagrawal

@ProfFan
Copy link
Collaborator

ProfFan commented Sep 26, 2019

In my recent bughunting I got really confused about this piece of code in NoiseModel.cpp:

namespace internal {
// switch precisions and invsigmas to finite value
// TODO: why?? And, why not just ask s==0.0 below ?
static void fix(const Vector& sigmas, Vector& precisions, Vector& invsigmas) {
  for (Vector::Index i = 0; i < sigmas.size(); ++i)
    if (!std::isfinite(1. / sigmas[i])) {
      precisions[i] = 0.0;
      invsigmas[i] = 0.0;
    }
}
}

If the sigma is 0, invsigma should be Inf instead of 0.

Also note that division by zero is actually UB in C++.

@dellaert
Copy link
Member

Yeah... Where is that called from?

@ProfFan
Copy link
Collaborator

ProfFan commented Sep 26, 2019

@dellaert

Constrained::Constrained(const Vector& sigmas)
  : Diagonal(sigmas), mu_(Vector::Constant(sigmas.size(), 1000.0)) {
  internal::fix(sigmas, precisions_, invsigmas_);
}

@ProfFan
Copy link
Collaborator

ProfFan commented Dec 23, 2019

#121

@ProfFan
Copy link
Collaborator

ProfFan commented Feb 19, 2020

@dellaert Ping

varunagrawal added a commit that referenced this issue Mar 10, 2021
b0eb968f2 Completely handle Matlab wrapping (#34)
4ad71812a Merge pull request #33 from borglab/fix/script-destination
0c832eed7 reverted from TYPE to DESTINATION for wrapper scripts
10e1efd6f Merge pull request #32 from ayushbaid/feature/pickle
55d5d7fbe ignoring pickle in matlab wrap
8d70c7fe2 adding newlines
dee8aaee3 adding markers for pickling
46fc45d82 separating out the marker for pickle
d37b8a972 Merge pull request #31 from ayushbaid/feature/pickle
efd4a0fb4 removing tab
42fd231f3 adding newline for test compare
0aa316150 removing extra brackets
7fe1d7d0f adding pickle code to expected file
9c3ab7a8b fixing string format for new classname
2f89284e8 moving pickle support with the serialization code
5a8abc916 adding pickle support for select classes using serialization

git-subtree-dir: wrap
git-subtree-split: b0eb968f29a2261700361599cab2823221dd0f9d
@varunagrawal varunagrawal mentioned this issue Jul 25, 2021
14 tasks
@ProfFan
Copy link
Collaborator

ProfFan commented Oct 7, 2021

We need to look at this after iMHS...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug report
Projects
None yet
Development

No branches or pull requests

3 participants