Skip to content
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

Disk Feature Extractor #49

Closed
wants to merge 8 commits into from
Closed

Disk Feature Extractor #49

wants to merge 8 commits into from

Conversation

lxxue
Copy link
Contributor

@lxxue lxxue commented Feb 5, 2021

Based on code in the original disk repo

Copy link
Member

@sarlinpe sarlinpe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this one too. Please see my comments :)

raise KeyError('Incompatible weight file!')
self.model = _DISK(window=8, desc_dim=conf['desc-dim'])
self.model.load_state_dict(weights)
self.model.to(torch.device('cuda' if torch.cuda.is_available() else 'cpu'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The device selection is already handled by extract_features

Comment on lines 16 to 23
# we rescale image only in the preprocessing step.
# we only pad the image size to be the multiple of 16 in this model
# 'height': None, # rescaled height (px). If unspecified, image is not resized in height dimension
# 'width': None, # rescaled width (px). If unspecified, image is not resized in width dimension
'window': 5, # NMS window size
'n': None, # Maximum number of features to extract. If unspecified, the number is not limited
'desc-dim': 128, # descriptor dimension. Needs to match the checkpoint value
'mode': 'nms', # Whether to extract features using the non-maxima suppresion mode or through training-time grid sampling technique
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to clean up the comments and make the key names consistent with the other extractors (see my comments on the R2D2 PR).

img = F.pad(img, (0, x_pad, 0, y_pad))
assert img.shape[1] == new_h and img.shape[2] == new_w, "Wrong Padding!"

with torch.no_grad():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parent function should already set no_grad

Comment on lines 70 to 74
msg = ('Please use input size which is multiple of 16 (or '
'adjust the --height and --width flags to let this '
'script rescale it automatically). This is because '
'we internally use a U-Net with 4 downsampling '
'steps, each by a factor of 2, therefore 2^4=16.')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the error message should be changed now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to remove this try-except block as we explicitly pad the image to be the multiple of 16.

kps_img_space = kps_crop_space # (2, N)
x = kps_crop_space[0, :]
y = kps_crop_space[1, :]
mask = (0 <= x) & (x < orig_w) & (0 <= y) & (y < orig_h)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be x <= (orig_w - 1) and similarly for y?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think this one is more accurate. Thanks!

Comment on lines 86 to 88
# we didn't rescale so no need for this
# kps_img_space, mask = image.to_image_coord(kps_crop_space)
kps_img_space = kps_crop_space # (2, N)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could maybe be cleaned up

@sarlinpe sarlinpe deleted the branch cvg:dev December 27, 2021 16:18
@sarlinpe sarlinpe closed this Dec 27, 2021
@sarlinpe sarlinpe mentioned this pull request Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants