-
Notifications
You must be signed in to change notification settings - Fork 7.1k
Updates to RoI transforms docs #3645
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
Conversation
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 the PR!
I think some parts might be a bit misleading now, could you have a look?
torchvision/ops/ps_roi_align.py
Outdated
@@ -19,23 +19,26 @@ def ps_roi_align( | |||
mentioned in Light-Head R-CNN. | |||
|
|||
Args: | |||
input (Tensor[N, C, H, W]): input tensor | |||
input (Tensor[N, C, H, W]): The input tensor, i.e. a batch with ``N`` feature maps |
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.
This is actually not correct -- N is the batch size, C is the number of feature maps (aka channels)
torchvision/ops/ps_roi_align.py
Outdated
output_size (int or Tuple[int, int]): the size of the output after the cropping | ||
is performed, as (height, width) | ||
If a single Tensor is passed, then the first column should | ||
contain the index of the corresponding feature map in the batch, i.e. a number in ``[0, N - 1]``. |
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.
index of the corresponding element in the batch. I think calling it feature map might be more more confusing
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.
ah thanks, I was assuming a feature map was C x H x W
for some reason
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!
Summary: * Edited roi transforms docs * remove incorrect param desc * Fixed confusion about feature maps according to comment Reviewed By: NicolasHug Differential Revision: D28169142 fbshipit-source-id: ee6abdf6d80b7ba53104aece3abc058ceb35c469
This PR adds some minor edits to the docstrings of the RoI transforms, in particular about the feature map indices in the RoIs tensor which were previously a source of confusion to me