[FRONTEND][TENSORFLOW] Add Pad and PadV2 support #1545

nishi-t · 2018-08-03T06:45:53Z

This PR adds Pad and PadV2 support for tensorflow.
@PariksheetPinjari909 @srkreddy1238 Could you please review?

srkreddy1238

@nishi-t thanks for this addition.

srkreddy1238 · 2018-08-03T07:17:46Z

nnvm/python/nnvm/frontend/tensorflow.py

@@ -669,6 +669,22 @@ def _impl(inputs, in_state_c, in_state_h, attr, params):
    return _impl


+def _pad(name):
+    def _impl(inputs, attr, params):
+        padlist = params.pop(inputs[1].list_output_names()[0]).asnumpy()


Suggest to assert if padlist is not available in params.

Please assert for mode not equal to CONSTANT
And the mode string is case-insensitive.

Thank you for the review.
For the second pointed out, I think that _pad is called from only CONSTANT in this PR, because Pad is generated by only tf.pad with CONSTANT mode without constant_value. In more detail, tf.pad generates different operators depending on the mode and the option:

mode is SYMMETRIC or REFLECT => MirrorPad

mode is CONSTANT without constant_value => Pad

mode is CONSTANT with constant_value => PadV2

I confirmed these little strange behavior by following script:

import tensorflow as tf import nnvm def main(_): def check_pad_graph(**kwargs): print("Pad: args = ", kwargs) with tf.Graph().as_default() as graph: sess = tf.InteractiveSession() a = tf.constant([[1,2,3], [4,5,6]], name='a') padding = tf.constant([[1, 1], [2,2]]) c = tf.pad(a, padding, **kwargs) sess.run(tf.global_variables_initializer()) print(sess.run(c)) graph_def = graph.as_graph_def(add_shapes=True) pad_mode = kwargs['mode'] if pad_mode == 'CONSTANT': name = "constant" if 'constant_values' in kwargs: name += "_with_values" elif pad_mode == 'REFLECT': name = "reflect" elif pad_mode == 'SYMMETRIC': name = "symmetric" name += "_pad.pbtxt" tf.train.write_graph(graph_def, '.', name, as_text=True) check_pad_graph(mode='CONSTANT') check_pad_graph(mode='CONSTANT', constant_values=1) check_pad_graph(mode='SYMMETRIC', constant_values=1) check_pad_graph(mode='REFLECT') if __name__ == '__main__': tf.app.run()

I put the script's result on my gist:
https://gist.github.com/nishi-t/a72c477c5f07812d327a9cb7f06b923d

@nishi-t thanks for the details.
I think this PR is good to go with the other cosmetic change in testcase.

Any data about demand for MirrorPad ? we could propose additional attribute to pad operator to support mode.

@srkreddy1238 Sorry for the delay. In terms of wether I have use cases to use MirrorPad (use tf.pad with symmetric or reflect mode), I don't have any data at the moment. But, since Pad is required for resnet, I want to add Pad support for now.

To support MirrorPad, we will have to add mode option(attribute?) to pad operator (in nnvm and perhaps tvm) to distinguish behavior of padding(CONSTANT, REFLECT, SYMMETRIC) . Since MirrorPad has mode as attribute to distinguish between REFLECT and SYMMETRIC, we will be able to pass it to the pad operator.

I might have a misunderstand so please let me if we can give you any other information.

@nishi-t don't worry about MirrorPad, I just asked about it to know the requirement of it.

srkreddy1238 · 2018-08-03T07:24:10Z

nnvm/python/nnvm/frontend/tensorflow.py

+        padlist = params.pop(inputs[1].list_output_names()[0]).asnumpy()
+        paddings = tuple([tuple(l) for l in padlist])
+        attr['pad_width'] = paddings
+        attr['pad_value'] = 0


For Pad constant_value should be taken from attributes (should be zero only if not available in attributes.).

Pad doesn't have constant_value as attribute and pad_value always should be 0 for Pad. Please check my previous comment.

srkreddy1238 · 2018-08-03T07:27:21Z

nnvm/tests/python/frontend/tensorflow/test_forward.py

