gtsfm icon indicating copy to clipboard operation
gtsfm copied to clipboard

Expanding gtsfm data

Open travisdriver opened this issue 4 years ago • 9 comments

Disclaimer: currently only working on AstroNet data

The docstring for the GtsfmData class states that its purpose is "describing the complete 3D scene." Therefore, I thought it made sense to extend the class' functionality slightly to support describing the ground truth scene as well. That way we can just track a single GtsfmData object as opposed to tracking the various components of the ground truth scene: gt_cameras_graph, gt_poses_graph, gt_scene_mesh_graph, etc.

This is definitely not in its final form, but I wanted to get some feedback before I sunk too much time into refactoring things.

travisdriver avatar Nov 05 '21 19:11 travisdriver

Have not looked at code, but just from your description, the ground truth scene and the computed scene are actually not the same reconstruction, so would it be better if we used a different GtsfmData instance for ground truth?

Gtsfm is currently in development and GT and metrics are everywhere. But once we get to a stable state, GtsfmData is our result which I think should be free from any GT members.

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

We could maybe put all GT members in a different GtsfmData or maybe have another container GTSfMGTData, curious what @ayushbaid thinks

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

We could maybe put all GT members in a different GtsfmData or maybe have another container GTSfMGtData, curious what @ayushbaid thinks

Agree with Akshay. It doesn't make sense to have additional information on this datastructure. We can have two if needed. The API design and the dataclasses have to be clean, and just have the arguments/attributes which are obvious.

GtsfmData is our output, which describes a 3d scene completely. It should not have attributes which are not needed to visualize a 3d scene.

ayushbaid avatar Nov 06 '21 16:11 ayushbaid

For any evaluation we need for the astronet dataset or any other dataset, we can load the gtsfmdata from disk, construct the loader, and compare them.

ayushbaid avatar Nov 06 '21 16:11 ayushbaid

I like the idea of having 1 class that holds a complete result.

  • Estimated result can be an instance of this.
  • GT result can be a separate instance of this.

If MVS produces a mesh, that I think we could safely assume the mesh could also be an attribute of a "complete result".

johnwlambert avatar Nov 06 '21 17:11 johnwlambert

Thank you guys for the feedback.

@akshay-krishnan Sorry, I think my description was a little misleading. The main change I made was having the loader create an instance of GtsfmData that contained all of the ground truth data, and replaced the gt_cameras_graph, gt_poses_graph, and gt_scene_mesh_graph with this instance in the SceneOptimizer (which still generates a different GtsfmData instance with our estimated reconstruction).

The only thing I added to the GtsfmData class was a scene_mesh attribute. While its only currently used in the GT for AstroNet, eventually GTSfM's MVS pipeline will populate it with our estimated mesh.

travisdriver avatar Nov 06 '21 20:11 travisdriver

Thank you guys for the feedback.

@akshay-krishnan Sorry, I think my description was a little misleading. The main change I made was having the loader create an instance of GtsfmData that contained all of the ground truth data, and replaced the gt_cameras_graph, gt_poses_graph, and gt_scene_mesh_graph with this instance in the SceneOptimizer (which still generates a different GtsfmData instance with our estimated reconstruction).

The only thing I added to the GtsfmData class was a scene_mesh attribute. While its only currently used in the GT for AstroNet, eventually GTSfM's MVS pipeline will populate it with our estimated mesh.

Sounds good. Does @codyly also plan to use the same trimesh format for dense reconstruction?

ayushbaid avatar Nov 06 '21 22:11 ayushbaid

Thanks for the PR, Travis. Agreed that this needed some consolidation/ clean up.

johnwlambert avatar Nov 29 '21 14:11 johnwlambert

I'm seeing a few flake8 failures:

Run flake8 --max-line-length 120 --ignore E201,E202,E203,E231,W291,W293,E303,W391,E402,W503,E731 gtsfm
gtsfm/runner/run_scene_optimizer_astronet.py:6:1: F401 'time' imported but unused
gtsfm/runner/run_scene_optimizer_astronet.py:8:1: F401 'dask.distributed.Client' imported but unused
gtsfm/runner/run_scene_optimizer_astronet.py:8:1: F401 'dask.distributed.LocalCluster' imported but unused
gtsfm/runner/run_scene_optimizer_astronet.py:8:1: F401 'dask.distributed.performance_report' imported but unused
gtsfm/runner/run_scene_optimizer_astronet.py:11:1: F401 'gtsfm.common.gtsfm_data.GtsfmData' imported but unused
gtsfm/common/gtsfm_data.py:12:1: F401 'dask.distributed.Client' imported but unused
Error: Process completed with exit code 1.

johnwlambert avatar Nov 29 '21 14:11 johnwlambert