Add nn::Flatten to C++ Frontend #28072

mrsalehi · 2019-10-16T12:32:43Z

Summary:
Adds torch::nn::Flatten module support for the C++ API.

Reviewer: @yf225

yf225

@mrsalehi Thanks a lot for the great work! I left some comments.

yf225 · 2019-10-16T20:16:54Z

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

+}
+
+Tensor FlattenImpl::forward(const Tensor& input) {
+  return input.flatten();


In Python version, torch.nn.Flatten takes start_dim and end_dim as optional constructor arguments, and we'd need to do the same for the C++ version.

We would also need to add tests for the optional constructor arguments start_dim and end_dim as well. Thanks a lot for your help!

mrsalehi · 2019-10-24T19:15:55Z

@yf225 Sorry for my delay in making the changes.
I think that the implementation should be alright, though I am not quite sure if I have run the tests properly. Could you please guide me through running the tests of a C++ module?

yf225

@mrsalehi Thanks a lot for the update! I left some comments. For running the tests of a C++ module, we can run ./build/bin/test_api --gtest_filter=ModulesTest* --gtest_stack_trace_depth=10 --gmock_verbose=info in the PyTorch root folder after building PyTorch locally :D

yf225 · 2019-10-25T18:42:32Z

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


+/// Options for the `Flatten` module.
+struct TORCH_API FlattenOptions {
+  FlattenOptions(int64_t start_dim, int64_t end_dim);


I think for options that has two or more optional arguments, we usually don't provide the non-default constructor (e.g. CosineEmbeddingLossOptions), and we should likely remove the constructor here

yf225 · 2019-10-25T18:43:56Z

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

+}
+
+Tensor FlattenImpl::forward(const Tensor& input) {
+  return torch::flatten(input, options.start_dim(), options.end_dim());


We might be able to call

Suggested change

return torch::flatten(input, options.start_dim(), options.end_dim());

return input.flatten(input, options.start_dim(), options.end_dim());

to match the Python version even better :D

yf225 · 2019-10-25T18:44:58Z

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

+/// A placeholder for Flatten operator
+class TORCH_API FlattenImpl : public Cloneable<FlattenImpl> {
+ public:
+  FlattenImpl(int64_t start_dim, int64_t end_dim) : FlattenImpl(FlattenOptions(start_dim, end_dim)) {}


We can remove this constructor and only keep explicit FlattenImpl(const FlattenOptions& options_);

mrsalehi · 2019-10-29T08:27:09Z

Hi @yf225!
I fixed the issues and ran the tests and everything worked fine. But, I still have some questions.
It is mentioned that the implementation of C++ modules must exactly match their Python's counterparts. I doubt if this is true for CosineEmbeddingLoss; all of the arguments of this module are optional in Python yet there is no options={} in the signature of the constructor in the C++ version. Moreover, margin is printed into the stream in C++ implementation while it is not printed in the Python's implementation.
I would appreciate if you could clarify this issue for me!

yf225 · 2019-10-29T16:17:32Z

there is no options={} in the signature of the constructor in the C++ version

I believe this should be what we are looking for:

pytorch/torch/csrc/api/include/torch/nn/modules/loss.h

Lines 184 to 186 in 5fbec1f

    
           struct TORCH_API CosineEmbeddingLossImpl : public Cloneable<CosineEmbeddingLossImpl> { 
        
             explicit CosineEmbeddingLossImpl( 
        
                 const CosineEmbeddingLossOptions& options_ = {});

margin is printed into the stream in C++ implementation while it is not printed in the Python's implementation

Thanks a lot for catching the issue! Yes I think the pretty print is not consistent now and we are thinking about whether we should print the full set of options or strictly follow the Python version (which does not print the full set of options in all cases). For now I think we can print torch::nn::Flatten() to be the same as the Python version.

yf225

@mrsalehi Thanks so much for the awesome work! :D

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-10-30T01:06:14Z

@yf225 merged this pull request in dfe7b25.

mrsalehi added 2 commits October 16, 2019 11:59

Add nn.Flatten to C++ Frontend

101ff3a

Add nn.Flatten to C++ Frontend

3748411

mrsalehi requested review from ebetica, goldsborough and yf225 as code owners October 16, 2019 12:32

yf225 reviewed Oct 16, 2019

View reviewed changes

yf225 mentioned this pull request Oct 16, 2019

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

Open

Will Feng and others added 2 commits October 16, 2019 18:44

Merge branch 'master' into mrsalehi/Flatten

f32e884

Adding support for the optional arguments of nn::Flatten

348663e

yf225 reviewed Oct 25, 2019

View reviewed changes

Fixed constructors issues

bb41db4

Will Feng added 2 commits October 29, 2019 12:21

Merge branch 'master' into mrsalehi/Flatten

60a5557

update parity table

29a7251

yf225 approved these changes Oct 29, 2019

View reviewed changes

facebook-github-bot reviewed Oct 29, 2019

View reviewed changes

facebook-github-bot closed this in dfe7b25 Oct 30, 2019

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

mruberry added the Merged label Oct 28, 2020

	return torch::flatten(input, options.start_dim(), options.end_dim());
	return input.flatten(input, options.start_dim(), options.end_dim());

Add nn::Flatten to C++ Frontend #28072

Add nn::Flatten to C++ Frontend #28072

Uh oh!

Conversation

mrsalehi commented Oct 16, 2019

Uh oh!

yf225 left a comment

Choose a reason for hiding this comment

Uh oh!

yf225 Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

yf225 Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

mrsalehi commented Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 25, 2019

Choose a reason for hiding this comment

Uh oh!

yf225 Oct 25, 2019

Choose a reason for hiding this comment

Uh oh!

yf225 Oct 25, 2019

Choose a reason for hiding this comment

Uh oh!

mrsalehi commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yf225 commented Oct 29, 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!

facebook-github-bot commented Oct 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mrsalehi commented Oct 24, 2019 •

edited

Loading

yf225 left a comment •

edited

Loading

mrsalehi commented Oct 29, 2019 •

edited

Loading