Skip to content

Adds symbolic op for logsumexp #22306

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
iiSeymour wants to merge 4 commits into from
Closed

Adds symbolic op for logsumexp #22306

iiSeymour wants to merge 4 commits into from

Conversation

@iiSeymour
Copy link
Contributor

@iiSeymour iiSeymour commented Jun 27, 2019

No description provided.

@pytorchbot pytorchbot added the module: onnx Related to torch.onnx label Jun 27, 2019
@iiSeymour iiSeymour mentioned this pull request Jun 27, 2019
Closed
@lara-hdr
Copy link
Contributor

lara-hdr commented Jun 27, 2019

The caffe2 test is failing.
I looked in https://caffe2.ai/docs/operators-catalogue.html and it seems like ReduceLogSumExp op does not exist in caffe2.
Try adding an ONNXRuntime test in test/onnx/test_pytorch_onnx_onnxruntime.py to verify the behavior?

lara-hdr
lara-hdr reviewed Jun 27, 2019
View reviewed changes
@iiSeymour
Copy link
Contributor Author

iiSeymour commented Jun 27, 2019

The caffe2-py2-devtoolset7-rocmrpm-centos7.5 failure appears to be unrelated to the change.

@li-roy li-roy requested a review from houseroad June 28, 2019 03:23
@li-roy li-roy added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jun 28, 2019
facebook-github-bot
facebook-github-bot reviewed Jun 28, 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 28, 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. If the test can cover the single dimension case, it will be better.

Thanks!

@iiSeymour
Copy link
Contributor Author

iiSeymour commented Jun 28, 2019

Single dim test added & tests passing ✔️

facebook-github-bot
facebook-github-bot reviewed Jun 29, 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 Jun 29, 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, thanks.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jun 29, 2019

@houseroad merged this pull request in d8de69d.

@iiSeymour iiSeymour deleted the feature/onnx-logsumexp branch June 29, 2019 08:38
xzhu1900 pushed a commit to xzhu1900/pytorch that referenced this pull request Jul 5, 2019
@iiSeymour @xzhu1900
Adds symbolic op for logsumexp
58836f3 
Summary: Pull Request resolved: pytorch#22306

Differential Revision: D16046027

Pulled By: houseroad

fbshipit-source-id: 7319fd58321220941250c5b8eff024914798e392
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

+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 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.

8 participants

@iiSeymour @lara-hdr @facebook-github-bot @houseroad @ezyang @li-roy @pytorchbot @mruberry