You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2020/01/15 09:23:14 UTC
[GitHub] [incubator-mxnet] TaoLv opened a new pull request #17318: [WIP]
Enable MKL-DNN FullyConnected backward
TaoLv opened a new pull request #17318: [WIP] Enable MKL-DNN FullyConnected backward
URL: https://github.com/apache/incubator-mxnet/pull/17318
## Description ##
Discussed with @rongzha1 offline. This PR incorporates the code changes in #16890 which was reverted previously.
## Checklist ##
### Essentials ###
Please feel free to remove inapplicable items for your PR.
- [ ] The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant [JIRA issue](https://issues.apache.org/jira/projects/MXNET/issues) created (except PRs with tiny changes)
- [ ] Changes are complete (i.e. I finished coding on this PR)
- [ ] All changes have test coverage:
- Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
- Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
- Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
- [ ] Code is well-documented:
- For user-facing API changes, API doc string has been updated.
- For new C++ functions in header files, their functionalities and arguments are documented.
- For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
- Check the API doc at https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
- [ ] To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change
### Changes ###
- [ ] Feature1, tests, (and when applicable, API doc)
- [ ] Feature2, tests, (and when applicable, API doc)
## Comments ##
- If this change is a backward incompatible change, why must this change be made.
- Interesting edge cases to note here
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] ciyongch commented on a change in pull request
#17318: Enable MKL-DNN FullyConnected backward
Posted by GitBox <gi...@apache.org>.
ciyongch commented on a change in pull request #17318: Enable MKL-DNN FullyConnected backward
URL: https://github.com/apache/incubator-mxnet/pull/17318#discussion_r379387996
##########
File path: tests/cpp/include/test_mkldnn.h
##########
@@ -63,24 +63,24 @@ struct TestArrayShapes {
};
// Init arrays with the default layout.
-inline static void InitDefaultArray(NDArray *arr, bool is_rand = false) {
+inline static void InitDefaultArray(NDArray *arr, bool is_rand = false, int max = 50) {
const TBlob &blob = arr->data();
mshadow::default_real_t *data = blob.dptr<mshadow::default_real_t>();
int size = blob.Size();
for (int i = 0; i < size; i++)
if (is_rand) {
- data[i] = (std::rand() % 100) - 50;
+ data[i] = (std::rand() % (max * 2)) - max;
Review comment:
How about change to `data[i] = std::rand() * 1.0 / RAND_MAX - 0.5;`? As `max = 1` will only generate two values: -1.0and 0.0 .
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] TaoLv commented on a change in pull request
#17318: Enable MKL-DNN FullyConnected backward
Posted by GitBox <gi...@apache.org>.
TaoLv commented on a change in pull request #17318: Enable MKL-DNN FullyConnected backward
URL: https://github.com/apache/incubator-mxnet/pull/17318#discussion_r379959402
##########
File path: tests/cpp/include/test_mkldnn.h
##########
@@ -63,24 +63,24 @@ struct TestArrayShapes {
};
// Init arrays with the default layout.
-inline static void InitDefaultArray(NDArray *arr, bool is_rand = false) {
+inline static void InitDefaultArray(NDArray *arr, bool is_rand = false, int max = 50) {
const TBlob &blob = arr->data();
mshadow::default_real_t *data = blob.dptr<mshadow::default_real_t>();
int size = blob.Size();
for (int i = 0; i < size; i++)
if (is_rand) {
- data[i] = (std::rand() % 100) - 50;
+ data[i] = (std::rand() % (max * 2)) - max;
Review comment:
@ciyongch Failed to do so. There are cases doing bit correction check. It seems we cannot pass the tests with these floating numbers.
http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/mxnet-validation%2Funix-gpu/detail/PR-17318/17/pipeline/
https://github.com/apache/incubator-mxnet/blob/master/tests/cpp/include/test_mkldnn.h#L605
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] TaoLv commented on a change in pull request
#17318: Enable MKL-DNN FullyConnected backward
Posted by GitBox <gi...@apache.org>.
TaoLv commented on a change in pull request #17318: Enable MKL-DNN FullyConnected backward
URL: https://github.com/apache/incubator-mxnet/pull/17318#discussion_r379465354
##########
File path: tests/cpp/include/test_mkldnn.h
##########
@@ -63,24 +63,24 @@ struct TestArrayShapes {
};
// Init arrays with the default layout.
-inline static void InitDefaultArray(NDArray *arr, bool is_rand = false) {
+inline static void InitDefaultArray(NDArray *arr, bool is_rand = false, int max = 50) {
const TBlob &blob = arr->data();
mshadow::default_real_t *data = blob.dptr<mshadow::default_real_t>();
int size = blob.Size();
for (int i = 0; i < size; i++)
if (is_rand) {
- data[i] = (std::rand() % 100) - 50;
+ data[i] = (std::rand() % (max * 2)) - max;
Review comment:
Because I don't want to affect other test case which still use the default max=50 to generate integers in [50, 50). But For the FullyConnectedOp, I want to generate relative small numbers. With the given code, the elements will be -1 and 0. Any suggestion?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] ciyongch commented on a change in pull request
#17318: Enable MKL-DNN FullyConnected backward
Posted by GitBox <gi...@apache.org>.
ciyongch commented on a change in pull request #17318: Enable MKL-DNN FullyConnected backward
URL: https://github.com/apache/incubator-mxnet/pull/17318#discussion_r379900144
##########
File path: tests/cpp/include/test_mkldnn.h
##########
@@ -63,24 +63,24 @@ struct TestArrayShapes {
};
// Init arrays with the default layout.
-inline static void InitDefaultArray(NDArray *arr, bool is_rand = false) {
+inline static void InitDefaultArray(NDArray *arr, bool is_rand = false, int max = 50) {
const TBlob &blob = arr->data();
mshadow::default_real_t *data = blob.dptr<mshadow::default_real_t>();
int size = blob.Size();
for (int i = 0; i < size; i++)
if (is_rand) {
- data[i] = (std::rand() % 100) - 50;
+ data[i] = (std::rand() % (max * 2)) - max;
Review comment:
I've no idea about why the range was set to [-50, 50) previously, and I can't figure out any specific reasons to use this range for the tests (any upper bound test?). It'll be great if you have any background for it.
But anyway, the tensors with only two values (-1 and 0, 50% are 0) might not be a good candidate for the tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] pengzhao-intel commented on a change in pull
request #17318: Enable MKL-DNN FullyConnected backward
Posted by GitBox <gi...@apache.org>.
pengzhao-intel commented on a change in pull request #17318: Enable MKL-DNN FullyConnected backward
URL: https://github.com/apache/incubator-mxnet/pull/17318#discussion_r379349568
##########
File path: tests/cpp/include/test_mkldnn.h
##########
@@ -330,7 +330,7 @@ inline void PrintVerifyMsg(const NDArrayAttrs &arr1, const NDArrayAttrs &arr2) {
*/
inline std::vector<NDArrayAttrs> GetTestInputArrays(
int types = ArrayTypes::All, bool rand = false,
- std::vector<float> scale = {1}, bool spatial_data_format = false) {
+ std::vector<float> scale = {1}, bool spatial_data_format = false, int max = 50) {
Review comment:
Is the new parameter for better usability?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] pengzhao-intel merged pull request #17318: Enable
MKL-DNN FullyConnected backward
Posted by GitBox <gi...@apache.org>.
pengzhao-intel merged pull request #17318: Enable MKL-DNN FullyConnected backward
URL: https://github.com/apache/incubator-mxnet/pull/17318
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] ciyongch commented on a change in pull request
#17318: Enable MKL-DNN FullyConnected backward
Posted by GitBox <gi...@apache.org>.
ciyongch commented on a change in pull request #17318: Enable MKL-DNN FullyConnected backward
URL: https://github.com/apache/incubator-mxnet/pull/17318#discussion_r379389375
##########
File path: tests/cpp/include/test_mkldnn.h
##########
@@ -63,24 +63,24 @@ struct TestArrayShapes {
};
// Init arrays with the default layout.
-inline static void InitDefaultArray(NDArray *arr, bool is_rand = false) {
+inline static void InitDefaultArray(NDArray *arr, bool is_rand = false, int max = 50) {
const TBlob &blob = arr->data();
mshadow::default_real_t *data = blob.dptr<mshadow::default_real_t>();
int size = blob.Size();
for (int i = 0; i < size; i++)
if (is_rand) {
- data[i] = (std::rand() % 100) - 50;
+ data[i] = (std::rand() % (max * 2)) - max;
} else {
- data[i] = i % 100 - 50;
+ data[i] = i % (max * 2) - max;
Review comment:
Same as above, how about change to something like `data[i] = i * 2.0 / size - 1.0` to generate [-1.0, 1.0)?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] ciyongch commented on a change in pull request
#17318: Enable MKL-DNN FullyConnected backward
Posted by GitBox <gi...@apache.org>.
ciyongch commented on a change in pull request #17318: Enable MKL-DNN FullyConnected backward
URL: https://github.com/apache/incubator-mxnet/pull/17318#discussion_r379962028
##########
File path: tests/cpp/include/test_mkldnn.h
##########
@@ -63,24 +63,24 @@ struct TestArrayShapes {
};
// Init arrays with the default layout.
-inline static void InitDefaultArray(NDArray *arr, bool is_rand = false) {
+inline static void InitDefaultArray(NDArray *arr, bool is_rand = false, int max = 50) {
const TBlob &blob = arr->data();
mshadow::default_real_t *data = blob.dptr<mshadow::default_real_t>();
int size = blob.Size();
for (int i = 0; i < size; i++)
if (is_rand) {
- data[i] = (std::rand() % 100) - 50;
+ data[i] = (std::rand() % (max * 2)) - max;
Review comment:
With `memcmp` to check the results, then the only choice is integer numbers. Is it reasonable to check the results by `AssertEqual` within a small enough threshold like 1e-6, then we can keep the floating number with better distribution?
Or we can just increase `max` to filling more different numbers other than only -1 and 0.
What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] TaoLv commented on a change in pull request
#17318: Enable MKL-DNN FullyConnected backward
Posted by GitBox <gi...@apache.org>.
TaoLv commented on a change in pull request #17318: Enable MKL-DNN FullyConnected backward
URL: https://github.com/apache/incubator-mxnet/pull/17318#discussion_r379465641
##########
File path: tests/cpp/include/test_mkldnn.h
##########
@@ -330,7 +330,7 @@ inline void PrintVerifyMsg(const NDArrayAttrs &arr1, const NDArrayAttrs &arr2) {
*/
inline std::vector<NDArrayAttrs> GetTestInputArrays(
int types = ArrayTypes::All, bool rand = false,
- std::vector<float> scale = {1}, bool spatial_data_format = false) {
+ std::vector<float> scale = {1}, bool spatial_data_format = false, int max = 50) {
Review comment:
See https://github.com/apache/incubator-mxnet/pull/17318#discussion_r379465354.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] TaoLv commented on a change in pull request
#17318: Enable MKL-DNN FullyConnected backward
Posted by GitBox <gi...@apache.org>.
TaoLv commented on a change in pull request #17318: Enable MKL-DNN FullyConnected backward
URL: https://github.com/apache/incubator-mxnet/pull/17318#discussion_r379905050
##########
File path: tests/cpp/include/test_mkldnn.h
##########
@@ -63,24 +63,24 @@ struct TestArrayShapes {
};
// Init arrays with the default layout.
-inline static void InitDefaultArray(NDArray *arr, bool is_rand = false) {
+inline static void InitDefaultArray(NDArray *arr, bool is_rand = false, int max = 50) {
const TBlob &blob = arr->data();
mshadow::default_real_t *data = blob.dptr<mshadow::default_real_t>();
int size = blob.Size();
for (int i = 0; i < size; i++)
if (is_rand) {
- data[i] = (std::rand() % 100) - 50;
+ data[i] = (std::rand() % (max * 2)) - max;
Review comment:
Okay, i will change to generate float numbers in [-max, max) rather than integer numbers. Previously I thought sparse (say 50% zeros) is also a way to avoid float number accumulation error.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] ciyongch commented on a change in pull request
#17318: Enable MKL-DNN FullyConnected backward
Posted by GitBox <gi...@apache.org>.
ciyongch commented on a change in pull request #17318: Enable MKL-DNN FullyConnected backward
URL: https://github.com/apache/incubator-mxnet/pull/17318#discussion_r380416282
##########
File path: tests/cpp/include/test_mkldnn.h
##########
@@ -63,24 +63,24 @@ struct TestArrayShapes {
};
// Init arrays with the default layout.
-inline static void InitDefaultArray(NDArray *arr, bool is_rand = false) {
+inline static void InitDefaultArray(NDArray *arr, bool is_rand = false, int max = 50) {
const TBlob &blob = arr->data();
mshadow::default_real_t *data = blob.dptr<mshadow::default_real_t>();
int size = blob.Size();
for (int i = 0; i < size; i++)
if (is_rand) {
- data[i] = (std::rand() % 100) - 50;
+ data[i] = (std::rand() % (max * 2)) - max;
Review comment:
Ok, let's keep it as is for now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] TaoLv commented on issue #17318: Enable MKL-DNN
FullyConnected backward
Posted by GitBox <gi...@apache.org>.
TaoLv commented on issue #17318: Enable MKL-DNN FullyConnected backward
URL: https://github.com/apache/incubator-mxnet/pull/17318#issuecomment-586149875
This is ready for review now. I also changed the cpp test to address the FullyConnectedOp failure reported before. The main reason is the tensor shape is relative large and the max value in the tensor is 50. It will lead to float number accumulation error. @rongzha1 @ciyongch @pengzhao-intel
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] TaoLv commented on a change in pull request
#17318: Enable MKL-DNN FullyConnected backward
Posted by GitBox <gi...@apache.org>.
TaoLv commented on a change in pull request #17318: Enable MKL-DNN FullyConnected backward
URL: https://github.com/apache/incubator-mxnet/pull/17318#discussion_r380159023
##########
File path: tests/cpp/include/test_mkldnn.h
##########
@@ -63,24 +63,24 @@ struct TestArrayShapes {
};
// Init arrays with the default layout.
-inline static void InitDefaultArray(NDArray *arr, bool is_rand = false) {
+inline static void InitDefaultArray(NDArray *arr, bool is_rand = false, int max = 50) {
const TBlob &blob = arr->data();
mshadow::default_real_t *data = blob.dptr<mshadow::default_real_t>();
int size = blob.Size();
for (int i = 0; i < size; i++)
if (is_rand) {
- data[i] = (std::rand() % 100) - 50;
+ data[i] = (std::rand() % (max * 2)) - max;
Review comment:
I have reverted the changes for floating numbers. Changing ``memcmp` to `AssertEqual` is out of the scope of this PR, so I will keep it as is.
> Or we can just increase max to filling more different numbers other than only -1 and 0.
I was thinking about including number 2 into the generated tensor but found that with the given shapes, there still has chance to get error. That means for the worst case, the intermediate accumulation value will be > 2^24, so the 1 will be ignored when accumulating another 1 to it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services