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

[NNVM][ONNX] Slice, Floor, Ceil, Clip and MatMul support for frontend #1297 #1371

Merged
merged 3 commits into from
Jul 4, 2018

Conversation

srkreddy1238
Copy link
Contributor

Thanks for contributing to TVM! Please refer to guideline https://docs.tvm.ai/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from others in the community.

@srkreddy1238 srkreddy1238 changed the title Slice op support #1297 [NNVM][ONNX] Slice op support #1297 Jul 2, 2018
@tqchen
Copy link
Member

tqchen commented Jul 3, 2018

please request review from the reviewers

@srkreddy1238
Copy link
Contributor Author

@Huyuwei @masahi @zhreshold please review.

@masahi
Copy link
Member

masahi commented Jul 3, 2018

is the last commit related to this PR?

@srkreddy1238
Copy link
Contributor Author

Yes. I wish to conclude this PR here.

@masahi
Copy link
Member

masahi commented Jul 3, 2018

But how floor, clip, MatMul etc. relate to Slice op?

@srkreddy1238
Copy link
Contributor Author

I am working on upgrading ONNX frontend with new ops available recently.

Started of with slice :)

@srkreddy1238
Copy link
Contributor Author

I could raise another PR if needed.

@masahi
Copy link
Member

masahi commented Jul 3, 2018

Then edit the title accordingly.
And let us know when you are done.

@srkreddy1238 srkreddy1238 changed the title [NNVM][ONNX] Slice op support #1297 [NNVM][ONNX] Slice, Floor, Ceil, Clip and MatMul support for frontend #1297 Jul 3, 2018
@srkreddy1238
Copy link
Contributor Author

@masahi Changed the title. Thanks

else:
new_axes.append(i)
new_starts.append(0)
new_ends.append(10000) # very big number
Copy link
Member

Choose a reason for hiding this comment

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

how does strided_slice handle the end index? BTW, 10000 is definitely not big enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated intmax instead of 10000.
strided_slice adjusts the end index based on the input tensor size.

Please check.

@@ -477,7 +518,7 @@ def _get_convert_map(opset):
'SpatialBN': BatchNorm.get_converter(opset),

# defs/generator
# 'Constant'
# 'Constant' # Implemented
Copy link
Member

Choose a reason for hiding this comment

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

What is this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already implemented but it's not part of conversion map. Hence marked as Implemented.

_test_slice_iteration(x, x[:, :, 3:4], (0, 0, 3), (20, 10, 4))
_test_slice_iteration(x, x[:, 1:1000], (1), (1000), (1))
_test_slice_iteration(x, x[:, 0:-1], (0), (-1), (1))
#_test_slice_iteration(x, x[:, 1000:1000], (1000), (1000), (1))
Copy link
Member

Choose a reason for hiding this comment

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

You should enable or remove this line

@srkreddy1238
Copy link
Contributor Author

@masahi thanks for the review.

@srkreddy1238
Copy link
Contributor Author

@tqchen can you merge this ?

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.

None yet

4 participants