Skip to content

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

Merged
kevinthesun merged 1 commit into
apache:masterfrom
inadob:tf_slice
Jan 24, 2020
Merged

[Bugfix][Frontend][TF] Fix incorrect calculations in tf SLICE#4518
kevinthesun merged 1 commit into
apache:masterfrom
inadob:tf_slice

Conversation

@inadob

@inadobinadob commented Dec 13, 2019

Copy link
Copy Markdown
Contributor
  • 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
Copy Markdown
ContributorAuthor

@kevinthesun@FrozenGene can you please review this patch


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

Copy link
Copy Markdown
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

@inadobinadobDec 15, 2019

Copy link
Copy Markdown
ContributorAuthor

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.

@kevinthesunkevinthesun self-assigned this Dec 16, 2019
Comment threadpython/tvm/relay/frontend/tensorflow.py Outdated
@kevinthesunkevinthesun 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

@kevinthesunkevinthesun left a comment

Copy link
Copy Markdown
Contributor

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:masterJan 24, 2020
@kevinthesun

Copy link
Copy Markdown
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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: need updateneed update based on feedbacks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@inadob@kevinthesun@optima2005@tqchen