E5E7 MAINT: Make the refactor suggested in prepare_index by eric-wieser · Pull Request #8278 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

MAINT: Make the refactor suggested in prepare_index #8278

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.

Sign up for GitHub

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

Merged
seberg merged 11 commits into from
Jul 18, 2017
Merged

MAINT: Make the refactor suggested in prepare_index #8278

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
87e1294
MAINT: Make the refactor suggested in prepare_index
eric-wieser Nov 14, 2016
bfd70c2
MAINT: Fix typo, remove unnecessary nesting
eric-wieser Dec 5, 2016
37e6e1f
MAINT: use PyTuple_Pack, which is probably faster than Py_BuildValue
eric-wieser Dec 21, 2016
3a0ad85
MAINT: Overhaul function to try and increase speed
eric-wieser Apr 7, 2017
8c4e556
BUG: Fix reference counting
eric-wieser Apr 9, 2017
5d9315c
STY: Fix comment capitalization and format
eric-wieser Apr 9, 2017
966b2c7
fixup! BUG: Fix reference counting
eric-wieser Apr 9, 2017
9832f05
fixup! MAINT: Overhaul function to try and increase speed
eric-wieser Apr 9, 2017
68bad6a
MAINT: Improve comments, simplify handling of tuple subclasses
eric-wieser Jul 16, 2017
c587963
fixup! MAINT: Improve comments, simplify handling of tuple subclasses
eric-wieser Jul 16, 2017
105e0b4
fixup! MAINT: Improve comments, simplify handling of tuple subclasses
eric-wieser Jul 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
fixup! MAINT: Improve comments, simplify handling of tuple subclasses
  • Loading branch information
@eric-wieser
eric-wieser committed Jul 16, 2017
commit 105e0b4939a02ceda46ceb92f2978383cdc2b377
2 changes: 1 addition & 1 deletion numpy/core/src/multiarray/mapping.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ unpack_indices(PyObject *index, PyObject **result, npy_intp result_n)
* allocation, but doesn't need to be a fast path anyway
*/
if (PyTuple_Check(index)) {
Copy link
Member Author
@eric-wieser eric-wieser Apr 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The code here is subtly different to what we had before.

Calling PySequence_Tuple(index) invokes __iter__, whereas this invokes __getitem__.

So tuple subclasses that implement those methods inconsistently now behave differently. For instance:

class Plus1Tuple(tuple):
    def __getitem__(self, i):
        return super().__getitem__(i) + 1
    # oops, we forgot `__iter__`, and inherited `tuple.__iter__`, which
    # does not fall back on __getitem__

gives:

  • Before: a[PlusOneTuple([1, 2, 3])] → a[1, 2, 3] (!)
  • After: a[PlusOneTuple([1, 2, 3])] → a[2, 3, 4]

Copy link
Member
@seberg seberg Jul 16, 2017

Choose a reason for hiding this comment

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

Can't you just remove this whole block and replace it with commit_to_unpack=1?

Copy link
Member
@seberg seberg Jul 16, 2017

Choose a reason for hiding this comment

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

OK, plus a check for too many indices.

Copy link
Member
@seberg seberg Jul 16, 2017

Choose a reason for hiding this comment

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

I am fine with the changed behaviour I think, a tuple subclass should really be OK even with just using PyTuple_GET_ITEM to be honest, otherwise it should be considered badly broken.

Copy link
Member Author
@eric-wieser eric-wieser Jul 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps we should just bite the bullet here and call tuple(tuplesubclass), since efficiency isn't important for this weird case

Copy link
Member
@seberg seberg Jul 16, 2017

Choose a reason for hiding this comment

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

Yea, I suppose we might just as well put whatever was there before, it won't make the code any uglier and speed really does not matter. But I no need to add a test or so (or if, put a comment that it is fine to break it). This is too strange to really worry about.

Copy link
Member Author
@eric-wieser eric-wieser Jul 16, 2017

Choose a reason for hiding this comment

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

Ok, changed to call tuple(tuplesubclass), which makes our life a lot easier

PyTupleObject *tup = PySequence_Tuple(index);
PyTupleObject *tup = (PyTupleObject *) PySequence_Tuple(index);
if (tup == NULL) {
return -1;
}
Expand Down
0