[GitHub] haojin2 commented on a change in pull request #11656: Fix batchnorm problem with sparse matrices when fix_gamma=True
haojin2 commented on a change in pull request #11656: Fix batchnorm problem
with sparse matrices when fix_gamma=True
URL: https://github.com/apache/incubator-mxnet/pull/11656#discussion_r202203112
##
File path: src/operator/nn/batch_norm.cc
##
@@ -444,27 +445,39 @@ void BatchNormGradComputeExCPU(const nnvm::NodeAttrs
,
}
FallBackCompute(BatchNormGradCompute, attrs, ctx, inputs, req, outputs);
}
+#endif
static inline bool BatchNormStorageType(const nnvm::NodeAttrs ,
const int dev_mask,
DispatchMode *dispatch_mode,
std::vector *in_attrs,
std::vector *out_attrs) {
- CHECK_EQ(in_attrs->size(), 5);
- CHECK_EQ(out_attrs->size(), 3);
- return MKLDNNStorageType(attrs, dev_mask, true, dispatch_mode,
- in_attrs, out_attrs);
-}
+ const BatchNormParam = nnvm::get(attrs.parsed);
-static inline bool backward_BatchNormStorageType(const nnvm::NodeAttrs ,
- const int dev_mask,
- DispatchMode *dispatch_mode,
- std::vector *in_attrs,
- std::vector *out_attrs) {
- return MKLDNNStorageType(attrs, dev_mask, true, dispatch_mode,
- in_attrs, out_attrs);
-}
+ if ((common::ContainsStorageType(*in_attrs, kRowSparseStorage) ||
Review comment:
Done.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] haojin2 commented on a change in pull request #11656: Fix batchnorm problem with sparse matrices when fix_gamma=True
haojin2 commented on a change in pull request #11656: Fix batchnorm problem with sparse matrices when fix_gamma=True URL: https://github.com/apache/incubator-mxnet/pull/11656#discussion_r202167429 ## File path: src/operator/batch_norm_v1.cc ## @@ -89,6 +89,9 @@ the output. It is often used during inference. Both ``gamma`` and ``beta`` are learnable parameters. But if ``fix_gamma`` is true, then set ``gamma`` to 1 and its gradient to 0. +There's no sparse support for this operator, and will exhibit problematic behavior if used with Review comment: Done. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] haojin2 commented on a change in pull request #11656: Fix batchnorm problem with sparse matrices when fix_gamma=True
haojin2 commented on a change in pull request #11656: Fix batchnorm problem with sparse matrices when fix_gamma=True URL: https://github.com/apache/incubator-mxnet/pull/11656#discussion_r202165629 ## File path: tests/python/mkl/test_mkldnn.py ## @@ -233,7 +233,7 @@ def check_batchnorm_training(stype): mx.nd.array(beta).tostype(stype)] mean_std = [mx.nd.array(rolling_mean).tostype(stype), mx.nd.array(rolling_std).tostype(stype)] -test = mx.symbol.BatchNorm(data, fix_gamma=True) +test = mx.symbol.BatchNorm(data, fix_gamma=False) Review comment: BTW I think @anirudh2290 already approved the previous #11631 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] haojin2 commented on a change in pull request #11656: Fix batchnorm problem with sparse matrices when fix_gamma=True
haojin2 commented on a change in pull request #11656: Fix batchnorm problem with sparse matrices when fix_gamma=True URL: https://github.com/apache/incubator-mxnet/pull/11656#discussion_r202128834 ## File path: tests/python/mkl/test_mkldnn.py ## @@ -233,7 +233,7 @@ def check_batchnorm_training(stype): mx.nd.array(beta).tostype(stype)] mean_std = [mx.nd.array(rolling_mean).tostype(stype), mx.nd.array(rolling_std).tostype(stype)] -test = mx.symbol.BatchNorm(data, fix_gamma=True) +test = mx.symbol.BatchNorm(data, fix_gamma=False) Review comment: No problem. @zheng-da @eric-haibin-lin This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] haojin2 commented on a change in pull request #11656: Fix batchnorm problem with sparse matrices when fix_gamma=True
haojin2 commented on a change in pull request #11656: Fix batchnorm problem with sparse matrices when fix_gamma=True URL: https://github.com/apache/incubator-mxnet/pull/11656#discussion_r202122089 ## File path: tests/python/mkl/test_mkldnn.py ## @@ -233,7 +233,7 @@ def check_batchnorm_training(stype): mx.nd.array(beta).tostype(stype)] mean_std = [mx.nd.array(rolling_mean).tostype(stype), mx.nd.array(rolling_std).tostype(stype)] -test = mx.symbol.BatchNorm(data, fix_gamma=True) +test = mx.symbol.BatchNorm(data, fix_gamma=False) Review comment: Yes you got it, that's why we're only testing the necessary legal case here. Please merge this once you feel good to do so, thanks! This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] haojin2 commented on a change in pull request #11656: Fix batchnorm problem with sparse matrices when fix_gamma=True
haojin2 commented on a change in pull request #11656: Fix batchnorm problem with sparse matrices when fix_gamma=True URL: https://github.com/apache/incubator-mxnet/pull/11656#discussion_r202116683 ## File path: tests/python/mkl/test_mkldnn.py ## @@ -233,7 +233,7 @@ def check_batchnorm_training(stype): mx.nd.array(beta).tostype(stype)] mean_std = [mx.nd.array(rolling_mean).tostype(stype), mx.nd.array(rolling_std).tostype(stype)] -test = mx.symbol.BatchNorm(data, fix_gamma=True) +test = mx.symbol.BatchNorm(data, fix_gamma=False) Review comment: The purpose of this mkl test is to test that we can fallback to sparse matrices correctly when using MKLDNN under the legal cases for sparse matrices. The logics of exclusion of illegal cases for sparse matrices(fix_gamma=True), which is shared by both USE_MKLDNN=1 and USE_MKLDNN=0 situtaions, were already tested by the test_batchnorm_fallback test. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] haojin2 commented on a change in pull request #11656: Fix batchnorm problem with sparse matrices when fix_gamma=True
haojin2 commented on a change in pull request #11656: Fix batchnorm problem with sparse matrices when fix_gamma=True URL: https://github.com/apache/incubator-mxnet/pull/11656#discussion_r202114945 ## File path: tests/python/mkl/test_mkldnn.py ## @@ -233,7 +233,7 @@ def check_batchnorm_training(stype): mx.nd.array(beta).tostype(stype)] mean_std = [mx.nd.array(rolling_mean).tostype(stype), mx.nd.array(rolling_std).tostype(stype)] -test = mx.symbol.BatchNorm(data, fix_gamma=True) +test = mx.symbol.BatchNorm(data, fix_gamma=False) Review comment: I thought we had the conversation in Lai's PR yesterday, and I've created the related issue #11647 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
