Skip to content

[ONNX] Export Meshgrid #26037

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
neginraoof wants to merge 31 commits into from
Closed

[ONNX] Export Meshgrid #26037

neginraoof wants to merge 31 commits into from

Conversation

@neginraoof
Copy link
Contributor

@neginraoof neginraoof commented Sep 11, 2019

Exporting meshgrid op in opset 9 symbolics

@neginraoof neginraoof requested a review from apaszke as a code owner September 11, 2019 18:53
@pytorchbot pytorchbot added oncall: jit Add this issue/PR to JIT oncall triage queue module: onnx Related to torch.onnx labels Sep 11, 2019
lara-hdr
lara-hdr reviewed Sep 11, 2019
View reviewed changes
BowenBao
BowenBao previously requested changes Sep 11, 2019
View reviewed changes
Copy link
Collaborator

@BowenBao BowenBao left a comment

Choose a reason for hiding this comment

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

Some inline comments

BowenBao
BowenBao reviewed Sep 12, 2019
View reviewed changes
BowenBao
BowenBao reviewed Sep 12, 2019
View reviewed changes
torch/onnx/symbolic_opset9.py Outdated
for i, t in enumerate(tensors):
if t.isCompleteTensor():
shape_i = [g.op("Constant", value_t=torch.ones(1, dtype=torch.int64))] * len(tensors)
shape_i[i] = g.op("Shape", t)
Copy link
Collaborator

@BowenBao BowenBao Sep 12, 2019

Choose a reason for hiding this comment

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

consider reusing tensors_shape above..

Copy link
Contributor Author

@neginraoof neginraoof Sep 12, 2019

Choose a reason for hiding this comment

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

this is not tensors shape. This is [1,1,..,n,1,...]

@BowenBao
Copy link
Collaborator

BowenBao commented Sep 13, 2019

test if failing for opset7. Expand was added in opset8, please add meshgrid to the blacklist in symbolic_opset7.py.

BowenBao
BowenBao approved these changes Sep 13, 2019
View reviewed changes
Copy link
Collaborator

@BowenBao BowenBao left a comment

Choose a reason for hiding this comment

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

LG, one last inline comment. Also please update expect file for test_operators

facebook-github-bot
facebook-github-bot reviewed Sep 18, 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.

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

@houseroad
Copy link
Member

houseroad commented Sep 19, 2019

@pytorchbot rebase this please

facebook-github-bot
facebook-github-bot reviewed Sep 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 requested changes Sep 19, 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.

Please make sure CIs are green.

@fmassa fmassa mentioned this pull request Sep 20, 2019
Merged
@neginraoof
Copy link
Contributor Author

neginraoof commented Oct 10, 2019

@houseroad please review updates.

@neginraoof
Copy link
Contributor Author

neginraoof commented Oct 11, 2019

cc @houseroad for review

@neginraoof
Copy link
Contributor Author

neginraoof commented Oct 15, 2019

@pytorchbot rebase this please

1 similar comment
@neginraoof
Copy link
Contributor Author

neginraoof commented Oct 17, 2019

@pytorchbot rebase this please

@neginraoof
Copy link
Contributor Author

neginraoof commented Oct 18, 2019

@pytorchbot retest this please

@neginraoof
Copy link
Contributor Author

neginraoof commented Oct 19, 2019

@pytorchbot rebase this please

@neginraoof
Copy link
Contributor Author

neginraoof commented Oct 21, 2019

@houseroad please review the updates

@houseroad
Copy link
Member

houseroad commented Oct 25, 2019

The test is still failing.

@neginraoof
Copy link
Contributor Author

neginraoof commented Oct 25, 2019

@houseroad Thanks. The CI was green before I rebased. The failing test is:
caffe2/python/data_parallel_model_test.py::RecurrentNetworkParallelTest::test_equiv_recurrent
Which I'm not sure if it's related. I'll look into that.

@neginraoof
Copy link
Contributor Author

neginraoof commented Oct 25, 2019

@pytorchbot retest this please

@neginraoof
Copy link
Contributor Author

neginraoof commented Oct 25, 2019

@houseroad Tests have passed.

facebook-github-bot
facebook-github-bot reviewed Oct 25, 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 Oct 26, 2019

@houseroad merged this pull request in 60d6060.

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 requested changes

@BowenBao BowenBao BowenBao approved these changes

@apaszke apaszke Awaiting requested review from apaszke

@spandantiwari spandantiwari Awaiting requested review from spandantiwari

+1 more reviewer

@lara-hdr lara-hdr lara-hdr left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Merged module: onnx Related to torch.onnx oncall: jit Add this issue/PR to JIT oncall triage queue 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.

9 participants

@neginraoof @BowenBao @houseroad @facebook-github-bot @lara-hdr @ezyang @yf225 @pytorchbot @mruberry