Add report from matcher with correct inlier ratio computation #374
Conversation
|
I'm not sure I really agree with any of these changes. I think spreading the evaluation tasks around to all the different modules just increases complexity. I think more responsibility should be given to the Moreover, we are now passing around three different reports (two of which are just copies of each other) that could easily be storing the same information. |
| @@ -122,6 +122,7 @@ def create_computation_graph( | |||
| descriptors_graph_list += [delayed_descs] | |||
|
|
|||
| # Estimate two-view geometry and get indices of verified correspondences. | |||
| matcher_reports_dict = {} | |||
There was a problem hiding this comment.
Why are we creating and passing around an additional report that just holds the number of matches and inlier ratio, both of which are already stored in the TwoViewEstimationReport.
There was a problem hiding this comment.
The main idea behind this is the following direction from @dellaert :
(1) Each blue box in the graph should have its own table.
(2) We need to be totally functional in our programming. (We cannot modify objects)
(3) Each table can only be returned by one blue box.
(4) No two blue boxes can create a single gray box.
(5) The graph must determine the code/data types exactly.
There was a problem hiding this comment.
As we talked about offline, I think all of this can be achieved without creating a redundant data type for each blue box. If we continue on this trajectory, we will be passing around 7-8 different reports, which will be extremely cumbersome.
I think we should continue brainstorming.
|
How does this solve #306? |
306 notes that the "inlier ratio w.r.t. GT model" was far higher than "inlier ratio w.r.t. est model". We would actually expect the following two fractions:
to be very close to each other. It turns out the "inlier ratio w.r.t. GT model" that we were reporting in the Verifier's output was actually
The denominator was different, explaining the issue we saw in 306. This PR provides the computation of Fraction 2 and adds it to the report. |
There was a problem hiding this comment.
I like having a different section for each blue box and each box returning an instance of its metrics which don't have anything to do with other boxes.
But I don't think we should have blue boxes defining custom data types for this.
I think treating them all as metrics makes the system simpler and don't see any advantages to having additional types. Let me know if there are any.
@akshay-krishnan @ayushbaid @travisdriver wanted to ping you guys on this again |
|
Could you pull from master and update the PR? Or would you like one of us to "take it from here"? Everything in the PR looks good, except for the custom matcher report. This adds more boilerplate. We created the metrics struct to avoid such custom reports per module and to share operations across metrics. We could either return (num_inliers, inlier_ratio) as tuples (simpler), or return a GtsfmMetricsGroup and merge groups from different matchers later. |
This actually computes inlier ratio w.r.t. gt model with # putatives as the denominator.
Solves #306
Adds section:
