Shonan edits #769
Shonan edits #769
Conversation
travisdriver
commented
Jan 22, 2024
travisdriver
commented
- Weight by inverse of # inlier matches
- Add Huber such that the weight k can be tuned
| else: | ||
| edges_mst.append((i, j)) | ||
| iter += 1 | ||
| # if iter >= max_iters: |
There was a problem hiding this comment.
nit: could we remove the commented out code?
|
|
||
| # Build global rotations from MST. | ||
| # TODO (travisdriver): This is simple but very inefficient. Use something else. | ||
| i_mst, j_mst = Tcsr.nonzero() |
There was a problem hiding this comment.
could we use a different variable name than Tcsr? didn't quite understand what it stood for, and has non-standard python variable naming
| """ | ||
|
|
||
| logger.info("Running GNC rotation averaging...") | ||
| #shonan = ShonanAveraging3(between_factors, self.__get_shonan_params()) |
There was a problem hiding this comment.
nit: can we remove the commented out code here?
| def __init__(self, two_view_rotation_sigma: float = _DEFAULT_TWO_VIEW_ROTATION_SIGMA) -> None: | ||
| """Initializes module. | ||
|
|
||
| Note: `p_min` and `p_max` describe the minimum and maximum relaxation rank. |
There was a problem hiding this comment.
is the p_min reference stale? Doesn't apply to GNC, right?
| @@ -0,0 +1,285 @@ | |||
| """Shonan Rotation Averaging. | |||
There was a problem hiding this comment.
nit: is the module docstring stale up here?
| graph: gtsam.NonlinearFactorGraph = self.__graph_from_2view_relative_rotations( | ||
| i2Ri1_dict, old_to_new_idxes | ||
| ) | ||
| # between_factors.extend(self._between_factors_from_pose_priors(i1Ti2_priors, old_to_new_idxes)) |
There was a problem hiding this comment.
nit: commented out code
| in the input to run_rotation_averaging(). | ||
| """ | ||
| if len(i2Ri1_dict) == 0: | ||
| logger.warning("Shonan cannot proceed: No cycle-consistent triplets found after filtering.") |
| @@ -0,0 +1,238 @@ | |||
| """Shonan Rotation Averaging. | |||
There was a problem hiding this comment.
nit: should the module docstring mention Shonan & GNC?
| self, num_connected_nodes: int, between_factors: BetweenFactorPose3s | ||
| self, num_connected_nodes: int, | ||
| measurements: gtsam.BinaryMeasurementsRot3, | ||
| measurements_not_robust: gtsam.BinaryMeasurementsRot3, |
There was a problem hiding this comment.
Do we need the separate arguments for measurements vs. measurements_not_robust
Can we consolidate into one?
| ) | ||
| between_factors.extend(self._between_factors_from_pose_priors(i1Ti2_priors, old_to_new_idxes)) | ||
| # between_factors.extend(self._between_factors_from_pose_priors(i1Ti2_priors, old_to_new_idxes)) |
There was a problem hiding this comment.
nit: can we remove the commented out code here?
| # ignore translation during rotation averaging | ||
| i2Ti1 = Pose3(i2Ri1, np.zeros(3)) | ||
| noise_model = gtsam.noiseModel.Isotropic.Sigma(ROT3_DOF, 1 / corr_idxs[(i1, i2)].shape[0]) |
There was a problem hiding this comment.
@travisdriver can you add a flag that allows us to run without using inliers as uncertainty? e.g. to support an ablation as we have it currently, or in case it can't converge with the uncertainty-weighted cost
|
|
||
| return between_factors | ||
|
|
||
| def _run_with_consecutive_ordering( |
There was a problem hiding this comment.
Can any of this code be shared from a parent class? Are there functional differences for this helper method?
|
|
||
| return between_factors | ||
|
|
||
| def _run_with_consecutive_ordering( |
There was a problem hiding this comment.
Can any of this code be shared from a parent class? Are there functional differences for this helper method?
| initial = shonan.initializeRandomly() | ||
|
|
||
| # Run with robust error. | ||
| shonan = ShonanAveraging3(measurements, self.__get_shonan_params()) |
There was a problem hiding this comment.
why do we need to instantiate the ShonanAveraging3 object twice in a row?
