Port dilated_max_pool2d() to ATen by skrah · Pull Request #20691 · pytorch/pytorch

skrah · 2019-05-19T22:48:43Z

No description provided.

…st.MNIST.

…ted_max_pool2d

ezyang · 2019-05-21T18:19:24Z

aten/src/ATen/native/DilatedMaxPool.h

+  check_dim_size(gradOutput, ndim, ndim-2, outputHeight);
+  check_dim_size(gradOutput, ndim, ndim-1, outputWidth);
+
+  if (cuda) {


I... would believe you if you told me this is what the code did before, but this special case is kind of shocking XD

It is shocking, but it's indeed what the old code did. :) The CUDA code here always uses 4 dimensions in the kernel for output, then resizes back after the kernel:

if(input->dim() == 3) THCTensor_(resize3d)(state, output, nInputPlane, nOutputRows, nOutputCols);

The C code uses 3 or 4 dimensions for the kernel.

Perhaps the shape check should be after all resizes and just before the kernel, but the old code checked right at the top of the function.

Yeah, I think that's the correct fix. No need to block this PR on fixing it, would be a nice readability improvement though.

ezyang · 2019-05-21T18:20:06Z

I've deduplicated the shape checks in the original code, which in turn led to renaming the Row/Col convention in the CUDA code to Height/Width for compatibility with the C++ code.

That is much appreciated, thank you!

ezyang · 2019-05-21T18:24:25Z

aten/src/ATen/native/DilatedMaxPool.h

+              "padW = ", padW, ", padH = ", padH, ", kW = ", kW, ", kH = ", kH);
+
+  if (outputWidth < 1 || outputHeight < 1) {
+    AT_ERROR("Given input size: (",


nit: TORCH_CHECK(outputWidth >= 1 && outputHeight >= 1, msg)

ezyang · 2019-05-21T18:25:28Z

aten/src/ATen/native/DilatedMaxPool2d.cpp

+          IntArrayRef dilation,
+          bool ceil_mode)
+{
+  // XXX JIT: Pooling.cpp allows stride.empty().


This sounds like another misannotated function.

The annotation Optional[BroadcastingList2[int]] looks correct, but there's some explicit handling of stride here:

pytorch/torch/nn/functional.py

Line 482 in 70ecddf

if stride is None:

I wonder if that could just be:

if stride is None: stride = kernel_size

The drawback is that one could crash cpp code by modifying Python code, but that is also the case for the annotation.

(Edit) "crash" meaning triggering an exception here, unless it will be changed to a real assert() in the future.

The other special case is here:

pytorch/test/cpp/api/integration.cpp

Line 303 in 70ecddf

x = torch::max_pool2d(conv1->forward(x), {2, 2}).relu();

It ends up calling max_pool2d_with_indices with all default parameters.

I didn't look very hard, but I couldn't find the overload of torch::max_pool2d(conv1->forward(x), {2, 2}) that works with just two parameters in the cpp files.

The drawback is that one could crash cpp code by modifying Python code, but that is also the case for the annotation.

I think, for now, we should consider these annotations as part of the TCB. I do wonder a little if they can't be checked for consistency with the main annotations, but... well... someone would have to figure that out :>

I wonder if that could just be:

Then we'd have to explain why it was permissible for all the other sites to also default things to empty list. Maybe they're all wrong.

@ailzhang, you helped us resolve the annotation last time in #20306. Do you know what's going on with these Optional[BroadcastingList2] things?

Hi @skrah @ezyang , yea this annotation looks fine, but I guess JIT should have instantiate it to int[2] before passing into aten. I'm looking into it, to double check the repro process:

checking out this PR

Run IntegrationTest.MNIST should trigger stride.empty() here right?
Thanks!

Hi @ailzhang, I think the stride.empty() case is also triggered by the regular Python tests if that is more convenient.

IntegrationTest.MNIST triggers all of stride.empty() && padding.size()==1 && dilation.size()==1.

And (for the sake of completeness) this PR already contains a workaround, so assert() will need to be added to reproduce it.

@ezyang I have a commit to fix this(and other similar ones). Once this PR is merged into master, I will rebase and send it out. :D

ezyang · 2019-05-21T18:26:07Z

aten/src/ATen/native/DilatedMaxPool.h

+
+template <typename dest_t, typename src_t>
+static inline dest_t
+safe_downcast(src_t v)


facebook-github-bot

@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ezyang · 2019-05-22T19:36:19Z

You forgot to delete aten/src/THCUNN/SpatialDilatedMaxPooling.cu and it's thus failing internal contbuilds. You didn't allow maintainers to edit your branch so I can't push the fix. Can you please fix this? :)

ezyang

Delete file

skrah · 2019-05-22T20:00:18Z

You didn't allow maintainers to edit your branch so I can't push the fix. Can you please fix this? :)

Done, I'll leave the "allow" button on next time, it's just a habit of mine from other projects. :)