@@ -704,6 +704,44 @@ def test_forward_resize_bilinear():
    _test_resize_bilinear((4, 16, 32, 32), [50, 50], False)
    _test_resize_bilinear((6, 32, 64, 64), [20, 20], True)

+#######################################################################
+# Pad
+# -------


Underline up to the text size !

srkreddy1238 · 2018-08-03T07:30:33Z

nnvm/tests/python/frontend/tensorflow/test_forward.py

+        pad_values = constant_op.constant(paddings)
+        pad = tf.pad(in_data, paddings=pad_values, **kwargs)
+
+        if 'constant_values' in kwargs:


constant_values could be there for both Pad and PadV2

srkreddy1238 · 2018-08-03T14:59:36Z

nnvm/python/nnvm/frontend/tensorflow.py

@@ -669,6 +669,22 @@ def _impl(inputs, in_state_c, in_state_h, attr, params):
    return _impl


+def _pad(name):
+    def _impl(inputs, attr, params):
+        padlist = params.pop(inputs[1].list_output_names()[0]).asnumpy()


@nishi-t thanks for the details.
I think this PR is good to go with the other cosmetic change in testcase.

Any data about demand for MirrorPad ? we could propose additional attribute to pad operator to support mode.

nishi-t · 2018-08-06T08:51:54Z

@srkreddy1238 I addressed. Please review again.

srkreddy1238

LGTM.

yzhliu · 2018-08-06T21:07:34Z

Thanks for contribution and reviewing. This is merged.

* [FRONTEND][TENSORFLOW] Add Pad and PadV2 support * Add assettion to _pad, and fix testcase for pad.

srkreddy1238 requested changes Aug 3, 2018

View reviewed changes

tqchen force-pushed the master branch from e316f03 to 7b59b8e Compare August 4, 2018 17:14

nishi-t force-pushed the tf_pad branch from d4eac7c to 3d0e793 Compare August 6, 2018 02:34

[FRONTEND][TENSORFLOW] Add Pad and PadV2 support

bfe659d

nishi-t force-pushed the tf_pad branch from 3d0e793 to bfe659d Compare August 6, 2018 02:40

Add assettion to _pad, and fix testcase for pad.

8c0e19b

nishi-t force-pushed the tf_pad branch from 33d639a to 8c0e19b Compare August 6, 2018 05:21

srkreddy1238 approved these changes Aug 6, 2018

View reviewed changes

yzhliu approved these changes Aug 6, 2018

View reviewed changes

yzhliu merged commit a8574e7 into apache:master Aug 6, 2018

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

[FRONTEND][TENSORFLOW] Add Pad and PadV2 support (apache#1545)

837354b

* [FRONTEND][TENSORFLOW] Add Pad and PadV2 support * Add assettion to _pad, and fix testcase for pad.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FRONTEND][TENSORFLOW] Add Pad and PadV2 support #1545

[FRONTEND][TENSORFLOW] Add Pad and PadV2 support #1545

nishi-t commented Aug 3, 2018

srkreddy1238 left a comment

srkreddy1238 Aug 3, 2018

srkreddy1238 Aug 3, 2018

nishi-t Aug 3, 2018 •

edited

Loading

srkreddy1238 Aug 3, 2018

nishi-t Aug 6, 2018

srkreddy1238 Aug 6, 2018

srkreddy1238 Aug 3, 2018

nishi-t Aug 3, 2018 •

edited

Loading

srkreddy1238 Aug 3, 2018

srkreddy1238 Aug 3, 2018

srkreddy1238 Aug 3, 2018

nishi-t commented Aug 6, 2018

srkreddy1238 left a comment

yzhliu commented Aug 6, 2018

[FRONTEND][TENSORFLOW] Add Pad and PadV2 support #1545

[FRONTEND][TENSORFLOW] Add Pad and PadV2 support #1545

Conversation

nishi-t commented Aug 3, 2018

srkreddy1238 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nishi-t Aug 3, 2018 • edited Loading

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

nishi-t Aug 3, 2018 • edited Loading

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

nishi-t commented Aug 6, 2018

srkreddy1238 left a comment

Choose a reason for hiding this comment

yzhliu commented Aug 6, 2018

nishi-t Aug 3, 2018 •

edited

Loading

nishi-t Aug 3, 2018 •

edited

Loading