Skip to content

[ONNX] Export det #26958

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 21 commits into from
Closed

[ONNX] Export det #26958

neginraoof wants to merge 21 commits into from

Conversation

@neginraoof
Copy link
Contributor

@neginraoof neginraoof commented Sep 27, 2019 •
edited
Loading

Added symbolic to export det in opset 11
Updating ONNX submodule is required for det export

@pytorchbot pytorchbot added the module: onnx Related to torch.onnx label Sep 27, 2019
BowenBao
BowenBao approved these changes Oct 1, 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.

LGTM, minor comment on better test coverage.

test/onnx/test_pytorch_onnx_onnxruntime.py Outdated
def forward(self, x):
return torch.det(x)

x = torch.randn(2, 3, 5, 5, requires_grad=True)
Copy link
Collaborator

@BowenBao BowenBao Oct 1, 2019

Choose a reason for hiding this comment

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

Can we add another test case where input shape is (5, 5)? That covers the case where output is scalar.

@lara-hdr
Copy link
Contributor

lara-hdr commented Oct 1, 2019

@neginraoof TestOperators.test_det is failing

@neginraoof
Copy link
Contributor Author

neginraoof commented Oct 7, 2019

@pytorchbot rebase this please

houseroad
houseroad requested changes Oct 9, 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.

Export looks good.

RuntimeError: lu: LAPACK library not found in compilation

Please fix the tests.

facebook-github-bot
facebook-github-bot reviewed Oct 9, 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.

@zou3519 zou3519 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Oct 10, 2019
@neginraoof
Copy link
Contributor Author

neginraoof commented Oct 10, 2019

@houseroad I didn't see this locally. Trying to repro. Do you have an idea why it complains about the missing package?

@neginraoof
Copy link
Contributor Author

neginraoof commented Oct 11, 2019

cc @houseroad for review

houseroad
houseroad reviewed Oct 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.

Fix the conflict?

@neginraoof
Copy link
Contributor Author

neginraoof commented Oct 14, 2019

@houseroad just updated. please review

houseroad
houseroad requested changes Oct 16, 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 do NOT update onnx submodule

@neginraoof
Copy link
Contributor Author

neginraoof commented Oct 16, 2019

@houseroad onnx is reset. Just rebased this

@neginraoof
Copy link
Contributor Author

neginraoof commented Oct 16, 2019

@houseroad this is ready

@neginraoof
Copy link
Contributor Author

neginraoof commented Oct 22, 2019

@houseroad please review the updates

houseroad
houseroad approved these changes Oct 22, 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.

LGTM

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

@houseroad merged this pull request in 4f70b5a.

thiagocrepaldi pushed a commit to thiagocrepaldi/pytorch that referenced this pull request Feb 4, 2020
@neginraoof
Export det (pytorch#26958)
0903123 
Summary:
Added symbolic to export det in opset 11
Updating ONNX submodule is required for det export
Pull Request resolved: pytorch#26958

Reviewed By: hl475

Differential Revision: D17844887

Pulled By: houseroad

fbshipit-source-id: 224ae3ff82939dc7ae8584c5a30a31fe6afa05f6
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

@BowenBao BowenBao BowenBao approved these changes

@houseroad houseroad houseroad approved these changes

Assignees

No one assigned

Labels

Merged module: onnx Related to torch.onnx module: third_party 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 @lara-hdr @facebook-github-bot @BowenBao @houseroad @ezyang @zou3519 @pytorchbot @mruberry