Structured kernel definition for upsample_nearest2d#50189
Structured kernel definition for upsample_nearest2d#50189soulitzer wants to merge 6 commits intopytorch:masterfrom
Conversation
💊 CI failures summary and remediationsAs of commit 1d53205 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. |
| (input.numel() != 0 || | ||
| (input.size(1) != 0 && input.size(2) != 0 && input.size(3) != 0) | ||
| ) && | ||
| input.dim() == 4, |
There was a problem hiding this comment.
you've just checked that input.dim() == 4, this is always true, and if it weren't, there would be trouble in input.size(3)
There was a problem hiding this comment.
hmm, this might actually be a useful check because input.size(3) might've been short-circuited if input.numel() != 0. Either way if we use prod_intlist method, we will have to keep this check.
There was a problem hiding this comment.
upsample_nearest_2d_common_check checks that input_size.size() == 4, so you can get to this line only if input.dim == 4, no need to check for it again.
| // Allow for empty batch size but not other dimensions | ||
| TORCH_CHECK( | ||
| (input.numel() != 0 || | ||
| (input.size(1) != 0 && input.size(2) != 0 && input.size(3) != 0) |
There was a problem hiding this comment.
prod_intlist(input.sizes()+1, input.sizes().end())
| ) { | ||
| auto full_output_size = upsample_nearest2d_common_check(input_size, output_size); | ||
|
|
||
| check_dim_size(grad_output, 4, 0, full_output_size[0]); |
There was a problem hiding this comment.
check_dim_size is pretty inefficient, you can compare sizes directly
There was a problem hiding this comment.
whoops, gonna have to fix upsample nearest 1d too
…mple-structured-kernel
Codecov Report
@@ Coverage Diff @@
## master #50189 +/- ##
=======================================
Coverage 80.68% 80.68%
=======================================
Files 1902 1902
Lines 206356 206339 -17
=======================================
- Hits 166496 166491 -5
+ Misses 39860 39848 -12 |
f2f8ee5 to
17e7332
Compare
| nbatch, | ||
| channels, | ||
| int64_t input_width = input_size[2]; | ||
| int64_t input_height = input_size[3]; |
| int64_t output_height = output_size[0]; | ||
| int64_t output_width = output_size[1]; | ||
| int64_t output_width = output_size[0]; | ||
| int64_t output_height = output_size[1]; |
There was a problem hiding this comment.
Looks wrong and not caught by test - can you please add a test with the different input and output width and height?
There was a problem hiding this comment.
I think the code here is functionally correct; just the error message would be rendered wrong here.
| grad_input.zero_(); | ||
|
|
||
| upsample_nearest2d_backward_kernel(kCPU, grad_input, grad_output, scales_h, scales_w); | ||
| return {nbatch, channels, output_width, output_height}; |
|
Everything else looks fine. |
76fe1e3 to
1d53205
Compare
|
@ngimel I've made an update to added the tests, and the other issues from previous comments should be addressed as well. Would you mind taking another look? |
facebook-github-bot
left a comment
There was a problem hiding this comment.
@soulitzer has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@soulitzer merged this pull request in 19a8e68. |
|
This pull request has been reverted by ef6be0e. |
|
Unlanding this because it broke internal tests |
See the structured kernel definition RFC for context.