[C++] Add nn.Bilinear to C++ Frontend #26082

MJ10 · 2019-09-12T03:42:48Z

Adds support for the Bilinear layer to the C++ frontend

yf225 · 2019-09-12T04:19:45Z

@MJ10 Thanks a lot for the PR! We might need to remove the changes in third_party/fbgemm because they are unrelated and likely due to out-of-sync submodule.

MJ10 · 2019-09-12T05:35:46Z

@yf225 thanks for pointing that out! I've removed it.

yf225 · 2019-09-12T15:25:07Z

@MJ10 Thanks a lot! I am curious would you like to select "allow updates from maintainers" as in https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork?

MJ10 · 2019-09-12T15:33:14Z

@yf225 it's already enabled

MJ10 · 2019-10-07T04:34:05Z

@yf225 do let me know if any changes are required, would be happy to update this PR :)

yf225

@MJ10 Thanks so much for the contribution and my sincere apologies for the delay. :( The folder structure of C++ torch::nn modules have changed considerably in the past few weeks, and we have finally settled down on the design. I believe we can easily move this PR to the new design and will be able to merge it very soon. :) Thanks again!

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

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

test/cpp/api/modules.cpp

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

yf225 · 2019-10-07T05:50:15Z

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

+class TORCH_API BilinearImpl : public Cloneable<BilinearImpl> {
+ public:
+  BilinearImpl(int64_t in1, int64_t in2, int64_t out) : BilinearImpl(BilinearOptions(in1, in2, out)) {}
+  explicit BilinearImpl(BilinearOptions options);


It would be awesome to change this to

Suggested change

explicit BilinearImpl(BilinearOptions options);

explicit BilinearImpl(const BilinearOptions& options_);

to match the design of other torch::nn modules.

MJ10 · 2019-10-10T12:48:54Z

@yf225 Thanks for the detailed comments! Apologies I couldn't follow the change in the design apologies for the delay. I've made the requested changes.

yf225

Thanks so much for the changes @MJ10! I left some comments, and we should be able to merge this PR very soon :D

yf225 · 2019-10-10T21:28:38Z

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

+
+void BilinearImpl::reset() {
+  weight =
+      register_parameter("weight", torch::empty({options.out_features_, options.in1_features_, options.in2_features_}));


options.name_ are private now and we would need to change it to options.name(). (For example, options.out_features_ would be options.out_features().)

yf225 · 2019-10-10T21:42:17Z

@MJ10 I fixed a code conflict using the github code editor, and I believe we would need to pull from the remote branch first. :D

yf225 · 2019-10-10T21:45:02Z

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

+
+BilinearOptions::BilinearOptions(int64_t in1_features, int64_t in2_features, int64_t out_features) : in1_features_(in1_features), in2_features_(in2_features), out_features_(out_features) {}
+
+BilinearImpl::BilinearImpl(BilinearOptions options) : options(options) {


We would need to also change this to:

Suggested change

BilinearImpl::BilinearImpl(BilinearOptions options) : options(options) {

BilinearImpl::BilinearImpl(const BilinearOptions& options_) : options(options_) {

yf225 · 2019-10-11T19:23:06Z

@pytorchbot rebase this please

yf225 · 2019-10-15T00:55:44Z

This is blocked by #27948. I will merge that PR as soon as possible to unblock this.

MJ10 · 2019-10-15T05:54:05Z

Thanks a lot @yf225 !

Summary: C++ API `Module::register_parameter` should accept undefined Tensor as parameter, which is equivalent to `module.register_parameter("param", None)` in Python API. This unblocks #26082 and #27156. Pull Request resolved: #27948 Differential Revision: D17931739 Pulled By: yf225 fbshipit-source-id: 21bdfc88e66e3dc39f3caf608a6a3de48c510fa9

test/cpp/api/functional.cpp

yf225 · 2019-10-16T05:44:09Z

@pytorchbot rebase this please

facebook-github-bot

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

facebook-github-bot · 2019-10-16T17:35:34Z

@yf225 merged this pull request in f38beff.

MJ10 · 2019-10-16T18:10:08Z

Thanks a lot for all the help @yf225

…#27948) Summary: C++ API `Module::register_parameter` should accept undefined Tensor as parameter, which is equivalent to `module.register_parameter("param", None)` in Python API. This unblocks pytorch#26082 and pytorch#27156. Pull Request resolved: pytorch#27948 Differential Revision: D17931739 Pulled By: yf225 fbshipit-source-id: 21bdfc88e66e3dc39f3caf608a6a3de48c510fa9

Summary: Adds support for the Bilinear layer to the C++ frontend Pull Request resolved: pytorch#26082 Differential Revision: D17954148 Pulled By: yf225 fbshipit-source-id: 5e746bdea29b00e25969cd7a22044b8059b53687

MJ10 added 3 commits September 11, 2019 21:53

add nn::bilinear to cpp frontend

9845bdc

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

7f73b08

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

1c3505e

MJ10 requested review from ebetica, goldsborough and yf225 as code owners September 12, 2019 03:42

pytorchbot added module: cpp Related to C++ API module: third_party labels Sep 12, 2019

fix nn::Bilinear tests

8400b9e

MJ10 force-pushed the master branch from a4f66cb to 8400b9e Compare September 12, 2019 05:00

ezyang added the open source label Sep 18, 2019

yf225 mentioned this pull request Sep 24, 2019

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

Open

pytorch deleted a comment from pytorchbot Oct 3, 2019

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

remove parity-tracker entry

69448e9

yf225 reviewed Oct 7, 2019

View reviewed changes

MJ10 added 3 commits October 10, 2019 18:06

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

ed4ffd6

resolve comments

23598ad

Merge branch 'master' of https://github.com/MJ10/pytorch

359c6d7

yf225 reviewed Oct 10, 2019

View reviewed changes

Merge branch 'master' into master

57bf23a

yf225 reviewed Oct 10, 2019

View reviewed changes

nn:bilinear fix test and add default argument

32bf364

pytorchbot and others added 6 commits October 11, 2019 19:23

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

9bd01e4

try to fix build error

c2197a5

fix options arg access

f0f17c0

remove extra )

f5d52aa

remove extra )

00c81ee

change torch::Tensor({...}) to torch::tensor({...})

933c97d

yf225 mentioned this pull request Oct 14, 2019

Allow passing undefined Tensor to Module::register_parameter #27948

Closed

Will Feng added 4 commits October 15, 2019 15:07

Merge branch 'master' into master

120442c

requires_grad=false when registering Tensor()

c1bae67

update

d721248

update

a1640ec

yf225 reviewed Oct 16, 2019

View reviewed changes

test/cpp/api/functional.cpp Outdated Show resolved Hide resolved

yf225 reviewed Oct 16, 2019

View reviewed changes

test/cpp/api/functional.cpp Outdated Show resolved Hide resolved

workaround

cd4f11f

facebook-github-bot reviewed Oct 16, 2019

View reviewed changes

facebook-github-bot closed this in f38beff Oct 16, 2019

facebook-github-bot added the merged label Oct 16, 2019

mruberry added the Merged label Oct 28, 2020

	explicit BilinearImpl(BilinearOptions options);
	explicit BilinearImpl(const BilinearOptions& options_);


		BilinearOptions::BilinearOptions(int64_t in1_features, int64_t in2_features, int64_t out_features) : in1_features_(in1_features), in2_features_(in2_features), out_features_(out_features) {}

		BilinearImpl::BilinearImpl(BilinearOptions options) : options(options) {

	BilinearImpl::BilinearImpl(BilinearOptions options) : options(options) {
	BilinearImpl::BilinearImpl(const BilinearOptions& options_) : options(options_) {

[C++] Add nn.Bilinear to C++ Frontend #26082

[C++] Add nn.Bilinear to C++ Frontend #26082

Uh oh!

Conversation

MJ10 commented Sep 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yf225 commented Sep 12, 2019

Uh oh!

MJ10 commented Sep 12, 2019

Uh oh!

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

Uh oh!

Uh oh!

MJ10 commented Sep 12, 2019

Uh oh!

MJ10 commented Oct 7, 2019

Uh oh!

yf225 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yf225 Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

MJ10 commented Oct 10, 2019

Uh oh!

yf225 left a comment • 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!

yf225 Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

yf225 commented Oct 10, 2019

Uh oh!

yf225 Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

yf225 commented Oct 11, 2019

Uh oh!

yf225 commented Oct 15, 2019

Uh oh!

MJ10 commented Oct 15, 2019

Uh oh!

Uh oh!

Uh oh!

yf225 commented Oct 16, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 16, 2019

Uh oh!

MJ10 commented Oct 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

MJ10 commented Sep 12, 2019 •

edited

Loading

yf225 commented Sep 12, 2019 •

edited

Loading

yf225 left a comment •

edited

Loading