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

Add slicing support for ellipsis and None #37

Merged
merged 5 commits into from Dec 25, 2017

Conversation

Projects
None yet
2 participants
@mrocklin
Collaborator

mrocklin commented Dec 23, 2017

I ran into this while testing https://github.com/mrocklin/sparse/pull/35 .

@hameerabbasi if you have a moment to look this over I would welcome your review

@mrocklin mrocklin force-pushed the slice-ellipsis branch from 7fe0638 to b9c2561 Dec 23, 2017

@mrocklin mrocklin force-pushed the slice-ellipsis branch from b9c2561 to df1f468 Dec 23, 2017

@hameerabbasi

This comment has been minimized.

Collaborator

hameerabbasi commented Dec 24, 2017

Looks great to me. One minor point: A check to see there isn't more than one Ellipsis and a subsequent exception + test for that exception would be great. Either way, it doesn't block previous use cases, so should be good to merge.

Edit: Ideally, the exception should be IndexError: an index can only have a single ellipsis ('...') to match Numpy behavior.

@mrocklin

This comment has been minimized.

Collaborator

mrocklin commented Dec 24, 2017

Added such a test. It looks like Python handles this check for us.

@mrocklin

This comment has been minimized.

Collaborator

mrocklin commented Dec 24, 2017

Ah, never mind. My test was faulty here

@mrocklin mrocklin force-pushed the slice-ellipsis branch from 16ba00c to 06ef208 Dec 24, 2017

if len(index) - index.count(None) > self.ndim:
raise IndexError("too many indices for array")
if index.count(Ellipsis) > 1:
raise IndexError("an index can only have a single sllipsis ('...')")

This comment has been minimized.

@hameerabbasi

hameerabbasi Dec 24, 2017

Collaborator

There is a spelling error here. sllipsis should be Ellipsis.

@mrocklin

This comment has been minimized.

Collaborator

mrocklin commented Dec 24, 2017

@mrocklin mrocklin force-pushed the slice-ellipsis branch from 06ef208 to 852ecdb Dec 24, 2017

@@ -233,10 +233,19 @@ def __sizeof__(self):
def __getitem__(self, index):
if not isinstance(index, tuple):
index = (index,)
index = tuple(ind + self.shape[i] if isinstance(ind, numbers.Integral) and ind < 0 else ind
if len(index) - index.count(None) > self.ndim:
raise IndexError("too many indices for array")

This comment has been minimized.

@hameerabbasi

hameerabbasi Dec 24, 2017

Collaborator

This will fail on things like a[1, ...] where a is (n,)-shaped. There should be an additional factor of - index.count(Ellipsis) here.

@hameerabbasi

This comment has been minimized.

Collaborator

hameerabbasi commented Dec 24, 2017

Sorry for requesting so much changes. Here's the last one. It's nitpicky, sure, but I want to make sure all our bases are covered.

@mrocklin

This comment has been minimized.

Collaborator

mrocklin commented Dec 24, 2017

Sorry for requesting so much changes. Here's the last one. It's nitpicky, sure, but I want to make sure all our bases are covered.

No, this is great. Thank you for all of your work reviewing here.

if coords:
coords = np.stack(coords, axis=0)
else:
coords = np.array(coords)

This comment has been minimized.

@hameerabbasi

hameerabbasi Dec 24, 2017

Collaborator

I may be missing something here, but won't np.array([]) create an array with a dtype of float64 and a shape of (0,)? It would be much nicer to do this: coords = np.empty((len(shape), 0), dtype=np.uint8)

This comment has been minimized.

@hameerabbasi

hameerabbasi Dec 24, 2017

Collaborator

And also to specify a dtype even in the first case, as for empty arrays it might also be float64. np.result_type and np.min_scalar_type should be useful here.

@hameerabbasi

This comment has been minimized.

Collaborator

hameerabbasi commented Dec 24, 2017

Some minor comments that will hopefully address the CI failures.

hameerabbasi and others added some commits Dec 25, 2017

@mrocklin mrocklin merged commit 8c20514 into master Dec 25, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@mrocklin mrocklin deleted the slice-ellipsis branch Dec 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment