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

Numpy indexing #15763

Closed
wants to merge 25 commits into from
Closed

Numpy indexing #15763

wants to merge 25 commits into from

Conversation

zoeygxy
Copy link
Contributor

@zoeygxy zoeygxy commented Aug 6, 2019

Description

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

Changes

  • Fixed NDArray indexing: newaxis and setitem.
  • Added support for numpy indexing, including basic and advanced indexing, None and Ellipsis.

Comments

  • This PR is mainly for CI test use. Code will be merged to master branch later.
  • Indexing speed issue awaiting fix.
  • Need to add test cases for np.full.
  • Boolean Array indexing is under construction.

@zoeygxy zoeygxy changed the base branch from master to numpy August 6, 2019 06:33
@reminisce reminisce added the Numpy label Aug 6, 2019
@@ -614,6 +614,25 @@ def convert(num):
range(4),
range(3, 0, -1),
(range(4,), [1]),
# Test Ellipsis ('...')
((1, Ellipsis, -1), False),
Copy link
Contributor

Choose a reason for hiding this comment

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

Only the first item of the tuple is needed in this test. Please delete all Falses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks!

@zoeygxy
Copy link
Contributor Author

zoeygxy commented Aug 13, 2019

Still fixing autograd for zero-size tensors.

@haojin2
Copy link
Contributor

haojin2 commented Sep 2, 2019

Closing this PR now since #15942 has been merged.

@haojin2 haojin2 closed this Sep 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants