Skip to content

handle onnx nll with default ignore index #44816

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
liqunfu wants to merge 2 commits into from
Closed

handle onnx nll with default ignore index #44816

liqunfu wants to merge 2 commits into from

Conversation

@liqunfu
Copy link
Collaborator

@liqunfu liqunfu commented Sep 16, 2020

in ONNX NegativeLogLikelihoodLoss specification, ignore_index is optional without default value.
therefore, when convert nll op to ONNX, we need to set ignore_index attribute even if it is not specified (e.g. ignore_index=-100).

@dr-ci
Copy link

dr-ci bot commented Sep 16, 2020 •
edited
Loading

💊 CI failures summary and remediations

As of commit 0839233 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 3 times.

@codecov
Copy link

codecov bot commented Sep 17, 2020 •
edited
Loading

Codecov Report

Merging #44816 into master will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #44816   +/-   ##
=======================================
  Coverage   67.91%   67.92%           
=======================================
  Files         384      384           
  Lines       49839    49835    -4     
=======================================
  Hits        33848    33848           
+ Misses      15991    15987    -4
Impacted Files Coverage Δ
torch/onnx/symbolic_opset12.py 26.78% <ø> (+1.78%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20ac736...0839233. Read the comment docs.

@mruberry mruberry requested a review from bzinodev September 17, 2020 03:12
@mruberry mruberry added module: onnx Related to torch.onnx triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Sep 17, 2020
neginraoof
neginraoof approved these changes Sep 18, 2020
View reviewed changes
Copy link
Contributor

@neginraoof neginraoof 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 to me. Thanks a lot.

@spandantiwari
Copy link

spandantiwari commented Sep 22, 2020

@bzinodev - could you please take a look at this PR? We have reviewed it and looks good (and green).

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

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

@liqunfu
Copy link
Collaborator Author

liqunfu commented Sep 25, 2020

@bzinodev, this PR is failing with Facebook internal CI. Please help us take a look. Thanks!

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 28, 2020

@bzinodev merged this pull request in c3bf402.

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

+2 more reviewers

@neginraoof neginraoof neginraoof approved these changes

@bzinodev bzinodev bzinodev approved these changes

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.

7 participants

@liqunfu @spandantiwari @facebook-github-bot @neginraoof @bzinodev @pytorchbot @mruberry