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

[ONNX] Fix export of images with no detection #2215

Merged
merged 67 commits into from
May 20, 2020

Conversation

neginraoof
Copy link
Contributor

Fix export of images with no detection for Mask RCNN

@neginraoof
Copy link
Contributor Author

cc @fmassa for review. Thanks!

Copy link
Member

@fmassa fmassa left a 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, and sorry for the delay in reviewing.

I have a question, can you have a look?

Comment on lines 329 to 343
else:
w_half = (boxes[:, 2] - boxes[:, 0]).to(dtype=torch.float64) * .5
h_half = (boxes[:, 3] - boxes[:, 1]).to(dtype=torch.float64) * .5
x_c = (boxes[:, 2] + boxes[:, 0]).to(dtype=torch.float64) * .5
y_c = (boxes[:, 3] + boxes[:, 1]).to(dtype=torch.float64) * .5

w_half = w_half.to(dtype=torch.float64) * scale
h_half = h_half.to(dtype=torch.float64) * scale

boxes_exp0 = x_c - w_half
boxes_exp1 = y_c - h_half
boxes_exp2 = x_c + w_half
boxes_exp3 = y_c + h_half
boxes_exp = torch.stack((boxes_exp0, boxes_exp1, boxes_exp2, boxes_exp3), 1)
return boxes_exp.to(dtype=boxes.dtype)
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, which operation doesn't support empty tensors in ONNX?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I was seeing an issue when broadcasting And op:
"Can broadcast 0 by 0 or 1. 3 is invalid"
But after looks like this is not needed anymore. I've reverted this change.

boxes_exp = torch.stack((boxes_exp0, boxes_exp1, boxes_exp2, boxes_exp3), 1)
return boxes_exp
if boxes.numel() == 0:
return torch.empty(0, dtype=boxes.dtype)
Copy link
Member

Choose a reason for hiding this comment

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

This will return a box with a different number of dimensions in this case, as we normally return boxes with Nx4 dimensions. It might bring issues in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually looks like this branch is not needed. I have cleaned this up.

@@ -237,7 +237,7 @@ def __init__(self, in_channels, num_keypoints):
super(KeypointRCNNPredictor, self).__init__()
input_features = in_channels
deconv_kernel = 4
self.kps_score_lowres = misc_nn_ops.ConvTranspose2d(
self.kps_score_lowres = nn.ConvTranspose2d(
Copy link
Member

Choose a reason for hiding this comment

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

Great that we already support empty tensors in PyTorch! Thanks for the cleanup!

@fmassa
Copy link
Member

fmassa commented May 19, 2020

Apart from this question, I think the PR looks good to merge (minus the tensor having a different number of dimensions)

@neginraoof
Copy link
Contributor Author

@fmassa Thanks a lot for pointing out the changes in roi_heads.
If you agree, I can go ahead and remove class ConvTranspose2d() for misc.py since it's not used in the code anymore.

…of/fixDynamicResize

# Conflicts:
#	torchvision/models/detection/keypoint_rcnn.py
Comment on lines -23 to -60
class ConvTranspose2d(torch.nn.ConvTranspose2d):
"""
Equivalent to nn.ConvTranspose2d, but with support for empty batch sizes.
This will eventually be supported natively by PyTorch, and this
class can go away.
"""
def forward(self, x):
if x.numel() > 0:
return self.super_forward(x)
# get output shape

output_shape = [
(i - 1) * d - 2 * p + (di * (k - 1) + 1) + op
for i, p, di, k, d, op in zip(
x.shape[-2:],
list(self.padding),
list(self.dilation),
list(self.kernel_size),
list(self.stride),
list(self.output_padding),
)
]
output_shape = [x.shape[0], self.out_channels] + output_shape
return _new_empty_tensor(x, output_shape)

def super_forward(self, input, output_size=None):
# type: (Tensor, Optional[List[int]]) -> Tensor
if self.padding_mode != 'zeros':
raise ValueError('Only `zeros` padding mode is supported for ConvTranspose2d')

output_padding = self._output_padding(input, output_size, self.stride, self.padding, self.kernel_size)

return F.conv_transpose2d(
input, self.weight, self.bias, self.stride, self.padding,
output_padding, self.groups, self.dilation)


class BatchNorm2d(torch.nn.BatchNorm2d):
Copy link
Member

Choose a reason for hiding this comment

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

This is technically a BC-breaking change, as if users were relying on ConvTranspose2d or BatchNorm2d from misc.py this will error now. I'll merge this as is to move forward, but I'll send a follow-up PR adding back a backward-compatibility fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Thanks a lot @fmassa

Copy link
Member

@fmassa fmassa 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!

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