Skip to content

Conversation

alexander-soare
Copy link
Contributor

@alexander-soare alexander-soare commented Oct 1, 2021

Very minor thing and possibly just my opinion but I found the docs for spatial scale in the roi_pool variants misleading. Conceptually we are mapping the box co-ordinates to the input co-ordinates, not the other way around. So if scale_factor = 0.5 we might consider that we are doing the mapping box -> box * 0.5. I only figured this out because I felt suspicious enough to open up a console and try it with a toy feature map. For argument's sake, you could write it the other way around, but then you'd have to invert the scale factor for it to be semantically correct.

Also added an example to make it super clear.

Happy to get other opinions. If agreed, also happy to have this snuck in with a diff PR if convenient.

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks @alexander-soare , I agree that this direction feels more natural, and the example is of great help here. Just for ref, in the code we indeed map from box coordinate to input coordinate:

roi_start_w = offset_rois[1] * spatial_scale - offset;

offset_rois is in the "box coord space" and roi_start_w is in "input feature map coord space"

@NicolasHug NicolasHug merged commit 6d9ab57 into pytorch:main Oct 1, 2021
facebook-github-bot pushed a commit that referenced this pull request Oct 7, 2021
Reviewed By: kazhang

Differential Revision: D31337425

fbshipit-source-id: 56ad91d85b217817aa4a5cc149513c33883867b1
mszhanyi pushed a commit to mszhanyi/vision that referenced this pull request Oct 19, 2021
cyyever pushed a commit to cyyever/vision that referenced this pull request Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants