[Relay][Frontend][TF] Fix slice when begin or size is not Const #4372
Conversation
|
@yongwww could you take a look? |
| try: | ||
| begin = _get_list_param(params, inputs[1]) | ||
| except: | ||
| begin = _infer_value_simulated(inputs[1], params).asnumpy()[0] |
There was a problem hiding this comment.
how about begin = _infer_value(inputs[1], params).asnumpy().tolist()?
There was a problem hiding this comment.
I think _infer_value_simulated is better here. _infer_value assumes that all of the inputs leading up to inputs[1] are constant, _infer_value_simulated does not.
There was a problem hiding this comment.
I'm not sure whether _infer_value_simulated should be used here. size parameter can be a data dependent expression. If we want to infer the correct value, probably we should enforce all variables in params. For symbolic begin/size slice, it will be covered in #4312.
There was a problem hiding this comment.
_infer_value_simulated is not able to handle data dependent cases. Let's use _infer_value here, if _infer_value doesn't help, then we have to use dynamic strided_slice in pr 4312. @lsy643 let us know if _infer_value doesn't work for you
There was a problem hiding this comment.
@yongwww _infer_value works for me. It seems _infer_value_simulated use _infer_value inside, so why prefer one over another?
There was a problem hiding this comment.
_infer_value_simulated will create dummy data if some variables don't appear in params. In data dependent case, this will cause problem.
| try: | ||
| size = _get_list_param(params, inputs[2]) | ||
| except: | ||
| size = _infer_value_simulated(inputs[2], params).asnumpy()[0] |
|
|
||
|
|
||
| def test_forward_slice(): | ||
| _test_forward_slice_operation_input([1, 1], 0, 2) |
There was a problem hiding this comment.
add test cases to cover begin & size as tensor
There was a problem hiding this comment.
@yongwww begin and size as tensor are already covered. In the _test_forward_slice_operation_input([1, 1], 0, 2) , the input 0 and input 2 will be changed into tensors after tf.expand_dimes
def _test_forward_slice_operation_input(input_value, begin_value, size_value):
input_data = np.array(input_value, dtype=np.float32)
with tf.Graph().as_default():
input_tensor = tf.placeholder(
shape=input_data.shape, dtype=input_data.dtype, name="input")
begin_tensor = tf.expand_dims(begin_value, axis=0)
size_tensor = tf.expand_dims(size_value, axis=0)
slice_tensor = tf.slice(input_tensor, begin_tensor, size_tensor, name='slice_output')
compare_tf_with_tvm([input_data], ['input:0'], 'slice_output:0')
| size = _get_list_param(params, inputs[2]) | ||
| try: | ||
| begin = _get_list_param(params, inputs[1]) | ||
| except: |
There was a problem hiding this comment.
We should probably only catch the exceptions we expect to see here (like IndexError or AttributeError), and let the others go through.
| try: | ||
| begin = _get_list_param(params, inputs[1]) | ||
| except: | ||
| begin = _infer_value_simulated(inputs[1], params).asnumpy()[0] |
There was a problem hiding this comment.
I'm not sure whether _infer_value_simulated should be used here. size parameter can be a data dependent expression. If we want to infer the correct value, probably we should enforce all variables in params. For symbolic begin/size slice, it will be covered in #4312.
|
Thank you @lsy643 This is now merged. |
…he#4372) * fix slice bug when input is param * use _infer_value rather than _infer_value_simulated
…he#4372) * fix slice bug when input is param * use _infer_value rather than _infer_value_simulated
For Relay Frontend of Tensorflow, def _slice can not handle begin or size is not Const. _infer_value_simulated is used to handle these conditions