C++ Fold nn module #24160

ShahriarRezghi · 2019-08-11T16:49:13Z

No description provided.

ShahriarRezghi · 2019-08-11T20:36:28Z

Why does this PR have caffe2 label?

yf225

Thanks a lot for the PR @ShahriarSS! Overall it looks awesome. Once #23852 lands we can add the Fold module into the API parity whitelist, and then we can merge this PR.

yf225 · 2019-08-15T00:35:00Z

test/cpp/api/modules.cpp

+  ASSERT_EQ(y.size(2), 4);
+  ASSERT_EQ(y.size(3), 5);
+
+  // TODO check numel of grad


Do we plan to implement this part as well?

Yes. I just forgot about it.

yf225 · 2019-08-15T00:48:16Z

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

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


The documentation here looks great. We can experiment with porting all docs (including math formulas) from https://pytorch.org/docs/stable/nn.html#fold to here in this PR, or in a later PR if you prefer.

I'll add the formulas here. We also need to do this for the other modules already implemented.

yf225 · 2019-08-15T00:52:10Z

@ShahriarSS Regarding the caffe2 label: it's probably because we changed the CMakeLists.txt file within the caffe2/ folder. This change is not really Caffe2 related and we can ignore the label.

ShahriarRezghi · 2019-09-06T18:29:36Z

@yf225 I removed the TODO from fold test because fold doesn't have weights of any kind. I think nn::Fold is ready.

yf225 · 2019-09-06T18:44:49Z

@ShahriarSS Thanks! I am working on adding API parity test for nn::Fold (currently blocked by #25784), and I will merge the changes into this PR and then land it.

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

@pytorchbot rebase this please

facebook-github-bot

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

yf225 · 2019-09-09T18:23:11Z

caffe2/CMakeLists.txt

      ${TORCH_SRC_DIR}/csrc/api/src/nn/modules/conv.cpp
      ${TORCH_SRC_DIR}/csrc/api/src/nn/modules/dropout.cpp
      ${TORCH_SRC_DIR}/csrc/api/src/nn/modules/embedding.cpp
+      ${TORCH_SRC_DIR}/csrc/api/src/nn/modules/fold.cpp


@ShahriarSS the Facebook internal build seems to fail, and I am thinking we might need to add the fold.cpp entry into

pytorch/tools/build_variables.py

Lines 178 to 205 in 13292ec

torch_cpp_srcs = [

"torch/csrc/api/src/cuda.cpp", # this just forwards stuff, no real CUDA

"torch/csrc/api/src/data/datasets/mnist.cpp",

"torch/csrc/api/src/data/samplers/distributed.cpp",

"torch/csrc/api/src/data/samplers/random.cpp",

"torch/csrc/api/src/data/samplers/sequential.cpp",

"torch/csrc/api/src/data/samplers/stream.cpp",

"torch/csrc/api/src/jit.cpp",

"torch/csrc/api/src/nn/init.cpp",

"torch/csrc/api/src/nn/module.cpp",

"torch/csrc/api/src/nn/modules/batchnorm.cpp",

"torch/csrc/api/src/nn/modules/conv.cpp",

"torch/csrc/api/src/nn/modules/dropout.cpp",

"torch/csrc/api/src/nn/modules/embedding.cpp",

"torch/csrc/api/src/nn/modules/functional.cpp",

"torch/csrc/api/src/nn/modules/linear.cpp",

"torch/csrc/api/src/nn/modules/named_any.cpp",

"torch/csrc/api/src/nn/modules/rnn.cpp",

"torch/csrc/api/src/optim/adagrad.cpp",

"torch/csrc/api/src/optim/adam.cpp",

"torch/csrc/api/src/optim/lbfgs.cpp",

"torch/csrc/api/src/optim/optimizer.cpp",

"torch/csrc/api/src/optim/rmsprop.cpp",

"torch/csrc/api/src/optim/serialize.cpp",

"torch/csrc/api/src/optim/sgd.cpp",

"torch/csrc/api/src/serialize/input-archive.cpp",

"torch/csrc/api/src/serialize/output-archive.cpp",

]

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:26:00Z

@pytorchbot rebase this please

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-10T23:39:38Z

@yf225 merged this pull request in 3680cef.

Added the code and tests

4be392a

ShahriarRezghi requested review from ebetica, goldsborough and yf225 as code owners August 11, 2019 16:49

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

ezyang added the open source label Aug 11, 2019

jerryzh168 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Aug 12, 2019

yf225 reviewed Aug 15, 2019

View reviewed changes

Removed TODO from fold tests

10e07b7

yf225 mentioned this pull request Sep 8, 2019

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

Closed

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

f550d43

facebook-github-bot reviewed Sep 9, 2019

View reviewed changes

yf225 reviewed Sep 9, 2019

View reviewed changes

yf225 mentioned this pull request Sep 9, 2019

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

Open

Will Feng added 2 commits September 10, 2019 11:19

fix internal build

21d2a3a

add parity test

7ac43ac

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

pytorchbot and others added 2 commits September 10, 2019 15:26

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

5e98196

include fold.h in torch.h

f733573

facebook-github-bot reviewed Sep 10, 2019

View reviewed changes

facebook-github-bot closed this in 3680cef Sep 10, 2019

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

mruberry added the Merged label Oct 28, 2020

	torch_cpp_srcs = [
	"torch/csrc/api/src/cuda.cpp", # this just forwards stuff, no real CUDA
	"torch/csrc/api/src/data/datasets/mnist.cpp",
	"torch/csrc/api/src/data/samplers/distributed.cpp",
	"torch/csrc/api/src/data/samplers/random.cpp",
	"torch/csrc/api/src/data/samplers/sequential.cpp",
	"torch/csrc/api/src/data/samplers/stream.cpp",
	"torch/csrc/api/src/jit.cpp",
	"torch/csrc/api/src/nn/init.cpp",
	"torch/csrc/api/src/nn/module.cpp",
	"torch/csrc/api/src/nn/modules/batchnorm.cpp",
	"torch/csrc/api/src/nn/modules/conv.cpp",
	"torch/csrc/api/src/nn/modules/dropout.cpp",
	"torch/csrc/api/src/nn/modules/embedding.cpp",
	"torch/csrc/api/src/nn/modules/functional.cpp",
	"torch/csrc/api/src/nn/modules/linear.cpp",
	"torch/csrc/api/src/nn/modules/named_any.cpp",
	"torch/csrc/api/src/nn/modules/rnn.cpp",
	"torch/csrc/api/src/optim/adagrad.cpp",
	"torch/csrc/api/src/optim/adam.cpp",
	"torch/csrc/api/src/optim/lbfgs.cpp",
	"torch/csrc/api/src/optim/optimizer.cpp",
	"torch/csrc/api/src/optim/rmsprop.cpp",
	"torch/csrc/api/src/optim/serialize.cpp",
	"torch/csrc/api/src/optim/sgd.cpp",
	"torch/csrc/api/src/serialize/input-archive.cpp",
	"torch/csrc/api/src/serialize/output-archive.cpp",
	]

C++ Fold nn module #24160

C++ Fold nn module #24160

Uh oh!

Conversation

ShahriarRezghi commented Aug 11, 2019

Uh oh!

ShahriarRezghi commented Aug 11, 2019

Uh oh!

yf225 left a comment

Choose a reason for hiding this comment

Uh oh!

yf225 Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

ShahriarRezghi Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

yf225 Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

ShahriarRezghi Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

yf225 commented Aug 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ShahriarRezghi commented Sep 6, 2019

Uh oh!

yf225 commented Sep 6, 2019

Uh oh!

yf225 commented Sep 9, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

yf225 Sep 9, 2019

Choose a reason for hiding this comment

Uh oh!

yf225 commented Sep 10, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Sep 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

yf225 commented Aug 15, 2019 •

edited

Loading