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

moveaxis operator now accepts negative indices and sequence of ints as well. #14321

Merged
merged 14 commits into from
Mar 15, 2019
Merged

moveaxis operator now accepts negative indices and sequence of ints as well. #14321

merged 14 commits into from
Mar 15, 2019

Conversation

ifeherva
Copy link
Contributor

@ifeherva ifeherva commented Mar 4, 2019

Description

The moveaxis operator is now numpy compatible by accepting negative indices and tuples as well. Added a series of unit tests based on the original numpy repo.

Adds request from #13563

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • 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)

  • Code is well-documented:

  • For user-facing API changes, API doc string has been updated.

  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

@ifeherva ifeherva requested a review from szha as a code owner March 4, 2019 19:20
ThomasDelteil
ThomasDelteil approved these changes Mar 4, 2019
View reviewed changes
Copy link
Contributor

@ThomasDelteil ThomasDelteil left a comment

Choose a reason for hiding this comment

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

It would be good to have it as a symbol operator as well to be able to use it in hybridized graphs rather than just ndarray operations.

@ifeherva ifeherva mentioned this pull request Mar 4, 2019
Closed
@anirudhacharya
Copy link
Member

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

@marcoabreu marcoabreu added the pr-awaiting-merge Review and CI is complete. Ready to Merge label Mar 4, 2019
@vandanavk
Copy link
Contributor

@ifeherva could you check the CI failures and re-trigger the build?

@ifeherva
Copy link
Contributor Author

ifeherva commented Mar 5, 2019

@vandanavk the error messages do not really make sense to me, they look unrelated to the content of this PR. Can someone retrigger CI please?

@ifeherva
Copy link
Contributor Author

ifeherva commented Mar 7, 2019

Tests are very flaky, none of them related to this PR.

@wkcn
Copy link
Member

wkcn commented Mar 7, 2019

Sorry that I wrote a typo, which should be retrigger.

@eric-haibin-lin eric-haibin-lin mentioned this pull request Mar 8, 2019
Open
@wkcn
Copy link
Member

wkcn commented Mar 11, 2019 •
edited
Loading

I do not know why the CI always fail in this PR.

@ifeherva
Copy link
Contributor Author

ifeherva commented Mar 13, 2019 •
edited
Loading

Strange that the same GPU tests fail though I am puzzled how these changes contribute to CUDA errors.

@wkcn
Copy link
Member

wkcn commented Mar 15, 2019

Could we disable the new testcase to track the error?

@wkcn
Copy link
Member

wkcn commented Mar 15, 2019 •
edited
Loading

I guess assert_exception has some problems.

@wkcn
Copy link
Member

wkcn commented Mar 15, 2019

Current MXNet Does not support zero size tensor, so I modified the testcase.

@wkcn wkcn merged commit 43173f5 into apache:master Mar 15, 2019
@wkcn
Copy link
Member

wkcn commented Mar 15, 2019

Great! Glad to merge it.
Thanks for your contribution!

@ifeherva
Copy link
Contributor Author

Thanks for figuring out the error!

@ifeherva ifeherva deleted the moveaxis_negative_index branch March 15, 2019 14:41
vdantu pushed a commit to vdantu/incubator-mxnet that referenced this pull request Mar 31, 2019
@ifeherva @vdantu
moveaxis operator now accepts negative indices and sequence of ints a…
05e2cbe 
…s well. (apache#14321)

* nd.moveaxis now accepts negative indices and sequence of ints as well.

* Retrigger CI

* retrigger CI

* Retriever

* Retrigger CI

* Retrigger CI

* Retrigger CI, hope for success

* Update test_ndarray.py

* Let’s try to retrigger CI again.

* Retrigger CI

* Disable the new testcase to track bugs

* enable some new testcases

* Update test_ndarray.py

* Update test_ndarray.py
nswamy pushed a commit that referenced this pull request Apr 5, 2019
@ifeherva @nswamy
moveaxis operator now accepts negative indices and sequence of ints a…
5809c2d 
…s well. (#14321)

* nd.moveaxis now accepts negative indices and sequence of ints as well.

* Retrigger CI

* retrigger CI

* Retriever

* Retrigger CI

* Retrigger CI

* Retrigger CI, hope for success

* Update test_ndarray.py

* Let’s try to retrigger CI again.

* Retrigger CI

* Disable the new testcase to track bugs

* enable some new testcases

* Update test_ndarray.py

* Update test_ndarray.py
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@ifeherva
moveaxis operator now accepts negative indices and sequence of ints a…
e2c983d 
…s well. (apache#14321)

* nd.moveaxis now accepts negative indices and sequence of ints as well.

* Retrigger CI

* retrigger CI

* Retriever

* Retrigger CI

* Retrigger CI

* Retrigger CI, hope for success

* Update test_ndarray.py

* Let’s try to retrigger CI again.

* Retrigger CI

* Disable the new testcase to track bugs

* enable some new testcases

* Update test_ndarray.py

* Update test_ndarray.py
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ThomasDelteil ThomasDelteil ThomasDelteil approved these changes

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

Assignees
No one assigned
Labels
pr-awaiting-merge Review and CI is complete. Ready to Merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ifeherva @anirudhacharya @vandanavk @wkcn @ThomasDelteil @marcoabreu