DSF Track Estimation C++

[stepanyanhayk]

This PR integrates gtsam's DSF track estimation into gtsfm. The main purpose of this change is to bring runtime speedup by using C++ instead of Python.

[akshay-krishnan]

A few questions:

• What is the speedup youre getting?
• Can we use the gtsam data structures in other places in gtsfm, so that we can reduce the spike in memory usage by this module, and the time to copy from one to another? I think this should be possible for at least some of them.

[akshay-krishnan]

Could you verify the results in the CI? Make sure everything works without any accuracy loss?

[johnwlambert]

Thanks for adding the unit test, @stepanyanhayk!

I believe CI is failing:

tests/data_association/test_dsf_tracks_estimator.py:113:1: E302 expected 2 blank lines, found 1

Do you mind taking a look, and reformatting?

[johnwlambert]

Test and code looks great! Re @akshay-krishnan 's questions -- Hayk, could you attach a dashboard to this PR, showing the performance difference vs. master? Re benchmarking, you would see the greatest speedup on loftr, because it generates orders of magnitudes more tracks.

[johnwlambert]

By the way, the dashboard generation script is here: https://github.com/borglab/gtsfm/blob/master/gtsfm/evaluation/visualize_benchmark_comparison.py. It requires downloading the artifacts from a previous run on master (say the most recently landed PR), and the artifacts from this PR, and putting them in separate directories, and unzipping them. A report will be created, which looks like #594 (comment)

[stepanyanhayk]

Thanks a lot for the pointer, John. I will create the report today

[akshay-krishnan]

@stepanyanhayk friendy ping on this

[stepanyanhayk]

Sorry for the delay, @akshay-krishnan. These are the results I get from comparing the artifacts

[akshay-krishnan]

would mainly look at data association metrics.
# tracks computed with GT cameras EXCEEDS_REPOJ_ERROR, computed cameras: EXCEEDS_REPROJ_ERROR is regressing significantly, which means that we have more bad 2D tracks.
Shouldnt this PR be a no-op in terms of quality? @johnwlambert ? Are there any logical changes?

[akshay-krishnan]

Nitpicky comment on comments:
For example: "converting gtsfm Keypoints into gtsam Keypoints"

1. Punctuation -> "Converting gtsfm Keypoints into gtsam Keypoints."
2. Simple tense (like an instruction) -> "Convert gtsfm Keypoints into gtsam Keypoints."

Please refer to other comments in the file. I am personally flexible about these, but since its an open source project, I see some PRs fixing unrelated comments from other PRs. Better to write good comments to begin with :)

[johnwlambert]

Interesting, thanks for the dashboard @stepanyanhayk! Yes, this change should be a no-op. To understand better if we're seeing stochasticity from gtsfm itself, vs. from the change, we could make the dashboard one more time, with a second run from the CI. I'll retrigger a new run also. Would you mind checking the visual quality on any of the datasets?

Which master branch PR were you comparing with? (could you share a link?)

[stepanyanhayk]

Interesting, thanks for the dashboard @stepanyanhayk! Yes, this change should be a no-op. To understand better if we're seeing stochasticity from gtsfm itself, vs. from the change, we could make the dashboard one more time, with a second run from the CI. I'll retrigger a new run also. Would you mind checking the visual quality on any of the datasets?

Which master branch PR were you comparing with? (could you share a link?)

I was comparing with this master PR.

[stepanyanhayk]

Hi @johnwlambert, @akshay-krishnan

I have created a new dashboard with the recent run but it did not change the results, we still see the regression on the Data Association Metrics.

In terms of visual quality, I do not see much difference between the master and cpp changes.

Master:

Branch:

To debug further I have created a small notebook comparing the results from DsfTracksEstimator.run() function calls on both master and cpp versions: only 72% of the resulting tracks are the same.

So, the CPP implementation and Python implementation do not output the same 2D tracks. What are your thoughts on this? Do you think the issue might be from the CPP side?

[akshay-krishnan]

@johnwlambert please update the dashboard once we can use your C++ fixes

[johnwlambert]

BA Metrics: