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

[v1.x] ONNX Supoort for MXNet reverse op #19737

Merged
merged 3 commits into from
Jan 13, 2021
Merged

Conversation

Zha0q1
Copy link
Contributor

@Zha0q1 Zha0q1 commented Jan 9, 2021

There is no direct mapping of reverse in the onnx op set so I had to mimic the behavior by

  1. transpose the axis to be reversed to the first dim
  2. Gather along fist dim in reversed order
  3. transpose first axis back to its old position

The performance is not great most likely, but functionally i got it to behave the same as mxnet reverse

@Zha0q1 Zha0q1 requested a review from szha as a code owner January 9, 2021 01:20
@mxnet-bot
Copy link

Hey @Zha0q1 , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:

  • To trigger all jobs: @mxnet-bot run ci [all]
  • To trigger specific jobs: @mxnet-bot run ci [job1, job2]

CI supported jobs: [website, sanity, edge, windows-cpu, unix-gpu, centos-gpu, unix-cpu, windows-gpu, clang, miscellaneous, centos-cpu]


Note:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.

@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-awaiting-review PR is waiting for code review and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Jan 9, 2021
@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-awaiting-review PR is waiting for code review and removed pr-awaiting-review PR is waiting for code review pr-awaiting-testing PR is reviewed and waiting CI build and test labels Jan 11, 2021
# Transpose takes perm as a parameter, so we must 'pad' the input to a known dim (10 here)
perm = [i for i in range(10)]
perm[0], perm[axis] = axis, 0
print(perm)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove debug?

axis = int(attrs.get('axis', 0))

# Transpose takes perm as a parameter, so we must 'pad' the input to a known dim (10 here)
perm = [i for i in range(10)]
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if 10 is not enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think in mxnet 10-d is the largest you can get

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just checked: it seems we can create > 10-d tensors, but I think many ops do not support >= 10d tensors and in general there is no use case for high-dimensional tensors

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks for the explanation.

@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-work-in-progress PR is still work in progress pr-awaiting-review PR is waiting for code review and removed pr-awaiting-review PR is waiting for code review pr-awaiting-testing PR is reviewed and waiting CI build and test pr-work-in-progress PR is still work in progress labels Jan 12, 2021
Copy link
Contributor

@josephevans josephevans 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!

Copy link
Contributor

@waytrue17 waytrue17 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!

@Zha0q1 Zha0q1 merged commit 13c449e into apache:v1.x Jan 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants