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

[Relay][OP] Add reverse_reshape #2503

Merged
merged 4 commits into from
Feb 4, 2019
Merged

[Relay][OP] Add reverse_reshape #2503

merged 4 commits into from
Feb 4, 2019

Conversation

icemelon
Copy link
Member

Thanks for contributing to TVM! Please refer to guideline https://docs.tvm.ai/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers.

@junrushao
Copy link
Member

It somehow makes the API over-complicated compared with numpy's, but I do believe it is okay to be a super set of numpy, so it looks good to me

python/tvm/relay/op/transform.py Outdated Show resolved Hide resolved
src/relay/op/tensor/transform.cc Outdated Show resolved Hide resolved
@jroesch
Copy link
Member

jroesch commented Jan 24, 2019

Not against this change, but curious why do we need this?

@icemelon
Copy link
Member Author

Some MXNet models use "reverse" to reshape tensors. It seems unlikely to support it without this argument since it requires alignment from the right to infer shape.

@icemelon icemelon removed the request for review from siju-samuel January 26, 2019 00:06
@tqchen
Copy link
Member

tqchen commented Jan 26, 2019

While this makes things easy for MXNet in certain cases, I feel we should not introduce it in the normal reshape, or introduce an experimental operator(_contrib_reverse_reshape, level=10) to support such a case.

It would be great to keep the core IR simple and elegant.

@icemelon
Copy link
Member Author

@tqchen Sure, I can move this to an experimental operator.

@tqchen tqchen added the status: need update need update based on feedbacks label Jan 31, 2019
@icemelon
Copy link
Member Author

icemelon commented Feb 1, 2019

@tqchen Now implements reverse_reshape as a level-10 op. In C++, I re-use the code of reshape infer_type to reduce the code duplication. But Python and C++ sees reshape and reverse_reshape as two different ops.
@vinx13 @junrushao1994 Could you review this again? Thanks.

@icemelon icemelon changed the title [Relay][OP] Add argument reverse in reshape op [Relay][OP] Add reverse_reshape Feb 1, 2019
@icemelon icemelon added status: need review and removed status: need update need update based on feedbacks labels Feb 1, 2019
@tqchen
Copy link
Member

tqchen commented Feb 4, 2019

@icemelon9 please rebase against the master and you can merge the PR

@icemelon
Copy link
Member Author

icemelon commented Feb 4, 2019

@vinx13 @junrushao1994 @jroesch @tqchen Thanks for the review.
@tqchen Rebased to master now. Could you approve the PR? It requires 1 approving review from committers.

@icemelon icemelon self-assigned this Feb 4, 2019
@yzhliu yzhliu merged commit 40f7682 into apache:master Feb 4, 2019
@yzhliu
Copy link
Member

yzhliu commented Feb 4, 2019

Thanks @icemelon9 @junrushao1994 @jroesch @vinx13 @tqchen now it is merged.

@icemelon icemelon deleted the reshape branch February 13, 2019 05:30
libing4752 pushed a commit to libing4752/tvm that referenced this pull request Feb 18, 2019
* Enable reverse in reshape

* Fix lint and typo

* Put reverse_reshape into a separate op

* Fix pylint
merrymercy pushed a commit to merrymercy/tvm that referenced this pull request Feb 18, 2019
* Enable reverse in reshape

* Fix lint and typo

* Put reverse_reshape into a separate op

* Fix pylint
wweic pushed a commit to neo-ai/tvm that referenced this pull request Feb 20, 2019
* Enable reverse in reshape

* Fix lint and typo

* Put reverse_reshape into a separate op

* Fix pylint
wweic pushed a commit to neo-ai/tvm that referenced this pull request Feb 20, 2019
* Enable reverse in reshape

* Fix lint and typo

* Put reverse_reshape into a separate op

* Fix pylint
@yzhliu yzhliu mentioned this pull request Mar 2, 2019
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants