Skip to content

[ONNX] Add support for onnx tensor index export #21716

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.

Sign up for GitHub

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

Jump to bottom
Closed
BowenBao wants to merge 5 commits into from
Closed

[ONNX] Add support for onnx tensor index export #21716

BowenBao wants to merge 5 commits into from

Conversation

@BowenBao
Copy link
Collaborator

@BowenBao BowenBao commented Jun 13, 2019

Support exporting

  • Standard tensor indexing like
x = torch.ones(4, 5)
ind = torch.tensor([0, 1])

return x[ind]

x = torch.ones(4,5,6,7,8)
ind1 = torch.tensor([0, 1])
ind2 = torch.tensor([[3], [2]])
ind3 = torch.tensor([[2, 2], [4, 5]])

return x[2:4, ind1, None, ind2, ind3, :]

It would be ideal if ONNX can natively support indexing in future opsets, but for opset <= 10 it will always need this kind of workarounds.

There are still various limitations, such as not supporting advanced indexing with negative indices, not supporting mask indices of rank > 1, etc. My feeling is that these are less common cases that requires great effort to support using current opset, and it's better to not make the index export more cumbersome than it already is.

@pytorchbot pytorchbot added the module: onnx Related to torch.onnx label Jun 13, 2019
@BowenBao BowenBao mentioned this pull request Jun 13, 2019
Closed
houseroad
houseroad reviewed Jun 13, 2019
View reviewed changes
Copy link
Member

@houseroad houseroad left a comment

Choose a reason for hiding this comment

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

First, fix the linter?

@gchanan gchanan added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jun 13, 2019
@BowenBao BowenBao force-pushed the onnx_advanced_indexing branch from 5a539e3 to e65607f Compare June 14, 2019 00:06
facebook-github-bot
facebook-github-bot reviewed Jun 19, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

houseroad
houseroad reviewed Jun 21, 2019
View reviewed changes
houseroad
houseroad reviewed Jun 21, 2019
View reviewed changes
@BowenBao BowenBao force-pushed the onnx_advanced_indexing branch from e65607f to 170ef3d Compare June 24, 2019 21:53
facebook-github-bot
facebook-github-bot reviewed Jun 27, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@BowenBao BowenBao force-pushed the onnx_advanced_indexing branch from 170ef3d to 1e1a762 Compare July 2, 2019 20:28
facebook-github-bot
facebook-github-bot reviewed Jul 2, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@BowenBao BowenBao mentioned this pull request Jul 8, 2019
Closed
@BowenBao BowenBao force-pushed the onnx_advanced_indexing branch from 1e1a762 to 6776b8f Compare July 15, 2019 23:18
facebook-github-bot pushed a commit that referenced this pull request Jul 23, 2019
@BowenBao @facebook-github-bot
Export torch.arange to ONNX (#22601)
eb5137a 
Summary:
Some overlap with #21716 regarding caffe2 nonzero. Will rebase the other one accordingly whichever gets merged first.
Pull Request resolved: #22601

Reviewed By: zrphercule

Differential Revision: D16224660

Pulled By: houseroad

fbshipit-source-id: dbfd1b8776cb626601e0bf83b3fcca291806e653
facebook-github-bot
facebook-github-bot reviewed Jul 23, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

houseroad
houseroad approved these changes Jul 23, 2019
View reviewed changes
Copy link
Member

@houseroad houseroad left a comment

Choose a reason for hiding this comment

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

Looks good. Could you rebase again?

Btw, we should definitely introduce new index operator in ONNX opset 11. This translation is very complicated...

@BowenBao BowenBao force-pushed the onnx_advanced_indexing branch from 6776b8f to 2a1a718 Compare July 23, 2019 19:03
facebook-github-bot
facebook-github-bot reviewed Jul 23, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jul 24, 2019

@houseroad merged this pull request in a35136d.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@houseroad houseroad houseroad approved these changes

@spandantiwari spandantiwari Awaiting requested review from spandantiwari

Assignees

@houseroad houseroad

Labels

caffe2 Merged module: onnx Related to torch.onnx open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@BowenBao @facebook-github-bot @houseroad @ezyang @gchanan @pytorchbot @mruberry