Skip to content
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.

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

Support non-default negative axis value and intuitive data type combination for OneHot op #1317

Merged
merged 2 commits into from
Jul 1, 2019

Conversation

hariharans29
Copy link
Member

@hariharans29 hariharans29 commented Jun 29, 2019

Description:
This change tries to resolve 2 open issues regarding the OneHot operator:

#1313 : Treat (non-default) negative axis value like how any other ONNX operator does
#1314 : Support a more intuitive data type combination for this operator

Motivation and Context
Resolve #1313 and #1314

@hariharans29 hariharans29 requested a review from a team as a code owner June 29, 2019 00:20
@hariharans29
Copy link
Member Author

CC: @fdwr

@pranavsharma
Copy link
Contributor

Is this behavior documented in the spec? If not, can you document it?

@hariharans29
Copy link
Member Author

hariharans29 commented Jul 1, 2019

Is this behavior documented in the spec? If not, can you document it?

This a good suggestion.

But this behavior is implicitly understood and expected of all ONNX ops that have an axis attribute/input. In fact, there are quite a few ONNX ops where this behavior is not explicitly stated but is handled anyway. For example, ArgMax, ArgMIn (to name a couple) have a similar expectation but have no such behavior explicitly documented. ONNXRuntime handles negative axis wherever an axis input is provided.

It might be worthwhile to state this just once somewhere in ONNX as opposed to adding one line for this op (and every other op that's missing the behavior explicitly)

@hariharans29 hariharans29 merged commit a077ac8 into master Jul 1, 2019
@hariharans29 hariharans29 deleted the OneHotChanges branch July 1, 2019 21:29
hariharans29 added a commit that referenced this pull request Aug 29, 2019
…nation for OneHot op (#1317)

* Handle nondefault negative axis value

* Support more intuitive data types for this op
hariharans29 added a commit that referenced this pull request Sep 4, 2019
…nation for OneHot op (#1317) (#1732)

* Handle nondefault negative axis value

* Support more intuitive data types for this op
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OneHot should treat negative axis as range from last dimension
2 participants