Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bugfix][Frontend][TF] Fix incorrect calculations in tf SLICE #4518

Merged
merged 1 commit into from
Jan 24, 2020

Conversation

inadob
Copy link
Contributor

@inadob inadob commented Dec 13, 2019

  • fix formula for calculating end indices when size[i] == -1.
  • add a test case for size[i] == -1
  • discard expanding dimension of begin_value & end_value since
    it is needed only if you pass them as scalars, not as tensors. [Relay][Frontend][TF] Fix slice when begin or size is not Const #4372
  • discard 'slice_tensor' variable so that implementation matches with
    the TF parser pattern

@inadob
Copy link
Contributor Author

inadob commented Dec 13, 2019

@kevinthesun @FrozenGene can you please review this patch

compare_tf_with_tvm([input_data], ['input:0'], 'slice_output:0')


def test_forward_slice():
_test_forward_slice_operation_input([1, 1], 0, 2)
_test_forward_slice_operation_input([1, 1], [0], [2])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better to add below lines inside _test_forward_slice_operation_input to also accept begin_value, size_value input as a single int?

    begin_value = [begin_value] if isinstance(begin_value, int) else begin_value
    size_value = [size_value] if isinstance(size_value, int) else size_value

Copy link
Contributor Author

@inadob inadob Dec 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, in the TF documentation is said that tf.slice() arguments begin & size both should be tensors. I am aware that the scalars are 0D tensors but if the TF function, in general, doesn't accept scalars, then from my point of view, it doesn't make sense to test something that is not supposed to work like this.

@kevinthesun kevinthesun self-assigned this Dec 16, 2019
@kevinthesun kevinthesun added status: need update need update based on feedbacks and removed status: need review labels Dec 18, 2019
* fix formula for calculating end indices when size[i] == -1
* add a test case for size[i] == -1
* discard expanding dimension of begin_value & end_value since
  it is needed only if you pass them as scalars not as tensors.
* discard 'slice_tensor' variable so that implementation matches
  the tf parser pattern
Copy link
Contributor

@kevinthesun kevinthesun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kevinthesun kevinthesun merged commit 9bd2c7b into apache:master Jan 24, 2020
@kevinthesun
Copy link
Contributor

Thanks @optima2005 @inadob

alexwong pushed a commit to alexwong/tvm that referenced this pull request Feb 26, 2020
…#4518)

* fix formula for calculating end indices when size[i] == -1
* add a test case for size[i] == -1
* discard expanding dimension of begin_value & end_value since
  it is needed only if you pass them as scalars not as tensors.
* discard 'slice_tensor' variable so that implementation matches
  the tf parser pattern
alexwong pushed a commit to alexwong/tvm that referenced this pull request Feb 28, 2020
…#4518)

* fix formula for calculating end indices when size[i] == -1
* add a test case for size[i] == -1
* discard expanding dimension of begin_value & end_value since
  it is needed only if you pass them as scalars not as tensors.
* discard 'slice_tensor' variable so that implementation matches
  the tf parser pattern
zhiics pushed a commit to neo-ai/tvm that referenced this pull request Mar 2, 2020
…#4518)

* fix formula for calculating end indices when size[i] == -1
* add a test case for size[i] == -1
* discard expanding dimension of begin_value & end_value since
  it is needed only if you pass them as scalars not as tensors.
* discard 'slice_tensor' variable so that implementation matches
  the tf parser pattern
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: need update need update based on feedbacks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants