The test I added failed, so the test seems good. I don't see what's wrong with the implementation, though. When testing locally, it does not seem to call pooling_output_shape() at all, so it doesn't matter that I corrected it -- I even tried adding +10 and std::cout without seeing any effects (adding invalid statements gives me compilation errors, though, so at least it's getting compiled).

Quick one-liner to test the implementation:

python -c 'import torch; print(torch.nn.functional.max_pool1d(torch.zeros((1, 1, 4)), 3, stride=2, padding=0, dilation=2, ceil_mode=False).shape)'

This should raise a RuntimeError, but right now it prints (1, 1, 1). Any idea what's going on?

Okay, I grepped the source code for my (ceil_mode ? stride - 1 : 0) construct, and found that #20691 has copied it to another place: https://github.com/pytorch/pytorch/blob/31e1e63/aten/src/ATen/native/Pool.h#L24
This is annoying. I should have thought of this earlier and not waste my time on trying to get the test working correctly and tracing through the code.

Amended my commit to change all copies of the code. Fingers crossed.

Is there a way to get rid of the copies? Can THNN and THCUNN use the ATen version? Or is there another shared place for such things?

Status: My test passes, but there's another test that relied on the broken pooling behavior. Will look into that later (maybe not today); marked as WIP again.

Is there a way to get rid of the copies? Can THNN and THCUNN use the ATen version? Or is there another shared place for such things?

Yes please! There is no technical barrier here, it's purely a matter of code organization.

Can THNN and THCUNN use the ATen version?

Yes please! There is no technical barrier here, it's purely a matter of code organization.

The THNN version has been extended with support for asymmetric padding, though (in #21310), which neither the THCUNN nor the ATen version have. Shall this be ported over then? Otherwise THNN cannot use ATen. Is ATen supposed to replace THNN and THCUNN in the long run, or are they meant to co-exist?

ATen is meant to replace THNN/THCUNN; the way we are killing the old code is by porting it to ATen (most notable change in a port is removal of manual refcounting). We should not have accepted the previous PR if there was already a copy of the code in ATen. @akyrola do you have an opinion here? The correct fix at this point is to make sure asymmetric padding works for all versions of the code (and deduplicate code, if there are multiple copies). I have not actually looked into the situation.

@ezyang i am not really involved with pytorch development at this point, so don't have an opinion

The correct fix at this point is to make sure asymmetric padding works for all versions of the code (and deduplicate code, if there are multiple copies). I have not actually looked into the situation.

Ok. As far as I understand, asymmetric padding was added to support ceil_mode pooling with mkldnn, which doesn't support it natively. That's why it hadn't been added to the THCUNN version. So I think the correct fix will be to have https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/native/mkldnn/Utils.cpp use the ATen copy of the pooling size implementation, not the THNN copy of the implementation. The ATen copy will have to be adapted to match the THNN one. I can do that.

I will then try deleting the THNN and THCUNN versions to see if anything else still depends on them that should be using ATen instead. Does that make sense?

Finally, I will check the test that relied on the broken pooling behavior.

I will then try deleting the THNN and THCUNN versions to see if anything else still depends on them that should be using ATen instead. Does that make sense?

That'd be great! Thank you for your hard work!

The failing existing test had a python implementation of the pooling shape computation, but used float division instead of an integer division. The test only failed now that the pooling shape computation in torch was corrected. Fixed the python implementation.

There were two references to the THNN pooling shape function; I replaced them with the ATen version and removed the THNN and THCUNN versions completely.

I think this is good to go, I'll remove the WIP label.

The only failing test has nothing to do with me, as far as I see:

Jul 09 16:47:24 CMake Error at cmake/Modules/FindOpenMP.cmake:287 (cmake_parse_implicit_link_info):
Jul 09 16:47:24   CMAKE_PARSE_IMPLICIT_LINK_INFO Function invoked with incorrect arguments
Jul 09 16:47:24   for function named: CMAKE_PARSE_IMPLICIT_LINK_INFO
Jul 09 16:47:24 Call Stack (most recent call first):
Jul 09 16:47:24   cmake/Modules/FindOpenMP.cmake:481 (_OPENMP_GET_FLAGS)
Jul 09 16:47:24   cmake/Modules/FindMKL.cmake:172 (FIND_PACKAGE)
Jul 09 16:47:24   cmake/Modules/FindMKL.cmake:265 (CHECK_ALL_LIBRARIES)
Jul 09 16:47:24   cmake/Dependencies.cmake:131 (find_package)
Jul 09 16:47:24   CMakeLists.txt:270 (include)

@ezyang merged this pull request in 5adba33.