Skip to content

C++ API: torch::nn::BatchNorm{2,3}d #28936

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
nuka137 wants to merge 5 commits into from
Closed

C++ API: torch::nn::BatchNorm{2,3}d #28936

nuka137 wants to merge 5 commits into from

Conversation

@nuka137
Copy link
Contributor

@nuka137 nuka137 commented Oct 30, 2019 •
edited
Loading

Add torch::nn::BatchNorm{2,3}d module and functional support for the C++ API.

Related Issue: #25883 #28176

Reviewer: @yf225

@yf225 yf225 added the module: cpp Related to C++ API label Nov 1, 2019
yf225
yf225 reviewed Nov 1, 2019
View reviewed changes
Copy link
Contributor

@yf225 yf225 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nuka137 Thanks so much for the awesome work and thorough tests! I have one very minor comment.

torch/csrc/api/src/nn/modules/batchnorm.cpp
"expected 5D input (got ", input.dim(), "D input)");
}

template class BatchNormImplBase<3, BatchNorm3dImpl>;
Copy link
Contributor

@yf225 yf225 Nov 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From code organization point of view, it would be awesome to have all template specializations next to each other for easier discovery:

template class BatchNormImplBase<1, BatchNorm1dImpl>;
template class BatchNormImplBase<2, BatchNorm2dImpl>;
template class BatchNormImplBase<3, BatchNorm3dImpl>;

Thanks so much for your help!

Copy link
Contributor Author

@nuka137 nuka137 Nov 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Fixed it.

@nuka137
Copy link
Contributor Author

nuka137 commented Nov 1, 2019

@yf225

Thanks for your review. I fixed the source code you commented.
Could you check it again?

yf225
yf225 approved these changes Nov 1, 2019
View reviewed changes
Copy link
Contributor

@yf225 yf225 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nuka137 Thanks so much for the awesome work!

facebook-github-bot
facebook-github-bot reviewed Nov 1, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@yf225 yf225 mentioned this pull request Nov 1, 2019
Open
@ezyang
Copy link
Contributor

ezyang commented Nov 1, 2019

TEsts are failing:


Nov 01 17:32:00 [ RUN      ] ModulesTest.BatchNorm2dStateful
Nov 01 17:32:00 /var/lib/jenkins/workspace/test/cpp/api/modules.cpp:1224: Failure
Nov 01 17:32:00 Expected equality of these values:
Nov 01 17:32:00   bn->num_batches_tracked.dim()
Nov 01 17:32:00     Which is: 0
Nov 01 17:32:00   1

@yf225
Copy link
Contributor

yf225 commented Nov 1, 2019

@nuka137 Sorry it was my fault that causes this PR to be reverted - I will re-open this PR and make fixes directly to this PR, and then makes sure it passes CI before merging again.

@yf225 yf225 reopened this Nov 1, 2019
facebook-github-bot
facebook-github-bot reviewed Nov 1, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@yf225 yf225 removed the merged label Nov 1, 2019
@pytorch pytorch deleted a comment from facebook-github-bot Nov 1, 2019
facebook-github-bot
facebook-github-bot reviewed Nov 1, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Nov 2, 2019

@yf225 merged this pull request in a68c1e1.

@jerryzh168
Copy link
Contributor

jerryzh168 commented Nov 2, 2019 •
edited
Loading

Can we split aten function to aten::batch_norm_1d, aten::batch_norm_2d and aten::batch_norm_3d and move the dimension check inside the aten function? This will simplify the other works that needs to match batchnorm module or op. cc @ZolotukhinM

@yf225
Copy link
Contributor

yf225 commented Nov 5, 2019

@jerryzh168 Yes I think that would be beneficial - the C++ API implementation here is just strictly following Python API implementation, and we can change it once the Python implementation is changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@yf225 yf225 yf225 approved these changes

@ebetica ebetica Awaiting requested review from ebetica

@goldsborough goldsborough Awaiting requested review from goldsborough

Assignees

No one assigned

Labels

Merged module: cpp Related to C++ API

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@nuka137 @ezyang @yf225 @facebook-github-bot @jerryzh168 @mruberry