-
Notifications
You must be signed in to change notification settings - Fork 51
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
Merging changes from the hilti challenge #515
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. I flagged what I could. Would be good to have report before merging.
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.
This is going to be huge!
gtsfm/averaging/rotation/shonan.py
Outdated
) -> BetweenFactorPose3s: | ||
"""Create between factors from the priors on relative poses.""" | ||
between_factors = BetweenFactorPose3s() | ||
noise_model = gtsam.noiseModel.Isotropic.Sigma(POSE3_DOF, PRIOR_SIGMA) |
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.
why don't we use the covariance now? (i.e address your TODO?)
another nit: the POSE3_DOF is probably overkill. It's something that will never change.
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.
I think we did this somewhere, so you can reuse that code.
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.
I was getting convergence issues with Shonan if I used priors from Bayesian ICP. So I was hardcoding it. What should we do now as a feature introduction to master?
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.
Is this Shonan specific? Were you getting good results in BA or TA?
If all of them need hardcoding, we can do that in the loader. One of them needing it is weird, maybe the other hardcoded values can be changed?
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.
I think the main problem here is not the convergence issue, but the noise model. We cannot accept a full covariance matrix in the Shonan averaging. We need to address the TODO on the next line.
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.
This is a pending comment from an old review, please ignore if fixed:
I remember Frank suggested using an isotropic noise model with a Sigma which is an average of the diagonal elements. Not sure how that works.
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.
Addressed
REACT_METRICS_PATH = BASE_PATH / "rtf_vis_tool" / "src" / "result_metrics" | ||
REACT_RESULTS_PATH = BASE_PATH / "rtf_vis_tool" / "public" / "results" |
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.
can we remove this altogether?
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.
I think Frank was talking about some student who will pick up the JS work again. So lets keep it for the time being.
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, but isnt it the same data that we save in both results_path and react_metrics_path ? Do we need this duplication?
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.
I remember there was some need for this because the viz tool could not access the parent of the root folder.
gtsfm/scene_optimizer.py
Outdated
|
||
def save_gtsfm_data(image_graph: List[Delayed], ba_input_graph: Delayed, ba_output_graph: Delayed) -> List[Delayed]: | ||
"""Saves the Gtsfm data before and after bundle adjustment. | ||
aligned_ba_input = ba_input.align_via_Sim3_to_poses(gt_poses) |
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.
is this duplicated when we compute metrics somewhere?
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.
Removed it. Thanks
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.
Its aligned in a different function. We do align it in the BundleAdjustmentOptimizer::evaluate
, but that aligned GtsfmData
objects are not returned.
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.
was this the reason why we were aligning ba_input in the other TODO comment? Its pretty expensive so we should only do it once I think
Is there any way to break this PR up into smaller ones? |
Yes, obviously there is, but we discussed and decided to do it this way. |
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.
nice work! we're getting closer, I'm not going to add new comments, some of my old ones are not yet resolved.
gtsfm/averaging/rotation/shonan.py
Outdated
) -> BetweenFactorPose3s: | ||
"""Create between factors from the priors on relative poses.""" | ||
between_factors = BetweenFactorPose3s() | ||
noise_model = gtsam.noiseModel.Isotropic.Sigma(POSE3_DOF, PRIOR_SIGMA) |
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.
Is this Shonan specific? Were you getting good results in BA or TA?
If all of them need hardcoding, we can do that in the loader. One of them needing it is weird, maybe the other hardcoded values can be changed?
gtsfm/evaluation/eval_hilti.py
Outdated
|
||
# Modified for using both fastlio and gtsfm TUM files | ||
# If you already have matplotlib, scipy and numpy, this will need an additional | ||
# pip install evo==1.13.5 |
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.
I am not sure. I originally put in it in the evaluation directory above gtsfm/ directory, which is currently empty, because it is not part of gtsfm.
Or we could also move it to 3rdparty.
gtsfm/multi_view_optimizer.py
Outdated
# TODO(Frank): Why would we do this here? But maybe we should simply fix align_via_Sim3_to_poses | ||
# ba_input_graph = dask.delayed(ba_input_graph.align_via_Sim3_to_poses)(gt_wTi_list) |
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.
could you check who consumes ba_input? and whether they expect it to be aligned to GT? is the output aligned to GT?
REACT_METRICS_PATH = BASE_PATH / "rtf_vis_tool" / "src" / "result_metrics" | ||
REACT_RESULTS_PATH = BASE_PATH / "rtf_vis_tool" / "public" / "results" |
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, but isnt it the same data that we save in both results_path and react_metrics_path ? Do we need this duplication?
gtsfm/scene_optimizer.py
Outdated
|
||
def save_gtsfm_data(image_graph: List[Delayed], ba_input_graph: Delayed, ba_output_graph: Delayed) -> List[Delayed]: | ||
"""Saves the Gtsfm data before and after bundle adjustment. | ||
aligned_ba_input = ba_input.align_via_Sim3_to_poses(gt_poses) |
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.
was this the reason why we were aligning ba_input in the other TODO comment? Its pretty expensive so we should only do it once I think
@ayushbaid we can do this in another PR if we want to submit this one soon: we can remove the "augmentation" logic in TA after we have a release with borglab/gtsam#1224. |
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.
Looks like all comments have been resolved. Just need to fix the CI now.
gtsfm/averaging/rotation/shonan.py
Outdated
) -> BetweenFactorPose3s: | ||
"""Create between factors from the priors on relative poses.""" | ||
between_factors = BetweenFactorPose3s() | ||
noise_model = gtsam.noiseModel.Isotropic.Sigma(POSE3_DOF, PRIOR_SIGMA) |
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.
This is a pending comment from an old review, please ignore if fixed:
I remember Frank suggested using an isotropic noise model with a Sigma which is an average of the diagonal elements. Not sure how that works.
gtsfm/evaluation/eval_hilti.py
Outdated
|
||
# Modified for using both fastlio and gtsfm TUM files | ||
# If you already have matplotlib, scipy and numpy, this will need an additional | ||
# pip install evo==1.13.5 |
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.
if we are not going to use this often, no need to depend on evo. Feel free to mark this resolved.
Still 2 CI fails? |
|
||
def read_fastlio_result(poses_txt_path: str) -> Tuple[List[Pose3], np.ndarray]: | ||
""" | ||
Read FastLIO result |
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.
@ayushbaid can we update this docstring to the style guide when you get a chance?
return Pose3(pose_rotation, pose_translation) | ||
|
||
|
||
def create_initial_estimate(poses: List[Pose3]): |
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.
Can we add the return type hint here?
EXP07_PATH = DATA_ROOT_PATH / "exp07" | ||
|
||
|
||
def pose_of_vector(pose_vector): |
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.
Can we add the return type hint here?
Dict[Tuple[int, int], Optional[Unit3]], | ||
Dict[Tuple[int, int], np.ndarray], | ||
Dict[int, Tuple[int, int]], | ||
]: |
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.
@ayushbaid do you mind adding the return type hint here?
use_cuda (optional): Use CUDA (GPU) for inference. Defaults to True. | ||
use_outdoor_model (optional): Use the outdoor weights for SuperGlue network. Defaults to True. | ||
init_model_in_constructor (optional): Initialize the model with weights in the constructor instead of the | ||
`match` function. This setting is recommended when we are not using |
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.
I think the formatting got messed up here @ayushbaid ?
No description provided.