Skip to content

MAINT: Stop using non-tuple indices internally - #10618

Merged
mhvk merged 1 commit into
numpy:masterfrom
eric-wieser:avoid-nontuple-indices
Mar 15, 2018
Merged

mhvk merged 1 commit into
numpy:masterfrom
eric-wieser:avoid-nontuple-indices

Conversation

@eric-wieser

Copy link
Copy Markdown
Member

By not using this type of indexing, it becomes easier for subclasses to override indexing in a way that works correctly with numpy functions.

These locations were found by deprecating the behavior in question, which is deliberately not part of this commit

Split out from #9686. @mhvk requested further cleanup there, but I'd rather just get the fix in first with a minimal diff

By not using this type of indexing, it becomes easier for subclasses to override indexing in a way that works correctly with numpy functions.

These locations were found by deprecating the behavior in question, which is deliberately not part of this commit
@mhvk

mhvk commented Feb 17, 2018

Copy link
Copy Markdown
Contributor

I think it is good if our code serves in part as showing best practice; in that respect, I'd prefer to go for tuples directly where possible rather than from a list. On the other hand, there is only so much time in a day, so I'm also happy with going with this as is.

@charris

charris commented Feb 17, 2018

Copy link
Copy Markdown
Member

@mhvk If you are happy with it, feel free to merge ;)

@eric-wieser

eric-wieser commented Feb 20, 2018 •

Copy link
Copy Markdown
Member Author

@mhvk: My thinking is that this patch is an externally visible change for subclasses, (should maybe be ENH?), but what you propose is just internal cleanup - I'd like to keep the two separate, if possible.

Additionally, I think we can make some big cleanups to gradient now that we're not getting any efficiency saving by reusing the same lists - something that would do better as a standalone MAINT pr so as not to produce a large diff to bisect in the distant future

@mhvk

mhvk commented Mar 15, 2018

Copy link
Copy Markdown
Contributor

Going back through issues, I saw this again. It really is fine to proceed in steps, so I'm happy to merge. Just in case: @eric-wieser - no change needed here, correct?

@charris

charris commented Mar 15, 2018

Copy link
Copy Markdown
Member

What bothers me is how bloated the code has become. Is there a plan that makes it prettier?

@eric-wieser

Copy link
Copy Markdown
Member Author

@mhvk: Yep, I'm happy with this as is

@charris: I plan to make gradient prettier, but there's not much we can do with the rest.

@mhvk

mhvk commented Mar 15, 2018

Copy link
Copy Markdown
Contributor

@charris - I think the bloatiness is not necessary if one were to start from defining things as tuples instead of lists and not explicitly rely on mutability. But that is a much larger effort and this PR does serve the purpose of ensuring we do not rely on something we hope to deprecate -- as ideally in the end lists always are interpreted as are arrays of indices.

@charris

charris commented Mar 15, 2018

Copy link
Copy Markdown
Member

@mhvk Go ahead in put this in if you think it ready. I can't think of any easy way to improve on things.

@mhvk
mhvk merged commit 01541f2 into numpy:master Mar 15, 2018
@eric-wieser

Copy link
Copy Markdown
Member Author

@mhvk: This is not just about working towards deprecating it - it also makes it easier for subclasses to overload getitem, and not have to implement list → tuple conversion heuristics to make the functions touched here work

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.

3 participants