C++ API: torch::nn::BatchNorm1d #28176

nuka137 · 2019-10-16T23:23:38Z

Add torch::nn::BatchNorm1d function/module support for the C++ API.
torch::nn::BatchNorm{2,3}d will be added after this PR is merged.

Related Issue: #25883

Reviewer: @yf225

I would like to discuss about below items.

Necessity of num_batches_tracked in BatchNormImplBase
- num_batches_tracked is needed to calculate momentum when we do not feed momentum argument in Python API. But in C++ API, momentum argument has a default value.
- num_batches_tracked is only used for counting up BatchNorm1d::foward() call. I think it is no necessary for user anymore.
The design of BatchNorm{1,2,3}dOptions
- We have already BatchNormOptions used for deprecated BatchNorm module. However, it is hard to use it for BatchNorm{1,2,3}dOptions because of the arguments disagreement of each modules.
- In this PR, I introduce BatchNormOptionsv2 template class for the BatchNorm{1,2,3}dOptions. But I'm not sure this design is good or not.

yf225

@nuka137 My sincere apologies for the delay and thanks so much for the investigation and the awesome work! I left some comments regarding the design choices. The scope of work is large for BatchNorm{1,2,3}d and thanks so much for working on it. :D

yf225 · 2019-10-20T20:16:11Z

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

+
+  /// A momentum multiplier for the mean and variance.
+  /// Changing this parameter after construction __is effective__.
+  TORCH_ARG(double, momentum) = 0.1;


In the Python version I believe we allow None as value for momentum, and to support it in C++ version I believe we'll need to use c10::optional<double> as type for momentum.

Thanks. I fixed it.

yf225 · 2019-10-20T20:18:53Z

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

 };

+template <size_t D>
+struct BatchNormOptionsv2 {


I feel that we can probably name it BatchNormBaseOptions

Good suggestion. I use BatchNormBaseOptions instead of BatchNormOptionsv2 .

yf225 · 2019-10-20T20:21:23Z

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


 BatchNormImpl::BatchNormImpl(const BatchNormOptions& options_) : options(options_) {
+  LOG(WARNING) << "torch::nn::BatchNorm module is deprecated."
+               << "Use BatchNorm{1,2,3}d instead.";


I think TORCH_WARN might be a better way to print the warning message, which is consistent with other parts of the C++ frontend :D

I agree with that. Change to TORCH_WARN.

yf225 · 2019-10-20T20:29:33Z

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

+ public:
+  using BatchNormImplBase<1, BatchNorm1dImpl>::BatchNormImplBase;
+
+  Tensor forward(const Tensor& input);


I feel that we might be able to even move forward to the BatchNormImplBase class :D The Python version of forward seems to call F.batch_norm, and I think we can follow the same design and rename F::batch_norm1d to F::batch_norm to match the Python version even better :D

Thanks. I fixed it.
Also, I fixed the arguments of F::batch_norm too.

yf225 · 2019-10-20T21:00:58Z

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

+Tensor BatchNorm1dImpl::forward(const Tensor& input) {
+  TORCH_CHECK(
+      input.dim() != 2 && input.dim() !=3,
+      "expected 2D or 3D input (got %dD input)", input.dim());         


I feel that we can add a virtual function _check_input_dim to the BatchNormImplBase class, override its implementation in BatchNorm1dImpl, and call this _check_input_dim function from forward, to match the Python version even better :D

OK. I added pure virtual function _check_input_dim to BatchNormImplBase class.

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

nuka137 · 2019-10-23T21:46:24Z

@yf225

Thanks for reviewing. I fixed all issues.
And I found that there is no PrettyPrintBatchNorm1d test in previous commit, so I added it in new commit.
Could you check it together?

yf225

@nuka137 Thanks so much and really appreciated the awesome work! I left some comments.

yf225 · 2019-10-24T19:45:44Z

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

  TORCH_ARG(double, momentum) = 0.1;
 };

+template <size_t D>


I think we might not need to templatize BatchNormBaseOptions over D, because the arguments in the options is not using D :D

I agree with that. I removed template parameter.

yf225 · 2019-10-24T19:47:22Z

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

+  TORCH_ARG(bool, track_running_stats) = true;
+};
+
+using BatchNorm1dOptions = BatchNormBaseOptions<1>;


and then we can just write using BatchNorm1dOptions = BatchNormBaseOptions; :D

Thanks. I fixed it.

yf225 · 2019-10-24T19:50:05Z

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

+    running_mean = this->register_buffer("running_mean", Tensor());
+    running_var = this->register_buffer("running_var", Tensor());
+    num_batches_tracked = this->register_buffer("num_batches_tracked", Tensor());
+  }


I think we can move all of the initialization logic here to reset(), and the constructor can just call reset(), which is consistent with how other C++ layers behave :D

Moved all logic to reset method, and deleted reset_parameters method and reset_track_running_stats method.

yf225 · 2019-10-24T19:51:49Z

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

+  if (options.affine()) {
+    torch::nn::init::ones_(weight);
+    torch::nn::init::zeros_(bias);
+  }


I think we can remove the reset_parameters() function and move the logic into reset() (after the initialization logic), which is consistent with how other C++ layers behave :D

Moved all logic to reset method, and deleted reset_parameters method and reset_track_running_stats method.

yf225 · 2019-10-24T19:53:27Z

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

+void BatchNormImplBase<D, Derived>::pretty_print(std::ostream& stream) const {
+  stream << std::boolalpha
+         << "torch::nn::BatchNorm" << D << "d("
+         << "num_features=" << options.num_features() << ", "


I think we can remove the printing of num_features= here, to match the Python version even better :D

I fixed it.

yf225 · 2019-10-24T20:06:23Z

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

+                         const Tensor& bias, bool training,
+                         double momentum, double eps) {
+  if (training) {
+    std::vector<int64_t> size = input.sizes().vec();


I suspect we could do

Suggested change

std::vector<int64_t> size = input.sizes().vec();

auto size = input.sizes();

under the hood it uses ArrayRef as type for size :D

Thanks. I followed your instruction.

yf225 · 2019-10-24T20:09:32Z

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

+    training,
+    momentum,
+    eps,
+    torch::cuda::cudnn_is_available());


I think the C++ equivalent of torch.backends.cudnn.enabled is at::globalContext().userEnabledCuDNN()

I replaced torch.backends.cudnn.enabled to at::globalContext().userEnabledCuDNN().

yf225 · 2019-10-24T20:10:27Z

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

 #include <torch/nn/functional/normalization.h>
 #include <torch/nn/functional/pooling.h>
 #include <torch/nn/functional/vision.h>
+#include <torch/nn/functional/batchnorm.h>


It would be awesome to put it above #include <torch/nn/functional/distance.h>, to sort them alphabetically :D

OK. I followed your instruction.

test/cpp/api/modules.cpp

test/cpp/api/functional.cpp

yf225 · 2019-10-24T20:20:40Z

It would be awesome to update the BatchNorm1d entry in test/cpp_api_parity/parity-tracker.md as well. Thanks so much for the great work!

nuka137 · 2019-10-25T13:57:57Z

@yf225

Thanks for your instructions.
I followed them except for templatize BatchNormImplBase.
I think BatchNormImplBase is still needed to output dimension in pretty_print.
What do you think about it?

nuka137 · 2019-10-25T14:10:08Z

@yf225

Hi, I found that there is a change in PrettyPrintHardtanh test in this PR.
I believe that this is a accident from commit ada8d54 because PrettyPrintHardtanh test is failed now.
If you don't mind, I will fixed this in PR.

yf225

@nuka137 Thanks so much for the awesome work! I took the liberty to merge BatchNormBaseOptions into BatchNormOptions because after thinking about it more I feel that forcing F::batch_norm to use BatchNormBaseOptions might not be a good idea, aesthetically speaking. Instead I renamed features to num_features and stateful to track_running_stats in BatchNormOptions, which is backward-compatibility breaking, but I think is better for the long term.

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

@yf225 merged this pull request in cbc234b.

Summary: Add torch::nn::BatchNorm{2,3}d module and functional support for the C++ API. Related Issue: #25883 #28176 Reviewer: yf225 Pull Request resolved: #28936 Differential Revision: D18266918 Pulled By: yf225 fbshipit-source-id: f432904c72985d52ec52cb992cceb372b6ff0244

Summary: Add torch::nn::BatchNorm{2,3}d module and functional support for the C++ API. Related Issue: #25883 #28176 Reviewer: yf225 Pull Request resolved: #28936 Differential Revision: D18274584 Pulled By: yf225 fbshipit-source-id: 3784eee9f8947f6c7c9f1699544a3d36a1a019b7

C++ API: torch::nn::BatchNorm1d

af0bef0

nuka137 requested review from ebetica, goldsborough and yf225 as code owners October 16, 2019 23:23

yf225 mentioned this pull request Oct 18, 2019

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

Open

yf225 reviewed Oct 20, 2019

View reviewed changes

Apply review comments

c2c24b9

yf225 reviewed Oct 24, 2019

View reviewed changes

Merge branch 'master' into pr-6

ada8d54

Apply review comments

1504f47

Will Feng added 2 commits October 28, 2019 21:58

minor fixes

b6726e3

use a different design for options

469ff6e

yf225 force-pushed the pr-6 branch from bd910de to 469ff6e Compare October 29, 2019 02:23

Will Feng added 6 commits October 28, 2019 22:48

fix header include

cc92aa8

more renaming

fd682fd

more renaming

34368f5

fix momentum

ab21f2d

fix momentum again

65ddd96

fix functional call

de696e1

yf225 added the module: cpp Related to C++ API label Oct 29, 2019

Will Feng added 2 commits October 29, 2019 00:01

fix test

1c08b1e

more renaming

ef95524

yf225 approved these changes Oct 29, 2019

View reviewed changes

facebook-github-bot reviewed Oct 29, 2019

View reviewed changes

Will Feng added 2 commits October 29, 2019 11:50

fix error: comparison between signed and unsigned integer expressions

6ef0cda

Merge branch 'master' into pr-6

4ac309c

facebook-github-bot reviewed Oct 29, 2019

View reviewed changes

yf225 mentioned this pull request Oct 29, 2019

[C++ API] InstanceNorm{1,2,3}d #28790

Closed

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

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

nuka137 mentioned this pull request Oct 30, 2019

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

Closed

mruberry added the Merged label Oct 28, 2020

	std::vector<int64_t> size = input.sizes().vec();
	auto size = input.sizes();

C++ API: torch::nn::BatchNorm1d #28176

C++ API: torch::nn::BatchNorm1d #28176

Uh oh!

Conversation

nuka137 commented Oct 16, 2019

Uh oh!

yf225 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nuka137 commented Oct 23, 2019

Uh oh!

yf225 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yf225 commented Oct 24, 2019

Uh oh!

nuka137 commented Oct 25, 2019

Uh oh!

nuka137 commented Oct 25, 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!

facebook-github-bot left a comment

yf225 left a comment •

edited

Loading