[NNVM] Fix grads for sum and expand_like #1455

sgrechanik-h · 2018-07-19T14:36:02Z

Fix gradients for the sum operation when the result is a scalar and when keepdims=True.
~~Add a function for testing gradients by comparing them with numerically computed gradients. It is used to check gradients for the sum operation.~~

sgrechanik-h · 2018-07-19T15:54:12Z

Strange, looks like the rounding function works differently on gpu and cpu, and I'm not sure if this is connected to the changes or just a coincidence. Is it possible to rerun the tests?

tqchen · 2018-07-19T17:25:30Z

@kazum @kevinthesun @srkreddy1238 can you please review this PR?

kevinthesun · 2018-07-20T04:51:22Z

nnvm/python/nnvm/testing/gradient.py

+from ..import symbol
+from ..import graph
+
+def check_gradients_numeric(y, inputs=None, dtype='float32', print_graph=False,


Do we really need to implement another gradient test interface? Since there is backward interface to test operator gradient, can we reuse or enhance that interface?

Do you mean the helper function from test_top_level1.py? Yes, I think it's a good idea to move this helper function into nnvm.testing and improve it with the ability to compute gradients numerically. I'll try to do this in a couple of days.

Yes. That would be great.

nishi-t · 2018-07-20T06:30:55Z

#1460 may be able to fix the ci error

kazum · 2018-07-22T21:00:11Z

nnvm/src/top/tensor/reduce.cc

    return std::vector<NodeEntry>{
      MakeNode("expand_like", n->attrs.name + "_grad",
               {ograds[0], n->inputs[0]},
               {{"axis", axis.str()},
-                {"exclude", std::to_string(param.exclude)}})
+                {"keepdims", std::to_string(param.keepdims)},


Passing keepdims to expand_like looks a bit strange since expand_like is not a reduce operation actually. Whether the parameter is specified or not, the output dimension does not changed.

An alternative I came up with was squeezing the output of sum beforehand like as follows. This might be less efficient, but looks easier to understand what we want to do.

if (param.keepdims) { NodeEntry squeezed = MakeNode("squeeze", n->attrs.name + "_grad_sqz", {ograds[0]}, {{"axis", axis.str()}}); return std::vector<NodeEntry>{ MakeNode("expand_like", n->attrs.name + "_grad", {squeezed, n->inputs[0]}, {{"axis", axis.str()}, {"exclude", std::to_string(exclude)}})}; }

Well, expand_like is not a reduction operation, but it might be considered dual to reduction: when given the same parameters as a reduction operation, it expands exactly the same dimensions as the reduction operation reduces.

Your solution needs adding an exclude parameter to the squeeze operation, otherwise it won't work when exclude=True. Another approach, probably even less efficient, but reliable, is to use any reduction operator instead of squeeze.

Well, expand_like is not a reduction operation, but it might be considered dual to reduction: when given the same parameters as a reduction operation, it expands exactly the same dimensions as the reduction operation reduces.

I'd agree that those are dual, but I think it's not necessary to have the keepdims parameter in expand operators. I'll send a comment about it later.

Your solution needs adding an exclude parameter to the squeeze operation, otherwise it won't work when exclude=True.

Having the exclude option in squeeze sounds good to me. But if you don't prefer it, I'm fine with broadcasting in expand_like. :)

kazum · 2018-07-24T03:25:20Z

nnvm/python/nnvm/top/transform.py

@@ -15,6 +15,9 @@
 @reg.register_compute("expand_like")
 def compute_expand_like(attrs, inputs, _):
    """Compute definition of expand_like"""
+    if attrs.get_bool("keepdims"):


We can replace this line with if len(inputs[0].shape) == len(inputs[1].shape):. I'd suggest removing keepdims from the expand_like arguments since it only implies that inputs[0] was reduced with keepdims=True.

Looks elegant and works, I'll go with this one.

Full exception: AttributeError: '<class 'tvm.tensor.PlaceholderOp'>' object has no attribute 'axis'

sgrechanik-h · 2018-07-26T09:34:31Z

Merging numerical gradient testing into existing testing functions turned out to be a bit more difficult than I thought, so I'll create a separate PR for it when it's finished.

Now this is just a fix for sum, expand_like and their gradients for some special cases.
@kazum @kevinthesun could you please review this again?

kazum

Added minor comments. Other parts look good to me.

kazum · 2018-07-26T19:42:59Z

nnvm/python/nnvm/top/transform.py

@@ -15,6 +15,10 @@
 @reg.register_compute("expand_like")
 def compute_expand_like(attrs, inputs, _):
    """Compute definition of expand_like"""
+    if len(inputs[0].shape) == len(inputs[1].shape):
+        # If the shape is not changed then it is just a broadcasting


not shape but dimension?

kazum · 2018-07-26T19:46:45Z

nnvm/tests/python/compiler/test_top_level4.py

@@ -391,11 +398,17 @@ def forward(x, y):

    def backward(head_grads, x, y):
        odim = len(out_shape)
+
+        keepdims = len(x.shape) == len(y.shape) 


Remove a trailing space.

kazum

Looks good to me, thanks!

tqchen · 2018-07-27T15:50:59Z

Thanks @sgrechanik-h @kazum @kevinthesun this is now merged

tqchen added the status: need review label Jul 19, 2018

kevinthesun reviewed Jul 20, 2018

View reviewed changes

kazum reviewed Jul 22, 2018

View reviewed changes

kazum reviewed Jul 24, 2018

View reviewed changes

tqchen added status: review in progress status: need update need update based on feedbacks and removed status: need review labels Jul 25, 2018

sgrechanik-h added 3 commits July 26, 2018 11:09

[NNVM,TOPI] Fix sum grads and expand_like for scalar case

8db95e0

[NNVM,TOPI] expand_like now broadcasts if ndims are equal

b39a513

[TOPI] Fix expand_like (no attribute axis error)

a9d9a73

Full exception: AttributeError: '<class 'tvm.tensor.PlaceholderOp'>' object has no attribute 'axis'

sgrechanik-h changed the title ~~[NNVM] Fix grads for sum; Grad testing by comparing with numeric gradients~~ [NNVM] Fix grads for sum and expand_like Jul 26, 2018

kazum requested changes Jul 26, 2018

View reviewed changes

Fixed comments and formatting

2f88951

kazum approved these changes Jul 27, 2018

View reviewed changes

tqchen merged commit a8ca691 into apache:master Jul 27, 2018

tqchen added status: accepted and removed status: need update need update based on feedbacks status: review in progress labels Jul 27, 2018

tqchen pushed a commit to tqchen/tvm that referenced this pull request Aug 4, 2018

[NNVM] Fix grads for sum and expand_like (apache#1455)

6931274

sergei-mironov pushed a commit to sergei-mironov/tvm that referenced this pull request Aug 8, 2018

[NNVM] Fix grads for sum and expand_like (apache#1455)

598ce65

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NNVM] Fix grads for sum and expand_like #1455

[NNVM] Fix grads for sum and expand_like #1455

sgrechanik-h commented Jul 19, 2018 •

edited

Loading

sgrechanik-h commented Jul 19, 2018

tqchen commented Jul 19, 2018

kevinthesun Jul 20, 2018 •

edited

Loading

sgrechanik-h Jul 20, 2018

kevinthesun Jul 20, 2018

nishi-t commented Jul 20, 2018

kazum Jul 22, 2018

sgrechanik-h Jul 23, 2018

kazum Jul 24, 2018

kazum Jul 24, 2018

sgrechanik-h Jul 24, 2018

sgrechanik-h commented Jul 26, 2018

kazum left a comment

kazum Jul 26, 2018

kazum Jul 26, 2018

kazum left a comment

tqchen commented Jul 27, 2018

[NNVM] Fix grads for sum and expand_like #1455

[NNVM] Fix grads for sum and expand_like #1455

Conversation

sgrechanik-h commented Jul 19, 2018 • edited Loading

sgrechanik-h commented Jul 19, 2018

tqchen commented Jul 19, 2018

kevinthesun Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nishi-t commented Jul 20, 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

sgrechanik-h commented Jul 26, 2018

kazum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazum left a comment

Choose a reason for hiding this comment

tqchen commented Jul 27, 2018

sgrechanik-h commented Jul 19, 2018 •

edited

Loading

kevinthesun Jul 20, 2018 •

edited

Loading