-
Notifications
You must be signed in to change notification settings - Fork 52
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
Chordal initialization in Shonan #822
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Maybe paste performance increase in comments.
But still some things to figure out:
- Karcher or Anchor? Probably anchor (first key if not given)
- Huber, certify, pMax
- anchors from ground truth
And see if that fixes the weirdness in those datasets.
gtsfm/averaging/rotation/shonan.py
Outdated
@@ -52,14 +57,17 @@ def __init__( | |||
super().__init__() | |||
self._two_view_rotation_sigma = two_view_rotation_sigma | |||
self._p_min = 3 | |||
self._p_max = 64 | |||
self._p_max = 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
64 is excessive, but 3 is maybe a mistake :-) We should monitor the number on a few datasets and choose based on that.
gtsfm/averaging/rotation/shonan.py
Outdated
shonan_params = ShonanAveragingParameters3(lm_params) | ||
shonan_params.setUseHuber(False) | ||
shonan_params.setCertifyOptimality(True) | ||
shonan_params.setUseHuber(True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These things should not change, right?
Initial values as a gtsam.Values object. | ||
""" | ||
graph = gtsam.NonlinearFactorGraph() | ||
anchor_key = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we should figure out anchors in ground truth and use those if available.
Did latest changes fix those datasets? |
@akshay-krishnan can we merge? |
Yeah, the unit test failure also happens on master, I'll fix that in a different PR |
Actually, I should first merge the TA PR |
Yay !!!! |
No description provided.