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

Possible Normalization Bug in Deformable Attention Coordinates #6

Closed
Ali2500 opened this issue Dec 4, 2020 · 1 comment · Fixed by #7
Closed

Possible Normalization Bug in Deformable Attention Coordinates #6

Ali2500 opened this issue Dec 4, 2020 · 1 comment · Fixed by #7

Comments

@Ali2500
Copy link

Ali2500 commented Dec 4, 2020

This pertains to be cross-attention between queries and encoder feature maps in deformable_transformer.py. The reference_points for the deformable attention operation appear to be interpreted as (x,y) coordinates since they are multiplied by src_valid_ratios in line 339 which has a (width, height) format (as evident from the get_valid_ratio method, line 125).

However, the src_spatial_shapes tensor contains dimensions in (height, width) format (see line 138 and 139). Then, in ms_deform_attn.py the reference points and offsets are combined as follows:

sampling_locations = reference_points[:, :, None, :, None, :] \
                       + sampling_offsets / input_spatial_shapes[None, None, None, :, None, :]

So it appears that the sampling_offsets / input_spatial_shapes[None, None, None, :, None, :] part assumes coordinates to be in (y,x) format whereas reference_points[:, :, None, :, None, :] appears to be in (x,y) format.

Of course I'm not sure if I followed the code correctly here (I could have missed something), but just wanted to bring this to the authors' attention in case they did not know already.

@jackroos
Copy link
Member

jackroos commented Dec 7, 2020

@Ali2500 Thank you for your kind reminder. This is indeed a mistake made in this released version, but not in the version used in our paper. I will update the code and pre-trained models soon. Thanks again. I will close this issue after the code is updated.

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 a pull request may close this issue.

2 participants