facebook-github-bot

@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

skrah · 2019-05-23T15:12:27Z

@pytorchbot retest this please.

Summary: Pull Request resolved: pytorch/pytorch#20691 Differential Revision: D15435960 Pulled By: ezyang fbshipit-source-id: 548b7cc42e52ad2c641ec7d9cf78028d9411d02e

facebook-github-bot · 2019-05-24T02:15:10Z

@ezyang merged this pull request in ec57d1f.

skrah added 2 commits May 20, 2019 00:15

Port dilated_max_pool2d() to ATen.

774abb0

Remove SpatialMaxPooling.cu, which appears to be unused.

890c835

skrah closed this May 20, 2019

skrah reopened this May 20, 2019

Add workaround for non-expanded padding and dilation in IntegrationTe…

1bdf43b

…st.MNIST.

pytorchbot added module: build Build system issues module: cpu CPU specific problem (e.g., perf, algorithm) module: cuda Related to torch.cuda, and CUDA support in general module: internals Related to internal abstractions in c10 and ATen module: operators labels May 20, 2019

skrah removed module: build Build system issues module: cpu CPU specific problem (e.g., perf, algorithm) module: cuda Related to torch.cuda, and CUDA support in general module: internals Related to internal abstractions in c10 and ATen module: operators labels May 20, 2019

Merge branch 'master' of https://github.com/pytorch/pytorch into dila…

f514c40

…ted_max_pool2d

pytorchbot added the module: build Build system issues label May 20, 2019

Merge branch 'master' of https://github.com/pytorch/pytorch into dila…

082f8db

…ted_max_pool2d

ezyang reviewed May 21, 2019

View reviewed changes

ezyang approved these changes May 21, 2019

View reviewed changes

facebook-github-bot reviewed May 21, 2019

View reviewed changes

ezyang requested changes May 22, 2019

View reviewed changes

Delete aten/src/THCUNN/SpatialDilatedMaxPooling.cu.

6c01d1d

pytorchbot added module: build Build system issues module: cpu CPU specific problem (e.g., perf, algorithm) module: cuda Related to torch.cuda, and CUDA support in general module: internals Related to internal abstractions in c10 and ATen module: operators labels May 22, 2019

skrah requested a review from ezyang May 22, 2019 20:04

facebook-github-bot reviewed May 22, 2019

View reviewed changes

ezyang approved these changes May 22, 2019

View reviewed changes

facebook-github-bot closed this in ec57d1f May 23, 2019

ailzhang mentioned this pull request May 23, 2019

Fix annotation for Optional[BroadcastingListx] #20866

Closed

facebook-github-bot added the merged label May 24, 2019

skrah mentioned this pull request May 29, 2019

Port SpatialDilatedMaxPooling and VolumetricDilatedMaxPooling to ATen #18067

Closed

ezyang added the open source label Jun 24, 2019

f0k mentioned this pull request Jun 28, 2019

Use integer floor division for pooling shape computation #22304

Closed

mruberry added the Merged label Oct 28, 2020

Conversation

skrah commented May 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezyang commented May 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skrah May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

ezyang commented May 22, 2019

Uh oh!

ezyang left a comment

Choose a reason for hiding this comment

Uh oh!

skrah commented May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

skrah commented May 23, 2019

Uh oh!

facebook-github-bot commented May 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

skrah May 22, 2019 •

edited

Loading

skrah commented May 22, 2019 •

edited

Loading