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

MAINT: Use PySlice_GetIndicesEx instead of custom reimplementation #7215

Merged
merged 1 commit into from
Feb 19, 2016

Conversation

njsmith
Copy link
Member

@njsmith njsmith commented Feb 9, 2016

This has the side effects of:

  • changing several IndexError exceptions into TypeErrors
  • allowing slices like arr[False:True] as equivalent to
    arr[0:1] (because now we're using Python's logic for interpreting
    slices, and Python is happy with treating bools as integers in integer
    contexts).

It also deletes almost 100 lines of code :-).

@njsmith
Copy link
Member Author

njsmith commented Feb 9, 2016

CC @seberg

@seberg
Copy link
Member

seberg commented Feb 9, 2016

Sounds good to me, allowing booleans in slice context seems not so bad. I think I would wait with putting this in until the dust settled on 1.11 though.

@charris
Copy link
Member

charris commented Feb 9, 2016

Two of the errors (warnings) look legitimate.

@njsmith
Copy link
Member Author

njsmith commented Feb 9, 2016

Ugh, the warnings are totally not legitimate... on py2, the first argument is typed as PySliceObject*, and on py3 it's typed as PyObject*, so the implicit cast is fine and there's no way to make both versions happy at the same time. But we need to do something to make the tests work; I will ponder.

@juliantaylor
Copy link
Contributor

nothing a little macro magic can't solve, see e.g. nditer_pywrap.c for the same issue

@juliantaylor
Copy link
Contributor

that solution could be put into the 3kcompat header if we need it more than once

@njsmith
Copy link
Member Author

njsmith commented Feb 10, 2016

@juliantaylor: oh hey, and those nditer_pywrap.c usages are wrong anyway, because they call the terrible PySlice_GetIndices instead of the functional PySlice_GetIndicesEx. (in particular the calls in nditer_pywrap.c assumes that it sets an exception on error, which is not true...)

This has the side effects of:
- changing several IndexError exceptions into TypeErrors
- allowing slices like `arr[False:True]` as equivalent to
  `arr[0:1]` (because now we're using Python's logic for interpreting
  slices, and Python is happy with treating bools as integers in integer
  contexts).

It also deletes almost 100 lines of code :-).

While I was at it I also cleaned up some buggy uses of
PySlice_GetIndices (which is pretty broken -- e.g. the code was assuming
that it sets an exception on error, but this is not true! the Python
docs explicitly recommend that you never use it.)
@njsmith
Copy link
Member Author

njsmith commented Feb 10, 2016

Fixed / rebased / squashed, so should be good to go (hopefully travis agrees).

I can see @seberg's argument that we might want to wait on merging this until #7162 is resolved.

charris added a commit that referenced this pull request Feb 19, 2016
Use PySlice_GetIndicesEx instead of custom reimplementation
@charris charris merged commit c39c90b into numpy:master Feb 19, 2016
@charris
Copy link
Member

charris commented Feb 19, 2016

Thanks Nathaniel.

@charris charris changed the title Use PySlice_GetIndicesEx instead of custom reimplementation MAINT: Use PySlice_GetIndicesEx instead of custom reimplementation Nov 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants