Add nn::functional::normalize() to C++ Frontend #27280

gtamba · 2019-10-03T08:06:44Z

Addresses #27048

PR Summary:

Files Added:

torch/csrc/api/include/torch/nn/options/normalization.h
torch/csrc/api/include/torch/nn/functional/normalization.h

Files Modified:

test/cpp/api/functional.cpp
torch/csrc/api/include/torch/nn/functional.h

@yf225 : I couldn't find a C++ equivalent of gradcheck(), is there such a function or is it sufficient to call .backward() in the test body? I don't think any solutions are checked for the Python tests.

gtamba · 2019-10-03T15:51:53Z

@yf225 : This might be a silly question but do I need to do anything additional to get the new files included in the build targets? I couldn't find hints from other pull requests unless they were .cpp modules but now trying to build the test binary gives me a not in namespace error.

edit: Okay sorry I missed this but I I needed to add my headers to functional.h

yf225

@gtamba Thanks a lot for the great work! My sincere apologies for the delay in reviewing as I was out of office last week. I left some minor comments in the PR, and we should be able to merge it very soon :D

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

torch/csrc/api/include/torch/nn/functional/normalization.h

test/cpp/api/functional.cpp

yf225 · 2019-10-07T23:13:58Z

@pytorchbot rebase this please

gtamba · 2019-10-08T05:41:27Z

@yf225 : it seems that the bot has done a merge instead of a rebase, should I reset the head to my initial commit before the merge, add my changes and then rebase to hide the merge commit or is that all right? (I'm assuming that this sort of small PR is expected with 1 commit )

yf225 · 2019-10-08T05:53:51Z

@gtamba : it's totally alright and I think we should work on top of the merge commit. At PR merge time, all commits in this PR will automatically be squashed into one commit, so we don't need to worry about the number of commits we make to this PR :D

gtamba · 2019-10-08T08:45:06Z

@pytorchbot retest this please

gtamba · 2019-10-08T19:43:04Z

@pytorchbot retest this please

gtamba · 2019-10-08T23:03:11Z

@pytorchbot rebase this please

test/cpp/api/functional.cpp

torch/csrc/api/include/torch/nn/functional.h

torch/csrc/api/include/torch/nn/functional/normalization.h

test/cpp/api/functional.cpp

…m_cpp

yf225 · 2019-10-13T00:03:09Z

@gtamba I feel that we might need to add TORCH_API to all functions in torch/csrc/api/include/torch/arg.h, although it's really weird why we would need this. :/

yf225 · 2019-10-13T00:05:12Z

torch/csrc/api/include/torch/nn/functional/normalization.h

+
+inline Tensor normalize(
+    const Tensor& input,
+    const NormalizeOptions& options,


When we confirm that options = {} doesn't cause the problem, I believe we should change it back to options = {}, and it would be awesome to have a test for the F::normalize(input) (i.e. no options) use case as well :D Thanks so much for your help @gtamba !

@yf225 : Can I still make these changes? I notice that facebook-bot is trying to land the pull request, and I didn't add the test for no options yet

@gtamba : Yes sorry this is my fault - didn't notice this before trying to land yesterday :( I saw you updated the PR (thanks a lot :D) and I will try to land it again

gtamba · 2019-10-13T11:49:11Z

@gtamba I feel that we might need to add TORCH_API to all functions in torch/csrc/api/include/torch/arg.h, although it's really weird why we would need this. :/

I noticed that most other functions of the functional module also have a base NN Module and I couldn't find one which only has options included this way without a cpp counterpart declaration. I wonder if that could have something to do with it. But yeah I am confused from the error it's saying that it can't resolve the Options struct's internal variables, but the Options struct is pretty much identical to any other options struct in functional/

yf225 · 2019-10-13T22:16:59Z

@gtamba I think your assessment is correct, and I just pushed a commit to add TORCH_API to arg.h so that we don't need to have the cpp counterpart declaration. Hopefully that would work :)

facebook-github-bot

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

yf225

Thanks so much for the awesome work @gtamba!

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-10-14T16:20:30Z

@yf225 merged this pull request in cc5c34a.

yf225 · 2019-10-21T17:01:30Z

@gtamba Curious are you currently working on the Dropout modules? Please let me know if there is anything I can do to make the implementation easier. Thanks a lot for your help!

gtamba · 2019-10-22T02:11:01Z

@yf225 : Yes I'm currently working on the dropout modules, it took me a while to get started due to other commitments but I should have an initial PR up soon!

yf225 · 2019-10-22T02:14:49Z

@gtamba Awesome thanks so much for your help! Please keep me posted how it goes :D

gtamba · 2019-10-31T22:45:05Z

@yf225 : I am sorry for the delayed update, certain things came in the way and I could not work on this for a while. I noticed that #28424 has some relevant changes that might overlap with the rest of the dropout modules, I think I can follow the changes in Dropoutoptions in this PR? (Or just wait for it to get merged :))

yf225 · 2019-10-31T23:59:29Z

@gtamba : No worries about it at all! :D Yes I think #28424 is related and we also have some internal mandate to have the Dropout modules done within the next two weeks - curious do you mind if I take over the Dropout modules work instead? Thanks so much for your help again!

gtamba · 2019-11-01T01:01:08Z

@yf225 : That's totally fine it makes sense. If you're working on something else presently though, I should have a draft PR soon and it should wrap up shortly (+ code review) after the weekend, but iterations would make it risk taking too long. Feel free to hand me anything else. Again I apologize sorry for the delay

yf225 · 2019-11-01T01:23:50Z

@gtamba : Thanks so much for your help! Yes in that case I think if we can have it done over the next week or so it would be really awesome (I can personally help with the iterations by making changes to the PR if necessary). Please keep me posted on how it goes :D Thanks again!

Summary: Addresses pytorch#27048 PR Summary: Files Added: _torch/csrc/api/include/torch/nn/options/normalization.h torch/csrc/api/include/torch/nn/functional/normalization.h_ Files Modified: _test/cpp/api/functional.cpp torch/csrc/api/include/torch/nn/functional.h_ --- yf225 : I couldn't find a C++ equivalent of gradcheck(), is there such a function or is it sufficient to call .backward() in the test body? I don't think any solutions are checked for the Python tests. Pull Request resolved: pytorch#27280 Differential Revision: D17902109 Pulled By: yf225 fbshipit-source-id: 1bce1a88103d0f1848633fec90fde95ea8f3d1ed

gtamba requested review from ebetica, goldsborough and yf225 as code owners October 3, 2019 08:06

pytorchbot added the module: cpp Related to C++ API label Oct 3, 2019

gtamba force-pushed the norm_cpp branch from fb16a08 to 40afdf6 Compare October 3, 2019 08:20

ezyang added the open source label Oct 3, 2019

gtamba force-pushed the norm_cpp branch from 40afdf6 to 02a0f18 Compare October 3, 2019 16:31

yf225 mentioned this pull request Oct 6, 2019

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

Open

gtamba force-pushed the norm_cpp branch 3 times, most recently from 9b6d5c1 to 646776b Compare October 6, 2019 17:00

Add nn::functional::normalize()

fc367f8

gtamba force-pushed the norm_cpp branch from 646776b to fc367f8 Compare October 7, 2019 14:34

yf225 reviewed Oct 7, 2019

View reviewed changes

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

5e00579

Update nn:functional::normalize() tests and remove whitespace/newline

6041153

gtamba changed the title ~~[WIP] Add nn::functional::normalize() to C++ Frontend~~ Add nn::functional::normalize() to C++ Frontend Oct 8, 2019

Sort functional.h header imports alphabetically

40978cf

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

ad2ab5d

yf225 reviewed Oct 9, 2019

View reviewed changes

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

gtamba added 2 commits October 10, 2019 12:37

Update tests, style, and syntax

d65bd04

Merge branch 'norm_cpp' of https://github.com/gtamba/pytorch into nor…

8083c28

…m_cpp

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

ee6434b

gtamba force-pushed the norm_cpp branch 2 times, most recently from 4d936a7 to 849fb5e Compare October 12, 2019 14:48

Try to fix windows build

fb3b519

gtamba force-pushed the norm_cpp branch from 849fb5e to fb3b519 Compare October 12, 2019 15:20

yf225 reviewed Oct 13, 2019

View reviewed changes

add TORCH_API to arg.h

25fe70a

try to fix Windows build by adding options/normalization.cpp

776710b

facebook-github-bot reviewed Oct 14, 2019

View reviewed changes

yf225 approved these changes Oct 14, 2019

View reviewed changes

facebook-github-bot reviewed Oct 14, 2019

View reviewed changes

Make NormalizeOptions optional

3780a60

facebook-github-bot reviewed Oct 14, 2019

View reviewed changes

facebook-github-bot closed this in cc5c34a Oct 14, 2019

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

gtamba deleted the norm_cpp branch October 17, 2019 09:35

mruberry added the Merged label Oct 28, 2020

Add nn::functional::normalize() to C++ Frontend #27280

Add nn::functional::normalize() to C++ Frontend #27280

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

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 commented Oct 7, 2019

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

gtamba commented Oct 8, 2019

Uh oh!

gtamba commented Oct 8, 2019

Uh oh!

gtamba commented Oct 8, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yf225 commented Oct 13, 2019

Uh oh!

yf225 Oct 13, 2019

Choose a reason for hiding this comment

Uh oh!

gtamba Oct 14, 2019

Choose a reason for hiding this comment

Uh oh!

yf225 Oct 14, 2019

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

yf225 commented Oct 13, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

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 left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 14, 2019

Uh oh!

yf225 commented Oct 21, 2019

Uh oh!

gtamba commented Oct 22, 2019

Uh oh!

yf225 commented Oct 22, 2019

Uh oh!

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

Uh oh!

Uh oh!

gtamba commented Oct 3, 2019 •

edited

Loading

gtamba commented Oct 3, 2019 •

edited

Loading

gtamba commented Oct 8, 2019 •

edited

Loading

yf225 commented Oct 8, 2019 •

edited

Loading

gtamba commented Oct 13, 2019 •

edited

Loading

gtamba commented Oct 31, 2019 •

edited

Loading

gtamba commented Nov 1, 2019 •

edited

Loading