Skip to content
New issue

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

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

Already on GitHub? # to your account

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

Conversation

lucacarlone
Copy link
Contributor

@lucacarlone lucacarlone commented Jan 23, 2021

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.

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 ;-)

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.

@@ -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!

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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement Improvement to GTSAM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants