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

divyanshsinghvi · 2019-10-28T17:05:57Z

I have a few doubts related to implementation:

What tests do I have to write?
What does _load_state_from_dict does?
Do I need to override reset() function as I can not see it's utility?
InstanceNormOptions could be removed with BatchNormOptions, but I find that
track_running_status is not defined instead stateful is defined.

InstanceNorm{1,2,3}d #25883

Tests need to be added

ssnl · 2019-10-28T17:29:53Z

_load_state_from_dict is used to resolve BC issue for loading old python instance norm state dict, because the state dict of instance norm changed at one point. I believe that it is not relevant for CPP API.

yf225

@divyanshsinghvi Thanks so much for the awesome work! I believe we will need to rebase this on top of #28176 after it's merged, because #28176 fixes the track_running_status vs. stateful issue that you mentioned, and also creates BatchNormImplBase which we'll need to subclass InstanceNormImpl from.

yf225 · 2019-10-29T15:58:58Z

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

+namespace torch {
+namespace nn {
+
+/// Base class for all (dimesnsion-specialized) instanceNorm modules


nit:

Suggested change

/// Base class for all (dimesnsion-specialized) instanceNorm modules

/// Base class for all (dimension-specialized) InstanceNorm modules

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

yf225 · 2019-10-29T15:59:57Z

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

+
+  Tensor forward(const Tensor& input);
+
+  /// The optons with which this `Module` was constructed.


nit:

Suggested change

/// The optons with which this `Module` was constructed.

/// The options with which this `Module` was constructed.

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

yf225 · 2019-10-29T16:09:15Z

For this PR we shouldn't need to override reset - we will get more clarity about it once we are able to rebase on top of #28176 :D

yf225 · 2019-10-30T02:53:42Z

@divyanshsinghvi I just rebased this PR on top of upstream master which contains the changes in #28176. I think we can let InstanceNormImpl subclass from the new BatchNormImplBase class, which mirrors the class hierarchy in Python. I also think we do need to define a separate InstanceNormOptions struct, because InstanceNorm constructor's affine and track_running_stats parameters take a different default value than the BatchNorm constructor's affine and track_running_stats parameters.

yf225 · 2019-10-30T02:59:16Z

I think it should be sufficient to provide an empty override for the reset() function, because I believe not overriding it would cause InstanceNormImpl to become an abstract class, and we cannot create objects of abstract classes.

yf225 · 2019-10-30T03:05:11Z

For tests, I think we don't need to be comprehensive and can just check for common input / output. This is an example for F::batch_norm

pytorch/test/cpp/api/functional.cpp

Lines 1269 to 1294 in a8b63ca

    
           TEST_F(FunctionalTest, BatchNorm1d) { 
        
             int num_features = 5; 
        
             double eps = 1e-05; 
        
             double momentum = 0.1; 
        
             auto input = torch::randn({2, 5}); 
        
             auto mean = torch::randn(5); 
        
             auto variance = torch::rand(5); 
        
             auto weight = torch::ones({num_features}); 
        
             auto bias = torch::zeros({num_features}); 
        
             auto output = F::batch_norm( 
        
               input, mean, variance, 
        
               BatchNormOptions().weight(weight).bias(bias).momentum(momentum).eps(eps), 
        
               /*training=*/false); 
        
             auto expected = (input - mean) / torch::sqrt(variance + eps); 
        
             ASSERT_TRUE(output.allclose(expected)); 
        
           } 
        
           TEST_F(FunctionalTest, BatchNorm1dDefaultOptions) { 
        
             auto input = torch::randn({2, 5}); 
        
             auto mean = torch::randn(5); 
        
             auto variance = torch::rand(5); 
        
             auto output = F::batch_norm(input, mean, variance); 
        
             auto expected = (input - mean) / torch::sqrt(variance + 1e-5); 
        
             ASSERT_TRUE(output.allclose(expected)); 
        
           }

and we can do the same for the module tests as well

pytorch/test/cpp/api/modules.cpp

Lines 1136 to 1147 in a8b63ca

    
           TEST_F(ModulesTest, BatchNorm1d) { 
        
             BatchNorm1d bn(BatchNorm1dOptions(5)); 
        
             bn->eval(); 
        
             auto input = torch::randn({2, 5}, torch::requires_grad()); 
        
             auto output = bn->forward(input); 
        
             auto s = output.sum(); 
        
             s.backward(); 
        
             ASSERT_EQ(input.sizes(), input.grad().sizes()); 
        
             ASSERT_TRUE(input.grad().allclose(torch::ones({2, 5}))); 
        
           }

divyanshsinghvi · 2019-10-30T19:35:54Z

I think it should be sufficient to provide an empty override for the reset() function, because I believe not overriding it would cause InstanceNormImpl to become an abstract class, and we cannot create objects of abstract classes.

Sorry I might be wrong in my understanding here. But since we are using BatchNormImplBase to subclass from, it would have already overridden the reset function. Hence, InstanceNormImpl shouldn't be an abstract class due to this. Though, it is likely the case that we would not like the function of reset defined in BatchNormImplBase getting called and hence we should redefine it

yf225 · 2019-11-01T00:43:50Z

@divyanshsinghvi Thanks a lot for the investigation! Yes I think you are right and we don't need to override reset in InstanceNormImpl, because it's already overridden in BatchNormImplBase.

yf225

@divyanshsinghvi Thanks so much for the awesome work! This work is highly complex and thanks so much for your help on this. I left some comments that we can work through together.

yf225 · 2019-11-01T00:46:10Z

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

+
+/// Base class for all (dimension-specialized) instanceNorm modules
+template <size_t D, typename Derived, typename BatchNormDerived>
+class TORCH_API InstanceNormImpl : public torch::nn::BatchNormImplBase<D, BatchNormDerived> {


I suspect that we can do

template <size_t D, typename Derived> class TORCH_API InstanceNormImpl : public torch::nn::BatchNormImplBase<D, Derived> {

yf225 · 2019-11-01T00:47:06Z

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

+  InstanceNormOptions options;
+};
+
+class TORCH_API InstanceNorm1dImpl : public InstanceNormImpl<1, InstanceNorm1dImpl, BatchNorm1dImpl> {


and then here we can write

class TORCH_API InstanceNorm1dImpl : public InstanceNormImpl<1, InstanceNorm1dImpl> {

yf225 · 2019-11-01T00:49:42Z

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

+class TORCH_API InstanceNorm1dImpl : public InstanceNormImpl<1, InstanceNorm1dImpl, BatchNorm1dImpl> {
+ public:
+  using InstanceNormImpl<1, InstanceNorm1dImpl, BatchNorm1dImpl>::InstanceNormImpl;
+ private:


Is it correct that this will prevent users from subclassing InstanceNorm1dImpl and overriding _check_input_dim in their custom subclass? If so I think we should make it protected instead.

Yes, right.

yf225 · 2019-11-01T00:51:35Z

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

+
+void InstanceNorm1dImpl::_check_input_dim(const Tensor& input) {
+  TORCH_CHECK( 
+      input.dim() == 2, 


I believe this should be

Suggested change

input.dim() == 2,

input.dim() != 2,

because TORCH_CHECK prints the message when the condition is false :D

yf225 · 2019-11-01T00:52:22Z

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

+      "(1, N * C, ...) from (N, C,...) and this makes",                                                                                                                   
+      "variances 0.");
+  TORCH_CHECK(
+      input.dim() != 3, 


I believe this should be

Suggested change

input.dim() != 3,

input.dim() == 3,

because TORCH_CHECK prints the message when the condition is false :D

yf225 · 2019-11-01T00:53:05Z

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

+      "variances 0.");
+  TORCH_CHECK(
+      input.dim() != 3, 
+      "expected 3D input (got", input.dim(), "D input)");  


nit:

Suggested change

"expected 3D input (got", input.dim(), "D input)");

"expected 3D input (got ", input.dim(), "D input)");

yf225 · 2019-11-01T00:59:53Z

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

+
+template <size_t D, typename Derived, typename BatchNormDerived>
+InstanceNormImpl<D, Derived, BatchNormDerived>::InstanceNormImpl(const InstanceNormOptions& options_)
+    : BatchNormImplBase<D, BatchNormDerived>(BatchNormOptions(options_.num_features())),


I believe we need to write

Suggested change

: BatchNormImplBase<D, BatchNormDerived>(BatchNormOptions(options_.num_features())),

: BatchNormImplBase<D, BatchNormDerived>(BatchNormOptions(options_.num_features()).eps(options.eps()).momentum(options.momentum()).affine(options.affine()).track_running_stats(options.track_running_stats())),

otherwise some of the InstanceNormOptions parameters are not passed to the BatchNormImplBase constructor :D

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

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

facebook-github-bot

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

yf225

@divyanshsinghvi Thanks a lot for the awesome work!

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-11-20T14:09:35Z

@yf225 merged this pull request in ec52d91.

divyanshsinghvi added 4 commits October 28, 2019 13:55

C++ API PArity::InstanceNorm{1,2,3}d

3798228

Revert to previous: Current Error (constructor for BatchNorm)

fbe029c

InstanceNorm Derived from BatchNorm

2db0f32

Removed D parameter from template.

8da1c3a

Tests need to be added

divyanshsinghvi requested review from ebetica, goldsborough and yf225 as code owners October 28, 2019 17:05

ssnl changed the title ~~Instance norm #25883~~ [cpp api] Instance norm #25883 Oct 28, 2019

divyanshsinghvi changed the title ~~[cpp api] Instance norm #25883~~ [cpp api] Instance norm (#25883) Oct 28, 2019

Changed _check_input_dim to Pure Virtual Function

76a77d2

yf225 mentioned this pull request Oct 29, 2019

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

Open

yf225 reviewed Oct 29, 2019

View reviewed changes

divyanshsinghvi and others added 2 commits October 30, 2019 01:56

Aesthetic changes

5c0253c

Merge branch 'master' into instanceNorm

5647cf6

InstanceNormImpl Derived from BatchNormImplBase

7b5fbd2

yf225 reviewed Nov 1, 2019

View reviewed changes

divyanshsinghvi and others added 7 commits November 11, 2019 22:51

InstanceNormImpl CRTP Fixed with proper Derived Class

020c530

Merge branch 'master' into instanceNorm

a9069ff

update

94296ee

update

61157d8

update

6c3cdaf

change

3c824cf

change

4ab31c1

yf225 force-pushed the instanceNorm branch 9 times, most recently from 8dda49e to caa5ecc Compare November 19, 2019 06:16

redesign class structure

b97427e

yf225 force-pushed the instanceNorm branch from caa5ecc to b97427e Compare November 19, 2019 06:19

Will Feng added 2 commits November 19, 2019 01:26

fix test

f2315e9

fix test

9ebf8df

facebook-github-bot reviewed Nov 19, 2019

View reviewed changes

yf225 approved these changes Nov 19, 2019

View reviewed changes

Will Feng added 4 commits November 19, 2019 12:36

fix ubsan

6b6c29a

debug

6d4d78e

fix test

ba63529

fix test

7af0202

facebook-github-bot reviewed Nov 19, 2019

View reviewed changes

Will Feng added 2 commits November 19, 2019 15:33

try to fix ubsan

1efa855

fix test

7098426

facebook-github-bot reviewed Nov 19, 2019

View reviewed changes

facebook-github-bot closed this in ec52d91 Nov 20, 2019

facebook-github-bot added the merged label Nov 20, 2019

yf225 added the module: cpp Related to C++ API label Nov 20, 2019

meganset mentioned this pull request Dec 3, 2019

nn/modules/batchnorm.h redefines "F" #30682

Closed

mruberry added the Merged label Oct 28, 2020

	/// Base class for all (dimesnsion-specialized) instanceNorm modules
	/// Base class for all (dimension-specialized) InstanceNorm modules


		Tensor forward(const Tensor& input);

		/// The optons with which this `Module` was constructed.

	/// The optons with which this `Module` was constructed.
	/// The options with which this `Module` was constructed.

	"expected 3D input (got", input.dim(), "D input)");
	"expected 3D input (got ", input.dim(), "D input)");

	: BatchNormImplBase<D, BatchNormDerived>(BatchNormOptions(options_.num_features())),
	: BatchNormImplBase<D, BatchNormDerived>(BatchNormOptions(options_.num_features()).eps(options.eps()).momentum(options.momentum()).affine(options.affine()).track_running_stats(options.track_running_stats())),

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

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

ssnl commented Oct 28, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yf225 commented Oct 29, 2019

Uh oh!

yf225 commented Oct 30, 2019

Uh oh!

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

Uh oh!

Uh oh!

yf225 commented Oct 30, 2019

Uh oh!

divyanshsinghvi commented Oct 30, 2019

Uh oh!

yf225 commented Nov 1, 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!

Uh oh!

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 Nov 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

divyanshsinghvi commented Oct 28, 2019 •

edited

Loading

yf225 commented Oct 30, 2019 •

edited

Loading