Initialize shonan using minimum spanning tree #777
Conversation
|
I cannot make sense of the results. For datasets where GTSFM performs poorly, we do good. However, we see some regression on other datasets.
|
There was a problem hiding this comment.
nit: would spanning_tree be a better name for this file?
There was a problem hiding this comment.
I indented to keep all rotations related util functions here. I feel this is not as generic to be named a spanning tree right now because the args are rotations and not a generic type.
|
@ayushbaid CI appears to still be failing: |
gtsfm/utils/rotation.py
Outdated
| from gtsam import Rot3 | ||
|
|
||
|
|
||
| def random_rotation() -> Rot3: |
There was a problem hiding this comment.
@ayushbaid appears to be unused, right?
tests/utils/test_rotation_utils.py
Outdated
|
|
||
| class TestRotationUtil(unittest.TestCase): | ||
| def test_mst_initialization(self): | ||
| """Test for 4 poses in a circle, with a pose connected all others.""" |
There was a problem hiding this comment.
nit: typo "connected to all others"?
gtsfm/utils/rotation.py
Outdated
| # Compute the Minimum Spanning Tree (MST) | ||
| mst = nx.minimum_spanning_tree(graph) | ||
|
|
||
| wRis = [Rot3() for _ in range(num_images)] |
There was a problem hiding this comment.
I think this code will only work if the poses are given in an ordered line
| wRi_list_euler_deg_est = [np.rad2deg(wRi.roll()) for wRi in wRi_list_computed] | ||
| assert np.allclose(wRi_list_euler_deg_est, wRi_list_euler_deg_expected) | ||
|
|
||
| def test_greedily_construct_st_mixed_order_chain(self) -> None: |
There was a problem hiding this comment.
@ayushbaid I added a test that fails with the current implementation
There was a problem hiding this comment.
Do we need to change the lookup logic? Or do you have a fix in mind?
There was a problem hiding this comment.
I added a fix -- traversing the shortest path in the MST between the origin and destination, and chaining the poses together
gtsfm/utils/rotation.py
Outdated
|
|
||
|
|
||
|
|
||
| def initialize_mst( |
There was a problem hiding this comment.
@ayushbaid maybe we should consider Travis' implementation here?
| if i1 < i2: | ||
| i1Ri2 = i2Ri1_dict[(i1, i2)].inverse() | ||
| else: | ||
| i1Ri2 = i2Ri1_dict[(i2, i1)] |
There was a problem hiding this comment.
This is not guaranteed, right?
There was a problem hiding this comment.
High level comments:
-
I think a better API to is pass "edge_weights" to the averaging modules, not the correspondences. The edge weights can be computed in the verifier for instance. Not a blocker for this PR though.
-
I think this initialization should be made optional, with a flag to change it. This would be a blocker for me :)
gtsfm/utils/rotation.py
Outdated
| return wRi_list | ||
|
|
||
|
|
||
| # def initialize_mst( |
There was a problem hiding this comment.
is this not used? if not, please remove.
| """ | ||
| i1Ti2_priors: Dict[Tuple[int, int], PosePrior] = {} | ||
| wRi_computed = self.obj.run_rotation_averaging(len(wRi_expected), i2Ri1_input, i1Ti2_priors) | ||
| two_view_estimation_reports = { |
There was a problem hiding this comment.
could you add another test, keeping this one unchanged, (for default behavior that does not need two view reports)?
| self.assertTrue( | ||
| geometry_comparisons.compare_rotations(wRi_computed, expected_wRi_list, ROTATION_ANGLE_ERROR_THRESHOLD_DEG) | ||
| ) | ||
| # def test_simple_with_prior(self): |
There was a problem hiding this comment.
why is this being removed?
ayushbaid
force-pushed
the
feature/shonan_mst_init
branch
from
3a1cfd5
to
429177a
Compare
| @@ -55,6 +56,7 @@ def compare_rotations( | |||
| relative_rotations_angles = np.array( | |||
| [compute_relative_rotation_angle(aRi, aRi_) for (aRi, aRi_) in zip(aRi_list, aRi_list_)], dtype=np.float32 | |||
| ) | |||
| print(relative_rotations_angles) | |||
There was a problem hiding this comment.
Needs to be removed
No description provided.