C++ MaxPool Module #24860

ShahriarRezghi · 2019-08-19T20:17:51Z

No description provided.

yf225

@ShahriarSS Thanks and it looks fantastic! I left some very minor comments. We might also want to wait until #23852 is merged, so that MaxPool can be the first module that achieves Python/C++ API parity =D

yf225 · 2019-08-20T18:44:47Z

torch/csrc/api/include/torch/nn/modules/maxpool.h

+  TORCH_ARG(ExpandingArray<D>, padding) = 0;
+
+  /// a parameter that controls the stride of elements in the window
+  TORCH_ARG(ExpandingArray<D>, dialation) = 1;


nit: dialation -> dilation

yf225 · 2019-08-20T18:48:13Z

torch/csrc/api/include/torch/nn/modules/maxpool.h

+
+/// Applies maxpool over a 1-D input.
+/// See https://pytorch.org/docs/master/nn.html#torch.nn.MaxPool1d to learn
+/// about the exact behavior of this module.


We might want to write the formulas here similar to https://pytorch.org/docs/stable/nn.html#maxpool1d (we can either do it in this PR or in a follow-up PR).

yf225 · 2019-08-20T18:48:38Z

test/cpp/api/modules.cpp

+  torch::Tensor s = y.sum();
+
+  s.backward();
+  std::cout << y.sizes() << std::endl;


nit: need to remove print statement

yf225 · 2019-08-20T18:53:22Z

test/cpp/api/modules.cpp

+  std::cout << y.sizes() << std::endl;
+  ASSERT_EQ(y.ndimension(), 3);
+  ASSERT_EQ(s.ndimension(), 0);
+  ASSERT_EQ(y.sizes(), torch::IntArrayRef({1, 1, 2}));


Besides checking ndimension() and sizes(), can we also check that the values of y are what we expected? (we might need to set value of x in a specific way in order to test this)

yf225 · 2019-08-20T18:53:57Z

test/cpp/api/modules.cpp

+  s.backward();
+  ASSERT_EQ(y.ndimension(), 3);
+  ASSERT_EQ(s.ndimension(), 0);
+  ASSERT_EQ(y.sizes(), torch::IntArrayRef({2, 2, 2}));


Ditto for checking value of y.

yf225 · 2019-08-20T18:54:30Z

test/cpp/api/modules.cpp

+  ASSERT_EQ(s.ndimension(), 0);
+  for (auto i = 0; i < 3; i++) {
+    ASSERT_EQ(y.size(i), 2);
+  }


Ditto for checking value of y.

yf225 · 2019-08-20T18:54:43Z

test/cpp/api/modules.cpp

+  ASSERT_EQ(s.ndimension(), 0);
+  for (auto i = 0; i < 4; i++) {
+    ASSERT_EQ(y.size(i), 2);
+  }


Ditto for checking value of y.

ShahriarRezghi · 2019-09-06T20:54:28Z

@yf225 There were some problems and one test wasn't implemented. I fixed them so please take a look.

yf225 · 2019-09-09T15:06:40Z

@pytorchbot rebase this please

pytorchbot · 2019-09-09T15:06:43Z

There's nothing to do! This branch is already up to date with master (4fac61a).

(To learn more about this bot, see Bot commands.)

yf225

Thanks a lot @ShahriarSS ! I will add the parity test in a follow-up PR.

facebook-github-bot

@yf225 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

caffe2/CMakeLists.txt

Summary: This PR makes the following improvements to C++ API parity test harness: 1. Remove `options_args` since we can get the list of options from the Python module constructor args. 2. Add test for mapping `int` or `tuple` in Python module constructor args to `ExpandingArray` in C++ module options. 3. Use regex to split up e.g. `(1, {2, 3}, 4)` into `['1', '{2, 3}', '4']` for `cpp_default_constructor_args`. 4. Add options arg accessor tests in `_test_torch_nn_module_ctor_args`. We will be able to merge #24160 and #24860 after these improvements. Pull Request resolved: #25828 Differential Revision: D17266197 Pulled By: yf225 fbshipit-source-id: 96d0d4a2fcc4b47cd1782d4df2c9bac107dec3f9

