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

[Relay][Frontend][TFLite] transpose implementation for tflite.py #3705

Merged
merged 10 commits into from
Aug 19, 2019

Conversation

cchung100m
Copy link
Contributor

Hi @FrozenGene

I am a newbie to tvm and trying to do some contribution for frontend and relay. This is my first time to add the missing operator for tflite, I would appreciate if you can guide me how to go through it :)

Many thanks,

@cchung100m cchung100m changed the title transpose implementation for tflite.py [Relay][Frontend][TFLite] transpose implementation for tflite.py Aug 5, 2019
@FrozenGene
Copy link
Member

Hi, thanks for contribution!

For the TFLite frontend, I think you could use git history to see tflite.py how we implement one op previously. Then we could discuss further.

For your PR, besides tflite.py, we should also need to add op unittest in tests/python/frontend/tflite/test_forward.py. You could also refer how we do it previously. This could help you verify your implementation is right.

For GitHub CI, when it is red, you could click Details to see what happened. For example, your pr make CI red, when we click Details, we find:

************* Module tvm.relay.frontend.tflite

E:767,18: Unexpected keyword argument 'axis' in function call (unexpected-keyword-arg)

E:769,18: Unexpected keyword argument 'axis' in function call (unexpected-keyword-arg)

So, transpose doesn't have axis, you could check the doc: https://docs.tvm.ai/api/python/relay/op.html to solve it.

After you have done it, you could make one new pr, we could discuss and review it again.

Thanks.

@cchung100m
Copy link
Contributor Author

Hi @FrozenGene

Thanks for your kindly reply and It's really help me to do this work.

I update the implementation of transpose operator and also add unittest.

input_tensor_idx = input_tensor.tensor_idx

in_expr = self.get_expr(input_tensor_idx)
out = _op.transpose(data=in_expr)
Copy link
Member

Choose a reason for hiding this comment

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

You omit the parameter of axes, which is input_tensors[1] of transpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @FrozenGene
Thanks for reminding, I update the code for the parameter of axes and add new test case

# axis
in_axis = tuple(self.get_tensor_value(input_tensors[1]))

if in_axis is None:
Copy link
Member

Choose a reason for hiding this comment

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

Could we have the condition in_axis be None? I think we will never meet this condition. Because we construct it using tuple constructor. The bad case should be () in my opinion. Could you check the code here again and write one unit test to cover?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing. I improve the condition and add one unit test to cover empty tuple.

Copy link
Member

@FrozenGene FrozenGene left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@FrozenGene
Copy link
Member

@kevinthesun @yzhliu @tqchen @srkreddy1238 could anybody help to manage it?

@cchung100m
Copy link
Contributor Author

Hi @icemelon9 @zhiics

Could you help to review/merge the PR?

many thanks,

@icemelon icemelon merged commit 3c8901a into apache:master Aug 19, 2019
@icemelon
Copy link
Member

Thanks @cchung100m @FrozenGene. This is now merged.

wweic pushed a commit to wweic/tvm that referenced this pull request Sep 16, 2019
…che#3705)

* transpose implementation for tflite.py

* add TRANSPOSE to convert_map

* Fix Unexpected keyword argument 'axis' in function call

* add test for transpose oprator

* Add the parameter 'axes' handling

* add test for transpose oprator

* solve conflict within CONTRIBUTORS.md

* Improve the if condition for empty tuple

* Add one unit test to cover empty tuple

* solve conflict within CONTRIBUTORS.md
wweic pushed a commit to wweic/tvm that referenced this pull request Sep 16, 2019
…che#3705)

* transpose implementation for tflite.py

* add TRANSPOSE to convert_map

* Fix Unexpected keyword argument 'axis' in function call

* add test for transpose oprator

* Add the parameter 'axes' handling

* add test for transpose oprator

* solve conflict within CONTRIBUTORS.md

* Improve the if condition for empty tuple

* Add one unit test to cover empty tuple

* solve conflict within CONTRIBUTORS.md
wweic pushed a commit to neo-ai/tvm that referenced this pull request Sep 16, 2019
…che#3705)

* transpose implementation for tflite.py

* add TRANSPOSE to convert_map

* Fix Unexpected keyword argument 'axis' in function call

* add test for transpose oprator

* Add the parameter 'axes' handling

* add test for transpose oprator

* solve conflict within CONTRIBUTORS.md

* Improve the if condition for empty tuple

* Add one unit test to cover empty tuple

* solve conflict within CONTRIBUTORS.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants