-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ETH3D Benchmarks #773
Add ETH3D Benchmarks #773
Conversation
scripts/eth3d_benchmark.sh
Outdated
echo "Correspondence Generator: ${correspondence_generator_config_name}" | ||
echo "Num workers: ${num_workers}" | ||
|
||
images_dir=/home/tdriver6/dev/eth3d_datasets/${dataset}_dslr_undistorted/${dataset}/images |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for adding this. Could you add an inline comment in the line above that would show the original download URL?
@@ -290,6 +290,16 @@ def read_cameras_txt( | |||
float(k1), | |||
float(k2), | |||
) | |||
elif model == "PINHOLE": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be nice to include an inline comment here that
# The 'PINHOLE' model is defined as a 4-parameter camera model: fx, fy, cx, cy
# It is defined in https://github.com/colmap/colmap/blob/main/src/colmap/sensor/models.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gentle bump on this
@@ -50,6 +50,8 @@ def __generate_cache_key( | |||
self, keypoints_i1: Keypoints, keypoints_i2: Keypoints, putative_corr_idxs: np.ndarray | |||
) -> str: | |||
"""Generates a cache key according to keypoint coordinates and putative correspondence indices.""" | |||
if putative_corr_idxs.size == 0: # catch no correspondences | |||
return cache_utils.generate_hash_for_numpy_array(np.array([])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we add a brief comment explaining why our existing logic won't handle this case? Could we add a test also?
cameras, images, points3d, load_sfmtracks=False | ||
) | ||
cameras, images, points3d = colmap_io.read_model(path=data_dir, ext=file_format) | ||
img_fnames, wTi_list, calibrations, _, point_cloud, rgb, img_dims = colmap2gtsfm( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we put this change in a separate PR?
I don't think this new function is equivalent to the old one, since the other one provides images in a lexicographically sorted order: https://github.com/borglab/gtsfm/blob/master/gtsfm/utils/io.py#L378
the .bin format does not do that, and it will create issues for any retrieval
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
colmap2gtsfm
now lexigraphically sorts by the filenames as before
CI is failing, its fixed at HEAD |
echo "Correspondence Generator: ${correspondence_generator_config_name}" | ||
echo "Num workers: ${num_workers}" | ||
|
||
images_dir=/home/tdriver6/Downloads/eth3d_datasets/${dataset}_dslr_undistorted/${dataset}/images |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you move these hard-coded paths to the top, and could we use an uppercase variable name?
No description provided.