yf225 · 2019-09-10T15:24:40Z

@pytorchbot rebase this please

yf225 · 2019-09-10T15:41:29Z

torch/csrc/api/src/nn/modules/maxpool.cpp

+      options.stride_,
+      options.padding_,
+      options.dilation_,
+      options.ceil_mode_);


@ShahriarSS We might be missing options.return_indices_ here (and the forward calls for 2d and 3d), I will push a commit to add it.

Thanks. But I don't think that torch::max_poolxd uses it. That's why I didn't include it.

Yes @yf225 here is the documentation:

return_indices: if ``True``, will return the max indices along with the outputs. Useful for :class:`torch.nn.MaxUnpool2d` later

Ah got it, thanks a lot for the catch!

This reverts commit 2311cd5.

facebook-github-bot

@yf225 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@yf225 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@yf225 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2019-09-11T17:35:38Z

@yf225 merged this pull request in ba9fda1.

yf225 · 2020-03-31T16:36:07Z

cc. @glaringlee

ShahriarSS added 2 commits August 17, 2019 10:45

Wrote the code but tests fail

7ff5354

Fixed the problem

ad44c5c

ShahriarRezghi requested review from ebetica, goldsborough and yf225 as code owners August 19, 2019 20:17

pytorchbot added caffe2 module: build Build system issues module: cpp Related to C++ API labels Aug 19, 2019

yf225 reviewed Aug 20, 2019

View reviewed changes

ShahriarSS added 3 commits August 24, 2019 12:21

Made the changes

d5ac045

Fixed some stuff

d3fef78

Fixed some errors

18af69e

yf225 mentioned this pull request Sep 8, 2019

Make various improvements to C++ API parity test harness #25828

Closed

Merge branch 'master' into c++-maxpool

130f022

yf225 approved these changes Sep 9, 2019

View reviewed changes

facebook-github-bot reviewed Sep 9, 2019

View reviewed changes

yf225 reviewed Sep 9, 2019

View reviewed changes

caffe2/CMakeLists.txt Show resolved Hide resolved

yf225 mentioned this pull request Sep 9, 2019

Python/C++ API Parity: torch.nn modules and functional #25883

Open

pytorchbot and others added 3 commits September 10, 2019 15:24

Merge remote-tracking branch 'origin/master' into HEAD

882f9f6

fix internal build

189753f

add parity test

c4e9532

pytorchbot added the module: nn Related to torch.nn label Sep 10, 2019

yf225 reviewed Sep 10, 2019

View reviewed changes

Will Feng added 2 commits September 10, 2019 12:02

fix forward

2311cd5

Revert "fix forward"

a39e9a7

This reverts commit 2311cd5.

Will Feng added 2 commits September 10, 2019 12:40

fix parity test

7e307bf

fix parity test

2e4c75c

facebook-github-bot reviewed Sep 10, 2019

View reviewed changes

Merge branch 'master' into c++-maxpool

475e595

facebook-github-bot reviewed Sep 10, 2019

View reviewed changes

Will Feng added 2 commits September 10, 2019 21:38

Merge branch 'master' into c++-maxpool

aa0f624

Merge branch 'master' into c++-maxpool

1085aa9

facebook-github-bot reviewed Sep 11, 2019

View reviewed changes

facebook-github-bot closed this in ba9fda1 Sep 11, 2019

facebook-github-bot added the merged label Sep 11, 2019

mruberry added the Merged label Oct 28, 2020

C++ MaxPool Module #24860

C++ MaxPool Module #24860

Uh oh!

Conversation

ShahriarRezghi commented Aug 19, 2019

Uh oh!

yf225 left a comment

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!

ShahriarRezghi commented Sep 6, 2019

Uh oh!

yf225 commented Sep 9, 2019

Uh oh!

pytorchbot commented Sep 9, 2019

Uh oh!

yf225 left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yf225 commented Sep 10, 2019

Uh oh!

yf225 Sep 10, 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!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Sep 11, 2019

Uh oh!

yf225 commented Mar 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

yf225 Sep 10, 2019 •

edited

Loading