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.

Sign up for GitHub

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

Jump to bottom

changed barcsq to a vector to allow each factor to have a different inlier threshold #684

Merged
merged 7 commits into from
Feb 3, 2021
Merged

changed barcsq to a vector to allow each factor to have a different inlier threshold #684

merged 7 commits into from
Feb 3, 2021

Conversation

lucacarlone
Copy link
Contributor

@lucacarlone lucacarlone commented Jan 23, 2021 •
edited
Loading

I realized that having a default inlier threshold equal to 1 does not make sense for the gtsam user since:

  1. "1" is not a reasonable error threshold (for Gaussian noise, we should use the chi2inv to establish a reasonable maximum error) and
  2. the factors can have different dimensions, hence it is desirable to be able to set a different threshold for each factor.

The PR involves:

  • making barcsq into a vector
  • adding/adjusting the unit tests accordingly
  • initializing barcsq using chi2inv

@lucacarlone lucacarlone added the enhancement Improvement to GTSAM label Jan 23, 2021
@lucacarlone
Copy link
Contributor Author

@dellaert @jingnanshi @pantonante : I completed a small enhancement for GNC. please take a look and approve if you like it.

@dellaert : please take a look at the 2 lines commented at line 658 of testGncOptimizer. It seems that "dim()" segfaults when we use the default noise model in the factor. This might be a bug in gtsam.

dellaert
dellaert approved these changes Jan 27, 2021
View reviewed changes
Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Sorry about delay, teaching large class ;-)

dellaert
dellaert requested changes Jan 27, 2021
View reviewed changes
Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR is fine, but changing to "request changes" as I just noticed CI fails on Linux. Probably something small but needs to be fixed before we can merge.

@dellaert
Copy link
Member

@dellaert : please take a look at the 2 lines commented at line 658 of testGncOptimizer. It seems that "dim()" segfaults when we use the default noise model in the factor. This might be a bug in gtsam.

Is that why CI fails? Let me check.

@lucacarlone
Copy link
Contributor Author

@dellaert: let me know if you get some insight into what's failing in CI. I took another pass and cleaned up the comments. @jingnanshi @pantonante : feel free to approve if this looks good.

jingnanshi
jingnanshi reviewed Jan 30, 2021
View reviewed changes
gtsam/nonlinear/GncOptimizer.h Outdated
@@ -29,7 +29,18 @@
#include <gtsam/nonlinear/GncParams.h>
#include <gtsam/nonlinear/NonlinearFactorGraph.h>

//#include <boost/math/distributions/inverse_chi_squared.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to remove this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch - done!

jingnanshi
jingnanshi approved these changes Jan 30, 2021
View reviewed changes
Copy link
Contributor

@jingnanshi jingnanshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good! I left one comment regarding a commented out include file.

@dellaert
Copy link
Member

Still some failures on Ubuntu, though?

@lucacarlone
Copy link
Contributor Author

@dellaert : I only changed comments so far, I hoped you could look into the dim() issue. Let me know if that's not feasible.

@dellaert
Copy link
Member

@dellaert : I only changed comments so far, I hoped you could look into the dim() issue. Let me know if that's not feasible.

Dim issue is separate, I think. CI failure is unrelated

@lucacarlone
Copy link
Contributor Author

@dellaert : @jingnanshi fixed the issue! at this point, this branch should be good to go

@dellaert dellaert merged commit 8261326 into develop Feb 3, 2021
@dellaert dellaert deleted the feature/gncImprovements branch December 29, 2021 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jingnanshi jingnanshi jingnanshi approved these changes

@dellaert dellaert dellaert approved these changes

@pantonante pantonante Awaiting requested review from pantonante

Assignees
No one assigned
Labels
enhancement Improvement to GTSAM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lucacarlone @dellaert @jingnanshi