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

Fix/numpy indexing for issue #652 #778

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

laliberte
Copy link

This PR fixes the issue raised in #652. The fix required the implementation of "vector" indexing as in numpy:

a = np.arange(27).reshape((3, 3, 3))
assert(a[0, :, [1 ,2]].shape == (2, 3))
assert(a[:, [0, 1], [1, 2]].shape == (3, 2))

To fix the issue raised in #652 a full implementation of vector indexing semantics was required. I therefore removed the error about multiple lists because it is now 1) working and 2) tested in the newly provided tests.

@tacaswell tacaswell added this to the 2.7 milestone Dec 21, 2016
if selection.reorder:
# This allows vector numpy-style indexing:
arr = (arr.reshape(numpy.prod(arr.shape))
.reshape(arr.shape, order='F')
Copy link
Member

Choose a reason for hiding this comment

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

Why fortran order?

@laliberte
Copy link
Author

This updated PR works around the fortran reindexing from the previous push and introduces a 3D test that checks all possible slices, list, indices combinations. This test can easily be converted to higher dimensions if future development requires it. I have modified the docs as well to reflect to new vector indexing capabilities.

@laliberte
Copy link
Author

Before we proceed it is important to realized that this fix implements the numpy fancy index rules called .vindex described here. It has been implemented in the high-level API of dask arrays dask/dask#433.

Instead of allowing this enhancement it straight away, we could do it in two steps:

  1. Allow it only in the narrow cases raised in Difference with numpy indexing #652
  2. Raise an issue that recommends the implementation of a .vindex method as in Slicing with lists in multiple axes dask/dask#433 that would expose the full implementation of this PR to multiple lists.

Does anyone have an opinion?

@tacaswell tacaswell modified the milestones: 2.8, 2.7 Dec 22, 2016
@tacaswell
Copy link
Member

I have been convinced that this should get more thought from more people than it is likely to get in the next few weeks and should not be targeted for the next release 😞 .

@laliberte
Copy link
Author

I'm not surprised. It is a thorny issue. At least now we have a working implementation. Whenever we're ready to move along, its going to be ready. Just let me know if you'd like me to take some specific actions.

@tacaswell
Copy link
Member

Review #739 😉

@laliberte
Copy link
Author

I'm not sure what you want me to do here. Do you want me to see if we could remove the issue in #739 by actually allowing repeated indices?

@tacaswell
Copy link
Member

Sorry, I was being a bit cheeky. You have clearly been thinking slicing much more than I have been recently and are more familiar with this code than I am, so I am looking for input on if #739 is ready / should be merged as is so that we can tag 2.7.

@laliberte
Copy link
Author

I'd say go for it! It's not a new feature and is mostly meant for better debugging purposes. Ideally, no one would ever see this error so if we're able to circumvent the repeated indices limitation then it will be simple to leave it by the wayside...

@laliberte
Copy link
Author

I think it might be possible to go beyond this limitation. It would however require to pass fairly exhaustive reordering informations from the selection routine back to the dataset object. Do you have any objections?

@tacaswell tacaswell modified the milestones: 2.8, 2.9 Feb 11, 2018
@tacaswell
Copy link
Member

@laliberte Are you still interested in working on this? It looks like it needs a re-base.

@laliberte
Copy link
Author

@tacaswell I'll take a look next week and will let you know where we stand on this issue.

@tacaswell
Copy link
Member

Thanks!

@tacaswell tacaswell removed this from the 2.9 milestone Oct 14, 2018
@tacaswell tacaswell added this to the 2.10 milestone Oct 14, 2018
@takluyver
Copy link
Member

@laliberte are you still likely to find time for this? No rush, but if it's not ready soon, it will be bumped from another release.

@takluyver
Copy link
Member

Removing the milestone because I want to try to move 2.10 forwards.

@takluyver takluyver removed this from the 2.10 milestone Jun 3, 2019
@aragilar
Copy link
Member

aragilar commented Aug 9, 2020

I've tried rebasing this, I think this will need to be rewritten on top of the new selection logic (the new test is currently failing, so the rewrite didn't fix this).

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.

None yet

4 participants