Test/mkl dnn act #11026

azai91 · 2018-05-22T22:31:58Z

Description

(Brief description on what this PR is about)

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 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 http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the my best 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

anirudh2290 · 2018-05-22T22:42:39Z

src/operator/quadratics_op-inl.h

@@ -0,0 +1,31 @@
+//


Is this intentional?

pengzhao-intel · 2018-05-23T02:33:23Z

@juliusshufan is mkl act test on your test plan? you can work w/ @azai91 for this test.

zheng-da · 2018-05-23T15:35:19Z

tests/cpp/operator/mkldnn.cc

+  attrs.attrs.dict.insert({"act_type", "relu"});
+  attrs.attrs.op->attr_parser(&attrs.attrs);
+  attrs.dispatches.resize(1);
+  attrs.dispatches[0] = DispatchMode::kFCompute;


you need kFComputeEx here. MKLDNN uses kFComputeEx.

zheng-da · 2018-05-23T15:36:40Z

tests/cpp/operator/mkldnn.cc

+    for (int i = 0; i < size; i++)
+        data[i] = i - shift;
+}
+


This only works for the default layout. You need a version for the MKLDNN format as well.

zheng-da · 2018-05-23T15:37:08Z

tests/cpp/operator/mkldnn.cc

  TestArrayShapes tas = GetTestArrayShapes();
  std::vector<nnvm::TShape> shapes = tas.shapes;
  std::vector<mkldnn::memory::primitive_desc> pds = tas.pds;

  std::vector<NDArray> in_arrs;
  for (auto shape : shapes) {
    in_arrs.emplace_back(shape, Context());
-    InitArray(&in_arrs.back());
+    init_fn(&in_arrs.back());


You need a version to initialize the MKLDNN NDArray as well.

zheng-da · 2018-05-23T15:37:55Z

tests/cpp/operator/mkldnn.cc

+  mshadow::default_real_t *d2 = static_cast<mshadow::default_real_t*>(blob2.dptr_);
+  EXPECT_EQ(tmp1.shape().Size(), tmp2.shape().Size());
+  for (size_t i = 0; i < tmp1.shape().Size(); i++) {
+    EXPECT_EQ(d1[i], std::abs(d2[i]));


abs? if it's negative, it should be 0.

zheng-da · 2018-05-23T15:39:24Z

tests/cpp/operator/mkldnn.cc

+  }
+}
+
+void TestOp(const OpAttrs &attrs, InitFunc init_fn, VerifyFunc verify_fn) {


The function actually can only test unary operators. It's better to have unary in the name

zheng-da · 2018-05-24T17:54:28Z

tests/cpp/operator/mkldnn.cc

  std::vector<NDArray> in_arrs;
  in_arrs.emplace_back(shape, Context());
-  InitArray(&in_arrs.back());
+  init_fn(&in_arrs.back(), false);


could you make them true as well?

zheng-da · 2018-05-24T17:54:36Z

tests/cpp/operator/mkldnn.cc


  // Get a reused version.
  nnvm::TShape s(1);
  s[0] = shape.Size();
  NDArray arr(s, Context());
  arr = arr.AsArray(shape, arr.dtype());
-  InitArray(&arr);
+  init_fn(&arr, false);


the same here

anirudh2290 · 2018-05-24T20:39:38Z

tests/cpp/operator/mkldnn.cc

  const TBlob &blob = arr->data();
  mshadow::default_real_t *data = blob.dptr<mshadow::default_real_t>();
  size_t size = blob.Size();
-  for (size_t i = 0; i < size; i++)
-    data[i] = i;
+    if (is_rand) {


is_rand inside the for loop ?

anirudh2290 · 2018-05-24T20:40:06Z

tests/cpp/operator/mkldnn.cc

  const TBlob &blob = arr->data();
  mshadow::default_real_t *data = blob.dptr<mshadow::default_real_t>();
-  size_t size = blob.Size();
+  int size = blob.Size();
  if (is_rand) {


is_rand should be inside the for loop

anirudh2290 · 2018-05-24T21:18:03Z

can you fix lint

zheng-da · 2018-05-28T03:23:16Z

src/operator/nn/mkldnn/mkldnn_sum.cc

-    } else {
-      in_mem = inputs[i].GetMKLDNNData();
-    }
+    in_mem = inputs[i].GetMKLDNNData();


why do you modify the code here?

woops realized it was not the same. reverted.

zheng-da · 2018-05-29T21:54:55Z

tests/cpp/operator/mkldnn.cc

    }
  }
 }

 TEST(IMPERATIVE, UnaryOp) {
  OpAttrs attrs = GetCopyOp();
-  TestUnaryOp(attrs, VerifyCopyResult);
+    TestUnaryOp(attrs, InitDefaultArray, VerifyCopyResult);


please fix the indent here.

* add mkl act unit test * fix operator name * use custom ndarray init * func missing param * add init fn for act operator test * remove extra white space * fix fetch relu operator * fix get relu operator name * add assert abs in verify fn * remove unused operator * cast blob ptr to float * use parsed param * use attr_parser * fix header order * update test fn name * use relu fn * add kFComputeEx dispatch * init posneg mklarray * fix generating rnd pos neg ints * output arrays are rnd generated * test that getinputarrays creates view and mkldnn arrays * add more output types * fix typo * fix gettestput test * create arrattr struct to display arr info * refactor initarray * print arr description in verify fn * use long int string interpolation * fix alias params * iterate over dims * print c_str * print output info * improve print message * improve print * fix new lines in output * refactor print messages * fix typos * fix lint issues * fix rebase * pass ndarray as ptr * store copy of ndarray in attrs obj * fix rem inits * fix dispatch size * move print earlier * use createmkldnnmem helper fun * fix lint * refactor if else statement * use buffer ndarray * fix spacing * fix refactor * revert sum refactor * use fallback compute * fix typo * fix lint * use fallbackcompute fn for act operator * convert activation impl funcs to fxcompute std * remove unused var * move unused variable * fix indent

anirudh2290 reviewed May 22, 2018

View reviewed changes

src/operator/quadratics_op-inl.h Outdated

@@ -0,0 +1,31 @@

//

Copy link

Member

anirudh2290 May 22, 2018

Choose a reason for hiding this comment

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

Is this intentional?

azai91 force-pushed the test/mkl-dnn-act branch 2 times, most recently from 13b7a65 to fd39a23 Compare May 22, 2018 23:22

azai91 force-pushed the test/mkl-dnn-act branch 2 times, most recently from 119a21e to 857c3d4 Compare May 23, 2018 04:17

zheng-da reviewed May 23, 2018

View reviewed changes

azai91 force-pushed the test/mkl-dnn-act branch 8 times, most recently from c987bca to 2d24c60 Compare May 24, 2018 16:29

zheng-da reviewed May 24, 2018

View reviewed changes

azai91 force-pushed the test/mkl-dnn-act branch from 2d24c60 to e3d553b Compare May 24, 2018 20:10

anirudh2290 reviewed May 24, 2018

View reviewed changes

azai91 force-pushed the test/mkl-dnn-act branch from cf99f67 to 990eaf5 Compare May 25, 2018 18:31

zheng-da reviewed May 28, 2018

View reviewed changes

azai91 force-pushed the test/mkl-dnn-act branch 3 times, most recently from ded9ecc to 2e0c42a Compare May 29, 2018 04:59

azai91 added 5 commits May 29, 2018 12:47

add mkl act unit test

85887db

fix operator name

9e5bcf3

use custom ndarray init

c604579

func missing param

3b6a952

add init fn for act operator test

c36ffa4

azai91 added 22 commits May 29, 2018 12:47

fix typos

4b4008e

fix lint issues

50f4f29

fix rebase

54207a8

pass ndarray as ptr

7a79e49

store copy of ndarray in attrs obj

12ed71c

fix rem inits

7435692

fix dispatch size

75cb1e0

move print earlier

5396619

use createmkldnnmem helper fun

3c71dd8

fix lint

897a687

refactor if else statement

ca6afc8

use buffer ndarray

6dbcd43

fix spacing

6fcebae

fix refactor

08d3a74

revert sum refactor

66bc279

use fallback compute

1df1cbb

fix typo

9fd5d1f

fix lint

d1126d6

use fallbackcompute fn for act operator

e0ce845

convert activation impl funcs to fxcompute std

7a28188

remove unused var

15fef2b

move unused variable

55f4e88

azai91 force-pushed the test/mkl-dnn-act branch from 2e0c42a to 55f4e88 Compare May 29, 2018 19:47

zheng-da reviewed May 29, 2018

View reviewed changes

fix indent

3b97056

zheng-da approved these changes May 30, 2018

View reviewed changes

piiswrong merged commit 92286c9 into apache:master May 30, 2018

pengzhao-intel mentioned this pull request Jun 13, 2018

[MXNET-497] fix bugs in MKLDNN operators to handle the kAddTo request #11129

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test/mkl dnn act #11026

Test/mkl dnn act #11026

azai91 commented May 22, 2018

anirudh2290 May 22, 2018

pengzhao-intel commented May 23, 2018

zheng-da May 23, 2018

zheng-da May 23, 2018

zheng-da May 23, 2018

zheng-da May 23, 2018

zheng-da May 23, 2018

zheng-da May 24, 2018

zheng-da May 24, 2018

anirudh2290 May 24, 2018

anirudh2290 May 24, 2018

anirudh2290 commented May 24, 2018

zheng-da May 28, 2018

azai91 May 28, 2018

zheng-da May 29, 2018

Test/mkl dnn act #11026

Test/mkl dnn act #11026

Conversation

azai91 commented May 22, 2018

Description

Checklist

Essentials

Changes

Comments

Choose a reason for hiding this comment

pengzhao-intel commented May 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anirudh2290 commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment