[MXNET-92] Support float16 in L2Normalization operator #10078

haojin2 · 2018-03-12T20:13:34Z

Description

Add support for any datatype for L2Normalization operator mentioned in Issue #2302.

Checklist

Essentials

Passed code style checking (make lint)
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)
Code is well-documented:
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

Change L2Normalization operator from only supporting real_t to supporting any datatype
Add additional test cases for float16

cjolivier01 · 2018-03-12T20:30:25Z

src/operator/l2_normalization-inl.h

@@ -294,7 +321,13 @@ class L2NormalizationProp : public OperatorProperty {
    return {ResourceRequest::kTempSpace};
  }

-  Operator* CreateOperator(Context ctx) const override;
+  Operator* CreateOperator(Context ctx) const override {


Does something still call this?

Honestly I'm not really sure, with a simple grep for "CreateOperator" in src only this usage appeared:
nnvm/legacy_op_util.cc:297: return OpStatePtr::Create(prop.ptr->CreateOperatorEx(ctx, &is, &it),

Ok, I see it is masked by your override of CreateOperatorEx()

cjolivier01 · 2018-03-12T20:46:39Z

src/operator/l2_normalization-inl.h

@@ -294,7 +321,13 @@ class L2NormalizationProp : public OperatorProperty {
    return {ResourceRequest::kTempSpace};
  }

-  Operator* CreateOperator(Context ctx) const override;
+  Operator* CreateOperator(Context ctx) const override {


Ok, I see it is masked by your override of CreateOperatorEx()

cjolivier01 · 2018-03-12T20:47:31Z

src/operator/l2_normalization.cc

-  DO_BIND_DISPATCH(CreateOp, param_);
+Operator* L2NormalizationProp::CreateOperatorEx(Context ctx, std::vector<TShape> *in_shape,
+                                                std::vector<int> *in_type) const {
+  DO_BIND_DISPATCH(CreateOp, param_, in_type->at(0));


Since you're overriding CreateOperatorEx(), then what ends up calling InferShape(), InferType(), which is normally done by the base class' CreateOperatorEx()?

I see, just added calls to InferType and InferShape to the code, the PR will be updated soon.

Just FYI, usually, DType is determined within the Forward() and Backward() functions using the type switch from the actual input blob at runtime.

I am not saying you need to change it, but if that were the case, you wouldn;t have to override CreateOpEx(), which has nontrivial logic.

Where is InferShape(), InferType() being called?

haojin2 · 2018-03-13T04:06:42Z

I think this PR should be ready for merge, @rahul003 would you please take a look at it to double-check? Thanks!

cjolivier01 · 2018-03-13T04:14:33Z

src/operator/l2_normalization.cc

@@ -26,13 +26,22 @@
 namespace mxnet {
 namespace op {
 template<>
-Operator* CreateOp<cpu>(L2NormalizationParam param) {
-  return new L2NormalizationOp<cpu>(param);
+Operator* CreateOp<cpu>(L2NormalizationParam param, int dtype) {


is it done this way elsewhere?

https://github.com/apache/incubator-mxnet/pull/3011/files

piiswrong · 2018-03-13T10:40:37Z

src/operator/l2_normalization.cc

-  DO_BIND_DISPATCH(CreateOp, param_);
+Operator* L2NormalizationProp::CreateOperatorEx(Context ctx, std::vector<TShape> *in_shape,
+                                                std::vector<int> *in_type) const {
+  std::vector<TShape> out_shape, aux_shape;


these checks are not necessary

Do you mean the checks for InferType and InferShape?

cjolivier01 · 2018-03-13T18:01:20Z

Please add a JIRA ticket

reminisce · 2018-03-13T18:17:31Z

src/operator/l2_normalization-inl.h

+                 std::vector<int> *aux_type) const override {
+    CHECK_EQ(in_type->size(), 1U);
+    int dtype = (*in_type)[0];
+    CHECK_NE(dtype, -1) << "Input must have specified type";


Please use mutual inference instead of terminating the program.

cjolivier01 · 2018-03-13T18:36:44Z

JIRA: https://issues.apache.org/jira/browse/MXNET-92

anirudh2290 · 2018-03-13T18:54:08Z

tests/python/unittest/test_operator.py

@@ -2396,21 +2396,22 @@ def check_l2_normalization(in_shape, mode, norm_eps=1e-10):
    exe = out.simple_bind(ctx=ctx, data=in_data.shape)
    output = exe.forward(is_train=True, data=in_data)
    # compare numpy + mxnet
-    assert_almost_equal(exe.outputs[0].asnumpy(), np_out, rtol=1e-5)
+    assert_almost_equal(exe.outputs[0].asnumpy(), np_out, rtol=1e-2 if dtype is 'float16' else 1e-5)


can you also pass atol here. Default is 1e-20 which may result in test becoming flaky if the numbers are small.

haojin2 · 2018-03-19T17:56:02Z

This PR should be good for merge, @cjolivier01 @piiswrong @anirudh2290 @reminisce @rahul003, would you please take another look at this to see if this is good to go through?

anirudh2290 · 2018-03-20T17:28:38Z

tests/python/unittest/test_operator.py

@@ -2397,21 +2397,22 @@ def check_l2_normalization(in_shape, mode, norm_eps=1e-10):
    exe = out.simple_bind(ctx=ctx, data=in_data.shape)
    output = exe.forward(is_train=True, data=in_data)
    # compare numpy + mxnet
-    assert_almost_equal(exe.outputs[0].asnumpy(), np_out, rtol=1e-5)
+    assert_almost_equal(exe.outputs[0].asnumpy(), np_out, rtol=1e-2 if dtype is 'float16' else 1e-5, atol=1e-20)


default is 1e-20 can you make atol bigger than this number maybe 1e-5 ?

* enable other dtype in l2 normalization * Get rid of older code * address code reviews: get rid of unnecessary checks * address code reviews * fix buggy InferType in L2Normalization * address code review: change atol

haojin2 requested a review from cjolivier01 as a code owner March 12, 2018 20:13

cjolivier01 reviewed Mar 12, 2018

View reviewed changes

haojin2 force-pushed the master branch 2 times, most recently from b86f6d7 to a60d44b Compare March 12, 2018 23:07

cjolivier01 reviewed Mar 13, 2018

View reviewed changes

piiswrong reviewed Mar 13, 2018

View reviewed changes

haojin2 force-pushed the master branch from 34b930b to 98a4236 Compare March 13, 2018 16:56

reminisce reviewed Mar 13, 2018

View reviewed changes

cjolivier01 changed the title ~~Support float16 in L2Normalization operator~~ [MXNET-92] Support float16 in L2Normalization operator Mar 13, 2018

anirudh2290 reviewed Mar 13, 2018

View reviewed changes

haojin2 force-pushed the master branch 3 times, most recently from b2296ae to 74a2fee Compare March 16, 2018 19:55

haojin2 force-pushed the master branch from 74a2fee to 3e0d880 Compare March 19, 2018 23:25

anirudh2290 reviewed Mar 20, 2018

View reviewed changes

haojin2 and others added 6 commits March 20, 2018 17:32

enable other dtype in l2 normalization

a4bbfe1

Get rid of older code

d313aab

address code reviews: get rid of unnecessary checks

c402b56

address code reviews

57646cb

fix buggy InferType in L2Normalization

8a7ccb5

address code review: change atol

88effef

haojin2 force-pushed the master branch from 3e0d880 to 88effef Compare March 20, 2018 17:33

piiswrong merged commit 1b71ce1 into apache:master Mar 20, 2018

haojin2 mentioned this pull request Mar 20, 2018

[MXNET-101] Support float16 in LeakyReLU operator #10169

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MXNET-92] Support float16 in L2Normalization operator #10078

[MXNET-92] Support float16 in L2Normalization operator #10078

haojin2 commented Mar 12, 2018 •

edited

cjolivier01 Mar 12, 2018

haojin2 Mar 12, 2018 •

edited

cjolivier01 Mar 12, 2018

cjolivier01 Mar 12, 2018

cjolivier01 Mar 12, 2018

haojin2 Mar 12, 2018

cjolivier01 Mar 12, 2018

cjolivier01 Mar 12, 2018

cjolivier01 Mar 20, 2018

haojin2 commented Mar 13, 2018

cjolivier01 Mar 13, 2018

haojin2 Mar 13, 2018

cjolivier01 Mar 13, 2018

piiswrong Mar 13, 2018

haojin2 Mar 13, 2018

cjolivier01 commented Mar 13, 2018

reminisce Mar 13, 2018

haojin2 Mar 13, 2018

cjolivier01 commented Mar 13, 2018

anirudh2290 Mar 13, 2018

haojin2 Mar 13, 2018

haojin2 commented Mar 19, 2018

anirudh2290 Mar 20, 2018

haojin2 Mar 20, 2018

[MXNET-92] Support float16 in L2Normalization operator #10078

[MXNET-92] Support float16 in L2Normalization operator #10078

Conversation

haojin2 commented Mar 12, 2018 • edited

Description

Checklist

Essentials

Changes

Choose a reason for hiding this comment

haojin2 Mar 12, 2018 • edited

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

haojin2 commented Mar 13, 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

cjolivier01 commented Mar 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjolivier01 commented Mar 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haojin2 commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haojin2 commented Mar 12, 2018 •

edited

haojin2 Mar 12, 2018 •

edited