gtsfm icon indicating copy to clipboard operation
gtsfm copied to clipboard

remove unnecessary TwoViewReport attributes

Open johnwlambert opened this issue 4 years ago • 5 comments

i2Ri1 and i2Ui1 shouldn't be in this report (I don't remember how they got in here, probably was from some old debugging i did while prototyping)...

johnwlambert avatar Nov 05 '21 01:11 johnwlambert

I agree that these are unnecessary. However, they are still set here and possibly other places.

Also, now two_view_report_pp is really just an exact copy of two_view_report and should be removed.

Thanks for finding the other reference, I had missed that one. It's removed now.

@dellaert and I pair-programmed the new output of the Postprocessor (InlierSupportProcessor) last week and added this. The main idea behind it is: (1) We have to be able to know the statistics of the raw matches, vs. the postprocessed matches. (2) Two different variables that represent different quantities cannot be named the same thing. (3) We have to follow the graph -- each blue box creates its own table. In this case, the Verifier has its own table in the report, and the InlierSupportProcessor also has its own table in the report.

Screen Shot 2021-11-05 at 8 56 37 AM

johnwlambert avatar Nov 05 '21 13:11 johnwlambert

Ok, but two_view_report and two_view_report_pp are now just exact copies of each other (with two_view_report_pp being None if there are not enough inliers).

I'm not sure that just copying a variable and changing its name solves any of the action items you presented in an effective way.

travisdriver avatar Nov 05 '21 13:11 travisdriver

I think we are lacking a good concept/meaning/purpose for the TwoViewEstimationReport. So far, it has included (almost?) everything from the two view estimator.

IF the intended purpose of the report is to store metrics, then all data members that have a bigger use case than a metric can be moved to a different data structure. By that I mean the verified correspondences, inlier ratio, rotation, translation etc. This PR removes the rotation and translation, which is good, but can we clean up the rest too?

akshay-krishnan avatar Nov 06 '21 21:11 akshay-krishnan

Recap of decisions: each blue box creates both metrics (pink box?) and a result (gray box). These are 2 separate data structures. At some point, each blue box could inherit from a class, and have a method def __produce_metrics(self):

johnwlambert avatar Dec 09 '21 16:12 johnwlambert

I agree that v_corr_idxs and reproj_error_gt_model should probably be removed.

I'm ambivalent about removing v_corr_idxs_inlier_mask_gt because it's used to visualize the metrics.

travisdriver avatar Jan 01 '22 04:01 travisdriver