Skip to content

Conversation

@jakirkham
Copy link
Member

@jakirkham jakirkham commented Feb 20, 2017

Fixes https://github.com/alimanfoo/zarr/issues/93

Uses kenjutsu to handle a variety of tricky cases with Ellipsis, negative step sizes, negative indices, and standard cases as well. This simplifies the code, avoids a variety of bugs. Also this should avoid the need for normalizing each axis, but have not yet cut that code out.

@jakirkham jakirkham mentioned this pull request Feb 20, 2017
return len_slices(rf_item) == shape


def normalize_axis_selection(item, l):
Copy link
Member Author

Choose a reason for hiding this comment

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

This function is no longer needed. That said, I can leave it, change it to use kenjutsu as well, or just remove it. Please let me know what you think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Went ahead and tried to restructure things to use this function anyways for validation purposes. That way we can leverage all of the current tests in the first pass.

@jakirkham
Copy link
Member Author

Should add that I saw a lot of constraints on what kind of slices are permissible (e.g. step size). Am happy to add them back in.

Though some of them like an overly large slice are technically permissible both in vanilla Python and NumPy. Not to mention when provided with a slice and a shape, kenjutsu cuts these slices down to a proper size. So it shouldn't cost anything extra to just use them after they are reformatted/normalized.

@jakirkham jakirkham changed the title WIP: Fix slice normalization and length determination Fix slice normalization and length determination Feb 20, 2017
@jakirkham
Copy link
Member Author

Alright, I think this is ready for review.

@alimanfoo
Copy link
Member

Thank you, very helpful, and very nice that the new ellipsis tests are passing.

I'd like to mull this over for a bit. I've spent some time looking at the reformat_slices and reformat_slice functions in kenjutsu where the goodness happens. Mostly I can see what's happening but there are some bits I need to spend more time on.

I think I may bring simplified versions of these functions into Zarr as an interim solution. That way I can go through every line of code and get acquainted. This is such a critical piece of code I think I need to understand what every line is doing. Also I think it will help to have this functionality inside Zarr when working on fancy indexing, as fancy indexing will require some changes to these functions. But then ultimately when the dust settles from that, maybe consider factoring back out to kenjutsu.

@alimanfoo alimanfoo mentioned this pull request Apr 6, 2017
@alimanfoo
Copy link
Member

Closing as superseded by indexing work #172.

@alimanfoo alimanfoo closed this Nov 10, 2017
@jakirkham jakirkham deleted the fix_93 branch December 2, 2018 02:00
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.

2 participants