Expanding gtsfm data #376

travisdriver · 2021-11-05T19:07:57Z

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.

akshay-krishnan · 2021-11-06T16:06:52Z

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 · 2021-11-06T16:08:07Z

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

ayushbaid · 2021-11-06T16:12:22Z

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 · 2021-11-06T16:13:31Z

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.

johnwlambert · 2021-11-06T17:35:54Z

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".

travisdriver · 2021-11-06T20:40:24Z

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.

ayushbaid · 2021-11-06T22:57:29Z

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?

…ing-gtsfm_data

akshay-krishnan

Some comments, please let us know when the PR is ready to review/submit

gtsfm/common/gtsfm_data.py

akshay-krishnan · 2021-11-13T01:28:59Z

gtsfm/loader/astronet_loader.py

@@ -78,7 +79,7 @@ def __init__(
        if not Path(data_dir).exists():
            raise FileNotFoundError("No data found at %s." % data_dir)
        cameras, images, points3d = colmap_io.read_model(path=data_dir, ext=".bin")
-        self._calibrations, self._wTi_list, img_fnames, self._sfmtracks = self.colmap2gtsfm(
+        self._calibrations, self._wTi_list, img_fnames, self._sfmtracks, cameras_gtsfm = self.colmap2gtsfm(


seeing wTi and cameras together is always a red flag ..

Can you elaborate?

cameras contain poses, camera.pose(), so wTi is not necessary

Same could be said for the calibrations.

true, so why are we returning everything here?

Sorry, I forgot to push a commit that removed cameras_gtsfm; will do that.

What I meant was that even in the SceneOptimizer we are passing in calibrations and cameras. However, this is hard to avoid since it is not possible to set either the Pose or Calibration to None in PinholeCameraCal3Bundler.

So, the two options are (1) pass around Calibration(s) and Pose(s) separately and construct cameras as needed, or (2) pass around Calibration(s) and Camera(s) which have redundant information.

removed cameras_gtsfm

gtsfm/loader/astronet_loader.py

akshay-krishnan · 2021-11-13T01:31:09Z

gtsfm/loader/astronet_loader.py

+    @property
+    def gt_gtsfm_data(self):
+        return self._gt_gtsfm_data
+
    @staticmethod
    def colmap2gtsfm(


why is this a static method in this class? like what does it have to do with astronet specifically?

AstroNet data is in COLMAP format. I could move this to the base class since it would also be useful for the Colmap data?

I dont feel like it is related to the loader, maybe it can be moved to a util file. Why would a user of a loader want to convert colmap data to gtsfm format? I would assume it just provides the gtsfm format, using whatever underneath.

Because the AstroNet data is in Colmap format and it needs to be converted to GTSfM format before it can be used by the pipeline.

I mean the loader is supposed to abstract that away. I think this PR #346 is moving it to a util? Which is better I think

The loader is responsible for reading in the specific data and converting it to a format usable by GTSfM.

I don't think it makes sense to have to add a util in some other file for every dataset used, as this will just congest the other modules. However, I think it's fine to move this specific function to a util, as we work with the Colmap format a lot.

When other users want to apply GTSfM to their specific data, all they should need to do is fill in a new loader (and maybe a runner) without touching any other code. It's infeasible to expect our main code base to handle converting every single dataset to the expected GTSfM format.

However, I think it's fine to move this specific function to a util, as we work with the Colmap format a lot.

yes, that is my reason as well.
I don't see why anyone would use the Astronet loader to convert colmap data to gtsfm. It already provides data in gtsfm format. And even if we did want to convert colmap to gtsfm format for some other reason, it does not need a AstroNet loader.

#346 is trying to move it to io_utils.py, which I think is better than a static method in AstroNet loader (my point that its being used in a non-loader specific context).

Maybe I should just leave it as is here to avoid conflicts with #346?

SInce youre changing the API and Jon isn't I think we should merge this is first.

@womackj1 Can #346 wait until this is merged? I don't see any updates on it recently.

gtsfm/loader/colmap_loader.py

akshay-krishnan · 2021-11-13T01:35:03Z

gtsfm/scene_optimizer.py

+        # Build cameras graph from GT GtsfmData.
+        # TODO (): remove later; this is just to conform to the required input of other functions.
+        gt_cameras_graph = (
+            [dask.delayed(gt_gtsfm_data.get_camera)(i) for i in gt_gtsfm_data._cameras.keys()]


Maybe I should ask @ayushbaid , but what is our "standard representation" for cameras? Is it dicts or lists? why are converting one to another here?

GtsfmData uses dicts to account for noncontiguous cameras ids, as some frames may be rejected during the verification process.

I convert the to lists here because the rest of the code expects it as a list, but I do agree that it's dangerous to have the same data as different data types.

Yeah, so maybe we should update the rest of the code

I spent a while trying to update the code so that we do not rely on converting the dictionary of cameras in GtsfmData to a List. However, almost all of the metric computation functions rely on Lists of Pose derived from the camera dictionary of GtsfmData.

I'm assuming this is why the _number_images attribute of the GtsfmData class was added in the first place: so that the cameras data could be converted to a list such that its the same length as the original ground truth data. I think this is should be saved for another PR.

I do think that using Dicts instead of Lists like Colmap is a lot better and should be implemented in the near future.

Similarly, I think _tracks should be a dictionary as well.

…ing-gtsfm_data

gtsfm/common/gtsfm_data.py

akshay-krishnan · 2021-11-26T20:08:47Z

gtsfm/loader/astronet_loader.py

@@ -78,7 +79,7 @@ def __init__(
        if not Path(data_dir).exists():
            raise FileNotFoundError("No data found at %s." % data_dir)
        cameras, images, points3d = colmap_io.read_model(path=data_dir, ext=".bin")
-        self._calibrations, self._wTi_list, img_fnames, self._sfmtracks = self.colmap2gtsfm(
+        self._calibrations, self._wTi_list, img_fnames, self._sfmtracks, cameras_gtsfm = self.colmap2gtsfm(


true, so why are we returning everything here?

akshay-krishnan · 2021-11-26T20:12:18Z

gtsfm/loader/astronet_loader.py

+    @property
+    def gt_gtsfm_data(self):
+        return self._gt_gtsfm_data
+
    @staticmethod
    def colmap2gtsfm(


However, I think it's fine to move this specific function to a util, as we work with the Colmap format a lot.

yes, that is my reason as well.
I don't see why anyone would use the Astronet loader to convert colmap data to gtsfm. It already provides data in gtsfm format. And even if we did want to convert colmap to gtsfm format for some other reason, it does not need a AstroNet loader.

gtsfm/runner/run_scene_optimizer_astronet.py

akshay-krishnan · 2021-11-26T20:15:03Z

gtsfm/scene_optimizer.py

+        # Build cameras graph from GT GtsfmData.
+        # TODO (): remove later; this is just to conform to the required input of other functions.
+        gt_cameras_graph = (
+            [dask.delayed(gt_gtsfm_data.get_camera)(i) for i in gt_gtsfm_data._cameras.keys()]


akshay-krishnan · 2021-11-26T20:21:40Z

gtsfm/loader/astronet_loader.py

+    @property
+    def gt_gtsfm_data(self):
+        return self._gt_gtsfm_data
+
    @staticmethod
    def colmap2gtsfm(


#346 is trying to move it to io_utils.py, which I think is better than a static method in AstroNet loader (my point that its being used in a non-loader specific context).

…ing-gtsfm_data

gtsfm/common/gtsfm_data.py

- removed `cameras_gtsfm` from `AstronetLoader` - removed commented code from `AstronetRunner` and added scattering of `scene_mesh` to `LoaderBase`

…ing-gtsfm_data

akshay-krishnan · 2021-11-29T08:19:41Z

gtsfm/loader/astronet_loader.py

+    @property
+    def gt_gtsfm_data(self):
+        return self._gt_gtsfm_data
+
    @staticmethod
    def colmap2gtsfm(


SInce youre changing the API and Jon isn't I think we should merge this is first.

@womackj1 Can #346 wait until this is merged? I don't see any updates on it recently.

johnwlambert · 2021-11-29T14:43:49Z

gtsfm/two_view_estimator.py

@@ -94,6 +92,13 @@ def create_computation_graph(
            Two view report w/ verifier metrics wrapped as Delayed.
            Two view report w/ post-processor metrics wrapped as Delayed.
        """
+        # Unpack GT data.
+        if gt_gtsfm_data is not None:
+            gt_wTi1_graph = gt_gtsfm_data.get_camera(0).pose()


instead of hard-coding camera 0 and 1, we should pull out i1 and i2 here

If so, I would have to pass in i1 and i2.

I'm good with passing in i1 and i2. I think whenever we use a camera index in GTSFM, we need to be able to assume it really means the index we expect

I don't see the utility in passing in two indices for a single object as opposed to just using a known mapping (i.e., i1 -> 0 and i2 -> 1), especially since none of the other objects use the indices.

I partially agree with John. I think it might be best to accept gt_i1Ti2 and the gt_mesh as two arguments.
Passing i1 and i2 I think makes the API too messy. But yes, as you said (also in another comment), assuming the input has only 2 cameras at 0 and 1 could lead to bugs.

johnwlambert · 2021-11-29T14:44:30Z

gtsfm/runner/gtsfm_runner_base.py

+
+        Ref: http://distributed.dask.org/en/stable/api.html#distributed.Client.scatter
+        """
+        # TODO(travisdriver): find a way to scatter mesh that doesn't require copying the GtsfmData object.


where do we do the copy?

Lines 99-103

Any reason we can’t use copy.deepcopy?

johnwlambert · 2021-11-29T14:47:54Z

gtsfm/common/gtsfm_data.py

+    def get_two_view_data(self, i1: int, i2: int) -> "GtsfmData":
+        """Collects GtsfmData for a single image pair."""
+        # TODO (travisdriver): also collect tracks if available.
+        cameras_pair = {0: self.get_camera(i1), 1: self.get_camera(i2)}


I think we should always use i1 and i2 directly, instead of remapping to 0 and 1, since this can create some confusion/bugs downstream for the user.

@ayushbaid @akshay-krishnan what are your thoughts?

johnwlambert · 2021-11-29T14:48:11Z

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

johnwlambert · 2021-11-29T14:48:33Z

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.

travisdriver added 2 commits November 5, 2021 13:29

Getting LM error

b019c88

Expanding functionality/use of GtsfmData

9faedcc

This was linked to issues Nov 5, 2021

Organize the passing of ground truth (poses, meshes etc.) in the scene optimizer and individual modules #361

Open

Interface between loaders and GtsfmData #263

Open

travisdriver requested review from akshay-krishnan, ayushbaid and johnwlambert November 5, 2021 19:10

travisdriver added the refactor label Nov 5, 2021

travisdriver and others added 5 commits November 10, 2021 10:33

Merge branch 'master' of https://github.com/borglab/gtsfm into expand…

7001958

…ing-gtsfm_data

Added as abstract property to

170cdb6

Should be working on COLMAP data

adb0fc1

Fixing unit tests

af7165c

Removing random text files

8cbbc53

akshay-krishnan requested changes Nov 13, 2021

View reviewed changes

Merge branch 'master' of https://github.com/borglab/gtsfm into expand…

6bf0b18

…ing-gtsfm_data

travisdriver mentioned this pull request Nov 17, 2021

Remove conversion of GtsfmData Dictionaries to Lists #383

Open

travisdriver added 4 commits November 16, 2021 23:56

Removed unused libraries

372557f

Merge branch 'master' of https://github.com/borglab/gtsfm into expand…

c5ccee7

…ing-gtsfm_data

All unit tests should be passing

439578a

Remove unused imports

75918e7

travisdriver changed the title ~~[WIP] Expanding gtsfm data~~ Expanding gtsfm data Nov 17, 2021

Addressing comments

8654f37

travisdriver requested a review from akshay-krishnan November 26, 2021 18:48

akshay-krishnan reviewed Nov 26, 2021

View reviewed changes

Merge branch 'master' of https://github.com/borglab/gtsfm into expand…

3e2d2ba

…ing-gtsfm_data

ayushbaid reviewed Nov 26, 2021

View reviewed changes

gtsfm/common/gtsfm_data.py Outdated Show resolved Hide resolved

travisdriver added 3 commits November 26, 2021 18:48

Addressing comments

5740547

- removed `cameras_gtsfm` from `AstronetLoader` - removed commented code from `AstronetRunner` and added scattering of `scene_mesh` to `LoaderBase`

Merge branch 'master' of https://github.com/borglab/gtsfm into expand…

fe7ca72

…ing-gtsfm_data

Fixed typo in docstring

d5f83fe

travisdriver requested review from akshay-krishnan and ayushbaid November 27, 2021 17:40

akshay-krishnan approved these changes Nov 29, 2021

View reviewed changes

johnwlambert reviewed Nov 29, 2021

View reviewed changes

Expanding gtsfm data #376

Are you sure you want to change the base?

Expanding gtsfm data #376

Conversation

travisdriver commented Nov 5, 2021

akshay-krishnan commented Nov 6, 2021

akshay-krishnan commented Nov 6, 2021

ayushbaid commented Nov 6, 2021

ayushbaid commented Nov 6, 2021

johnwlambert commented Nov 6, 2021 • edited Loading

travisdriver commented Nov 6, 2021

ayushbaid commented Nov 6, 2021

akshay-krishnan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisdriver Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnwlambert commented Nov 29, 2021

johnwlambert commented Nov 29, 2021

johnwlambert commented Nov 6, 2021 •

edited

Loading

travisdriver Nov 17, 2021 •

edited

Loading