Gradient check by pkuyym · Pull Request #4333 · PaddlePaddle/Paddle

pkuyym · 2017-09-22T17:54:32Z

Initial version and need refine.
fix #4282
fix #4134

pengli09

The logic LGTM. Review on certain details (see comments) from someone else is needed.

pengli09 · 2017-09-25T02:14:35Z

python/paddle/v2/framework/tests/op_test.py

+    def get_backward_op(cls, scope, op, no_grad_set):
+        backward_op = core.Operator.backward(op, no_grad_set)
+        for in_var_name in backward_op.input_vars():
+            cls.create_variable(scope, in_var_name)


The original version invokes var.get_tensor(), but it's not invoked in cls.create_variable(). Is it OK?

Have you checked this?

I'm not sure for this, maybe we can wait reviews from someone else. @jacquesqiao

Done. Always invoke get_tensor() in create_variable.

pengli09 · 2017-09-25T02:14:46Z

python/paddle/v2/framework/tests/op_test.py

+        for in_var_name in backward_op.input_vars():
+            cls.create_variable(scope, in_var_name)
+        for out_var_name in backward_op.output_vars():
+            cls.create_variable(scope, out_var_name)


pengli09 · 2017-09-25T02:19:22Z

python/paddle/v2/framework/tests/op_test.py

+        return backward_op
+
+    @classmethod
+    def _feed_var(cls, scope, var_name, value, place):


This function LGTM. But I'm not familiar with the underlining code. Review from someone else is required.

pengli09 · 2017-09-25T02:19:30Z

python/paddle/v2/framework/tests/op_test.py

+
+    @classmethod
+    def feed_input(cls, scope, op, inputs, place):
+        for in_var_name, is_dup in Operator.get_op_inputs(op.type()):


This function LGTM. But I'm not familiar with the underlining code. Review from someone else is required.

pengli09 · 2017-09-25T02:24:11Z

python/paddle/v2/framework/tests/op_test.py

+                    output_vals = np.append(output_vals, output_val)
+                # get dimension info, allocate memory
+                jacobian_matrix.resize(
+                    (tensor_size, len(output_vals)), refcheck=False)


Why refcheck=False?

This matrix is not shared memory with other object, so we can set refcheck to False safely. Otherwise the allocation will fail (python will throw exception). Please see https://docs.scipy.org/doc/numpy-1.10.0/reference/generated/numpy.ndarray.resize.html

pengli09 · 2017-09-25T02:34:03Z

python/paddle/v2/framework/tests/op_test.py

+            x_pos = origin_val + delta
+            tensor_to_check.set_float_element(i, x_pos)
+            concat_flatten_output(i, x_pos_jacobian)
+            if in_place:


I'm not sure about this line. Review of someone else is needed.

Me too, just to keep consistent with original logic.

pengli09 · 2017-09-25T02:37:23Z

python/paddle/v2/framework/tests/op_test.py

+                    for sub_var_name, _ in sub_vars:
+                        out_var_tensor = cls.get_tensor_by_name(scope,
+                                                                sub_var_name)
+                        data = np.ones(out_var_tensor.shape(), dtype=np.float64)


Is np.float64 ok for op using float32?

Since the unittest works well, I think passing np.float64 to operator with float32 is ok.

pengli09 · 2017-09-25T02:38:04Z

python/paddle/v2/framework/tests/op_test.py

+                            place=place)
+                else:
+                    out_var_tensor = cls.get_tensor_by_name(scope, out_var_name)
+                    data = np.ones(out_var_tensor.shape(), np.float64)


Is np.float64 ok for op using float32?

pengli09 · 2017-09-25T02:45:33Z

python/paddle/v2/framework/tests/op_test.py

+        for i in xrange(len(out_var_shapes)):
+            accum_size[i] = cls.dim_to_size(out_var_shapes[i][1]) + \
+                    (accum_size[i - 1] if i > 0 else 0)
+            var_shape_idx[out_var_shapes[i][0]] = i


Change out_var_shapes[i][0] to out_var_shapes[i][0].encode('utf-8') to keep consistent with line 264.

change tensor_name.encode('utf-8') to tensor_name

pengli09 · 2017-09-25T02:49:48Z

