Skip to content
This repository was archived by the owner on Nov 17, 2023. It is now read-only.

onnx softmaxoutput fix#13116

Merged
sandeep-krishnamurthy merged 1 commit intoapache:masterfrom
Roshrini:onnx_bugs
Dec 7, 2018
Merged

onnx softmaxoutput fix#13116
sandeep-krishnamurthy merged 1 commit intoapache:masterfrom
Roshrini:onnx_bugs

Conversation

@Roshrini
Copy link
Copy Markdown
Member

@Roshrini Roshrini commented Nov 5, 2018 •
edited
Loading

Description

Fix for the failing tutorial mentioned here
#13099
@vandanavk

Checklist

Essentials

  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

@Roshrini Roshrini requested a review from szha as a code owner November 5, 2018 18:11
@ankkhedia
Copy link
Copy Markdown
Contributor

ankkhedia commented Nov 5, 2018

@mxnet-label-bot[pr-awaiting-review, ONNX]

@marcoabreu marcoabreu added ONNX pr-awaiting-review PR is waiting for code review labels Nov 5, 2018
@anirudhacharya
Copy link
Copy Markdown
Member

anirudhacharya commented Nov 6, 2018

@Roshrini can you give more context why this change is needed. The link you provided just says that there is a bug. What is the bug, how was it introduced, and how is this fixing it?

@zhreshold for review.

@Roshrini
Copy link
Copy Markdown
Member Author

Roshrini commented Nov 12, 2018

get_inputs(node, kwargs) method which was getting used works on all the inputs whereas for SoftmaxOutput operator we are using single input. So, iterating on all the inputs gave OutOfIndex error.
This bug was introduced while cleaning up the code - #12878

@Roshrini
Copy link
Copy Markdown
Member Author

Roshrini commented Nov 12, 2018

@zhreshold Can you review/merge this PR?

@kalyc
Copy link
Copy Markdown
Contributor

kalyc commented Nov 12, 2018

@mxnet-label-bot update [pr-awaiting-merge]

@marcoabreu marcoabreu added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed ONNX pr-awaiting-review PR is waiting for code review labels Nov 12, 2018
sandeep-krishnamurthy
sandeep-krishnamurthy reviewed Nov 14, 2018
View reviewed changes
Copy link
Copy Markdown

@sandeep-krishnamurthy sandeep-krishnamurthy left a comment

Choose a reason for hiding this comment

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

LGTM.
Why was this not caught in tests?
Are there any tests added now?

@stu1130
Copy link
Copy Markdown
Contributor

stu1130 commented Nov 21, 2018

@mxnet-label-bot update [pr-work-in-progress]

@marcoabreu marcoabreu added pr-work-in-progress PR is still work in progress and removed pr-awaiting-merge Review and CI is complete. Ready to Merge labels Nov 21, 2018
@vandanavk
Copy link
Copy Markdown
Contributor

vandanavk commented Nov 27, 2018

@mxnet-label-bot add [pr-awaiting-testing]

@marcoabreu marcoabreu added the pr-awaiting-testing PR is reviewed and waiting CI build and test label Nov 27, 2018
zhreshold
zhreshold suggested changes Dec 3, 2018
View reviewed changes
Comment thread python/mxnet/contrib/onnx/mx2onnx/_op_translations.py
and return the created node.
"""
name, _, _ = get_inputs(node, kwargs)
name = node["name"]
Copy link
Copy Markdown
Member

@zhreshold zhreshold Dec 3, 2018

Choose a reason for hiding this comment

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

The change LGTM, can you add a test case to guard this against happening again?

@Roshrini
Copy link
Copy Markdown
Member Author

Roshrini commented Dec 6, 2018

@sandeep-krishnamurthy @zhreshold Added test case for the operator

sandeep-krishnamurthy
sandeep-krishnamurthy approved these changes Dec 7, 2018
View reviewed changes
Copy link
Copy Markdown

@sandeep-krishnamurthy sandeep-krishnamurthy left a comment

Choose a reason for hiding this comment

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

Thanks

@sandeep-krishnamurthy sandeep-krishnamurthy merged commit f390f0c into apache:master Dec 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@szha szha Awaiting requested review from szha szha is a code owner

3 more reviewers

@zhreshold zhreshold zhreshold approved these changes

@sandeep-krishnamurthy sandeep-krishnamurthy sandeep-krishnamurthy approved these changes

@vandanavk vandanavk vandanavk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pr-awaiting-testing PR is reviewed and waiting CI build and test pr-work-in-progress PR is still work in progress

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@Roshrini @ankkhedia @anirudhacharya @kalyc @stu1130 @vandanavk @zhreshold @sandeep-krishnamurthy @marcoabreu