Adding expressive GTSFM data to allow non-consecutive cameras #113
Conversation
|
Some changes in the PR are just fixing the unit tests failing due to #104. |
gtsfm/utils/io.py
Outdated
| """ | ||
|
|
||
| # TODO: get image shape somehow | ||
|
|
There was a problem hiding this comment.
i believe to match COLMAP, we need also to dump 4 lines:
f.write("# Image list with two lines of data per image:\n")
f.write("# IMAGE_ID, QW, QX, QY, QZ, TX, TY, TZ, CAMERA_ID, NAME\n")
f.write("# POINTS2D[] as (X, Y, POINT3D_ID)\n")
f.write(f"# Number of images: {num_imgs}, mean observations per image: {mean_obs_per_img}")There was a problem hiding this comment.
Can I make a different PR to match the format of COLMAP? I feel it will make it easier to land?
gtsfm/scene_optimizer.py
Outdated
| @@ -213,12 +211,20 @@ def create_computation_graph( | |||
| if self._save_bal_files: | |||
| # save the input to Bundle Adjustment (from data association) | |||
| auxiliary_graph_list.append( | |||
| dask.delayed(write_sfmdata_to_disk)(ba_input_graph, os.path.join(RESULTS_PATH, "ba_input.bal")) | |||
| dask.delayed(io_utils.write_cameras)(ba_input_graph, os.path.join(RESULTS_PATH, "ba_input_cameras.txt")) | |||
There was a problem hiding this comment.
i see why you named these differently, because we have multiple versions, and we want to differentiate between them.
But can we just put them in separate subdirectories? Per https://colmap.github.io/cli.html, we need to preserve the names to be compatible:
COLMAP as far as I know
gtsfm/scene_optimizer.py
Outdated
| dask.delayed(io_utils.write_cameras)(ba_input_graph, os.path.join(RESULTS_PATH, "ba_input_cameras.txt")) | ||
| ) | ||
| auxiliary_graph_list.append( | ||
| dask.delayed(io_utils.write_images)(ba_input_graph, os.path.join(RESULTS_PATH, "ba_input_images.txt")) |
There was a problem hiding this comment.
Can we save "points3D.txt" as well here?
| self.assertEqual(computed.get_camera(2), computed.get_camera(2)) | ||
| self.assertEqual(computed.number_tracks(), 0) | ||
|
|
||
| @mock.patch.object(graph_utils, "get_nodes_in_largest_connected_component", return_value=[1, 2, 4]) |
There was a problem hiding this comment.
do you mind adding an inline comment explaining why we need to use mock here?
There was a problem hiding this comment.
I'm not too familiar with the mocking lib in Python or when we need it -- could you explain a little more about why a typical function call won't work?
gtsfm/common/gtsfm_data.py
Outdated
| return GtsfmData.pick_cameras(self, cameras_in_largest_cc) | ||
|
|
||
| @classmethod | ||
| def pick_cameras(cls, gtsfm_data: "GtsfmData", camera_indices: List[int]) -> "GtsfmData": |
There was a problem hiding this comment.
looks great @ayushbaid . much better. only thing i might change is to name this
def from_picked_cameras()
I think class methods often start with from_blah to make their factory functionality clearer?
|
Thanks @ayushbaid ! |
The current SfmData structure stores the camera as a list, and hence needs all the image to have a valid camera. This is not the case when some upstream module drops cameras.
The new data structure uses a dictionary to store the cameras, and hence can store non-contiguous cameras.