python/paddle/v2/framework/tests/op_test.py

+        out_grad_values = np.zeros(accum_size[-1], dtype=np.float64)
+        x_grad_jacobian = None
+
+        backward_op = cls.get_backward_op(scope, op, no_grad_set)


move it after line 301

I think reusing backward_op is more efficient.

Then move line 300-301 before this line. no_grad_set should be invoked before cls.get_backward_op

Thanks, you are right and follow the comments.

pkuyym

Thanks and follow comments.

pkuyym · 2017-09-26T13:04:53Z

python/paddle/v2/framework/tests/op_test.py

+                    for sub_var_name, _ in sub_vars:
+                        out_var_tensor = cls.get_tensor_by_name(scope,
+                                                                sub_var_name)
+                        data = np.ones(out_var_tensor.shape(), dtype=np.float64)


Since the unittest works well, I think passing np.float64 to operator with float32 is ok.

pkuyym · 2017-09-26T13:10:24Z

python/paddle/v2/framework/tests/op_test.py

+        for i in xrange(len(out_var_shapes)):
+            accum_size[i] = cls.dim_to_size(out_var_shapes[i][1]) + \
+                    (accum_size[i - 1] if i > 0 else 0)
+            var_shape_idx[out_var_shapes[i][0]] = i


change tensor_name.encode('utf-8') to tensor_name

pkuyym · 2017-09-26T13:11:25Z

python/paddle/v2/framework/tests/op_test.py

+            x_pos = origin_val + delta
+            tensor_to_check.set_float_element(i, x_pos)
+            concat_flatten_output(i, x_pos_jacobian)
+            if in_place:


Me too, just to keep consistent with original logic.

pkuyym · 2017-09-26T13:15:12Z

python/paddle/v2/framework/tests/op_test.py

+        out_grad_values = np.zeros(accum_size[-1], dtype=np.float64)
+        x_grad_jacobian = None
+
+        backward_op = cls.get_backward_op(scope, op, no_grad_set)


I think reusing backward_op is more efficient.

pengli09 · 2017-09-27T02:22:07Z

python/paddle/v2/framework/tests/op_test.py

+        out_grad_values = np.zeros(accum_size[-1], dtype=np.float64)
+        x_grad_jacobian = None
+
+        backward_op = cls.get_backward_op(scope, op, no_grad_set)


Then move line 300-301 before this line. no_grad_set should be invoked before cls.get_backward_op

pengli09 · 2017-09-27T02:23:39Z

python/paddle/v2/framework/tests/op_test.py

+    def get_backward_op(cls, scope, op, no_grad_set):
+        backward_op = core.Operator.backward(op, no_grad_set)
+        for in_var_name in backward_op.input_vars():
+            cls.create_variable(scope, in_var_name)


Have you checked this?

… fix-4282

pengli09

All LGTM

luotao1 · 2019-02-01T04:54:33Z

感谢您给PaddlePaddle贡献代码。由于Paddle V1/V2版本已不再维护，相关代码也已从develop分支上删除，因此关闭您的PR，欢迎您向Paddle最新版-Fluid贡献代码。
Thanks for contributing to PaddlePaddle! Since V1/V2 will not be maintained anymore, and related codes have been deleted from develop branch as well, we close this PR. Welcome to contribute to Fluid——the latest version of PaddlePaddle.

Initial version

ae97f1a

pkuyym requested review from QiJune, jacquesqiao, pengli09, qingqing01, reyoung and wangkuiyi September 22, 2017 17:55

pengli09 reviewed Sep 25, 2017

View reviewed changes

Add more test cases and do some minor optimization.

9cdd04a

pkuyym commented Sep 26, 2017

View reviewed changes

pengli09 requested changes Sep 27, 2017

View reviewed changes

pkuyym added 2 commits September 27, 2017 11:40

Follow comments.

e84217c

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

9ab1d54

… fix-4282

pengli09 reviewed Sep 27, 2017

View reviewed changes

luotao1 closed this Feb 1, 2019

Comments

Conversation

pkuyym commented Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pengli09 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkuyym left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pengli09 left a comment

Choose a reason for hiding this comment

Uh oh!

luotao1 commented Feb 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pkuyym commented Sep 22, 2017 •

edited

Loading