WIP: Actually try deprecating non-tuple nd-indices by seberg · Pull Request #4434 · numpy/numpy

seberg · 2014-03-04T16:43:07Z

Just here for discussion. I am not sure it isn't too brutal. Also there is a test failure right now, which I think is probably another bug...

njsmith · 2014-03-04T17:59:41Z

How about: "using a non-tuple sequence for multidimensional indexing is deprecated; use arr[tuple(foo)] instead of arr[foo]"

Sure. Will also add a test. Not sure about the failure, I think gradient possibly doesn't work with multidimensional datetime input, but I find it a bit puzzling that advanced indexing worked and normal doesn't. It might have something to do with arr[[0]] returning an array while arr[0,] returns a scalar...

njsmith · 2014-03-04T18:01:11Z

I like it, and no-one objected on the list. Not sure what's up with the test failure, but otherwise LGTM...

njsmith · 2014-03-04T18:02:48Z

Though, it would be good to also have:

a test
a mention in the release notes
a deprecation bug to remind us later

argriffing · 2014-03-04T19:17:25Z

This PR would change the behavior of this code?

>>> a = np.arange(10).reshape(5, 2)
>>> a
array([[0, 1],
       [2, 3],
       [4, 5],
       [6, 7],
       [8, 9]])
>>> a[(2, 1)]
5
>>> a[np.array([2, 1])]                                                         
array([[4, 5],
       [2, 3]])

njsmith · 2014-03-04T19:23:02Z

No, both of those are unaffected. All it does changes is to start issuing a
DeprecationWarning for a[[2, 1]], which right now acts like a[(2, 1)]
instead of a[np.array([2, 1])]. We want it to eventually act like
np.array([2, 1]).

On Tue, Mar 4, 2014 at 7:17 PM, argriffing [email protected] wrote:

This PR would change the behavior of this code?

a = np.arange(10).reshape(5, 2)
a
array([[0, 1],
[2, 3],
[4, 5],
[6, 7],
[8, 9]])
a[(2, 1)]
5
a[np.array([2, 1])]
array([[4, 5],
[2, 3]])

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/4434#issuecomment-36662374
.

Nathaniel J. Smith
Postdoctoral researcher - Informatics - University of Edinburgh
http://vorpus.org

seberg · 2014-03-05T13:01:37Z

Updated (made it a futurewarning, too). But the datetime gradient issue is open, so...

seberg · 2014-08-31T19:40:46Z

This should work now (though it is not unlikely that there is a tuple call misisng somewhere). Doesn't necessarily make it an easier decision :)

seberg · 2014-09-15T09:04:24Z

Note, this also changes the behaviour of tuple subclasses (or warns about a change). Tuple subclasses apparently were always handled like tuples before. I don't care much either way but the new test will need fixing (tests would fail if rerun currently), and before we put this in, we need to decide on it.
Should probably run some tests suits as a next step to see how much of scipy, etc. are broken by this ;).

homu · 2015-06-21T20:12:39Z

☔ The latest upstream changes (presumably #5990) made this pull request unmergeable. Please resolve the merge conflicts.

seberg · 2015-07-08T21:45:12Z

I have updated this (resolved merge conflict. Obviously it is not ready, since the 1.9. release notes are probably not the right place ;), but could discuss this again.

homu · 2015-07-25T19:58:29Z

☔ The latest upstream changes (presumably #6047) made this pull request unmergeable. Please resolve the merge conflicts.

Such indexing has problem with ambiguouity for example whether `arr[[[0], [1]]]` is `arr[[0], [1]]` or `arr[asarray([[0], [1]])]` Adds the tuple cast where it is necessary (at the time of writing this, it is not unlikely that there are some hidden ones missing).

seberg · 2015-12-02T15:09:44Z

Updated this a bit, but getting some weirder errors, weeding out everything, especially in the masked array stuff seems a bit tricky. Anyone who wants to pick it up, be my guest, it is getting a bit annoying :).

shoyer · 2016-01-14T22:01:24Z

I would write: arr[(slice(None),)] or simply arr[slice(None),]

Hehe, yeah, though that would somewhat defeat the purpose ;). should make the example longer... Anyway, doesn't look like 1.11 change.
Hmmm, reading my last comment, part of me wonders why the masked arrays are always so tricky when doing indexing changed.

EDIT: should make the list explicit.

homu · 2016-01-28T16:05:26Z

☔ The latest upstream changes (presumably #7134) made this pull request unmergeable. Please resolve the merge conflicts.

Recommended type for nd-indexing is a tuple. See numpy#4434

eric-wieser · 2016-11-15T13:25:54Z

+            }
+            else if (make_tuple == 1) {
+                if (DEPRECATE_FUTUREWARNING(
+                        "multi dimensional indexing tuple was not base class. "


Is there a good reason to deprecate this? Allowing subclasses of tuple doesn't seem unreasonable, as there's still no ambiguity

No, probably not, I probably wasn't sure and just thought I will go with the extreme version and make it less extreme from there ;).

eric-wieser · 2017-04-06T23:58:23Z

@seberg: Can you make this editable so that I can pick it up?

eric-wieser · 2017-04-07T00:05:38Z

    # just set y equal to the the array `f`.
    if f.dtype.char in ["M", "m"]:
-        y = f.view('int64')
+        y = f.view(otype)


What happened here?

there is some weird problems in gradient that made it work only if it did the wrong, fancy, indexing. I guess this maybe somehow fixed/worked around it, it is possible that the commit message explains more, but likely not.

Would not rule out that other datetime fixes in the meantime may have made this better, or that changing things at another place would be more correct.

seberg · 2017-04-07T15:01:10Z

Anyway, the problem with this is less the work to make it work, but more making the decision to do this very noisy deprecation which affects quite a lot of things.

eric-wieser · 2017-04-07T15:26:00Z

One thing we could do right now is merge all the fixes that make numpy only ever index with tuples internally. That way subclassers don't need to deal with this "feature" if all they care about is numpy itself

seberg · 2017-04-07T16:21:55Z

Yeah, I guess so, though unless you add a compile time option to give the warning or so (and use it during testing), there will be no guarantee for it in the future and its useless. There is a bit of a downer, that manual tuple conversion is slower (probably because python has kwarg parsing overhead or so), but there is no point being annoyed by it I suppose.

eric-wieser · 2017-09-14T04:41:17Z

@seberg, I went ahead and rebased this at #9686 (although I accidentally pushed over this PR first...)

charris · 2017-09-25T00:37:43Z

Should we close #4434?

seberg · 2017-09-25T08:52:27Z

Sure, why not, could always reopen anyway if there were a reason for it.

njsmith reviewed Mar 4, 2014
View reviewed changes

seberg mentioned this pull request Mar 5, 2014

Timedelta does not allow assignment from float #4440

Open

seberg mentioned this pull request Aug 1, 2014

Inconsistent advanced indexing behavior #4940

Closed

ewmoore mentioned this pull request Aug 12, 2014

Added an 'elementary' function. #4573

Closed

seberg force-pushed the force-tuple branch from 2e9808f to 48da7a6 Compare August 31, 2014 19:34

rgommers added component: numpy._core 54 - Needs decision labels Mar 8, 2015

seberg force-pushed the force-tuple branch from 48da7a6 to c8b2ae7 Compare July 8, 2015 21:43

seberg mentioned this pull request Nov 10, 2015

Inconsistent indexing result depending on argument type #6564

Closed

seberg force-pushed the force-tuple branch from c8b2ae7 to 7df5ab8 Compare December 2, 2015 00:03

seberg force-pushed the force-tuple branch from 7df5ab8 to 81aa766 Compare December 2, 2015 08:15

shoyer reviewed Jan 14, 2016
View reviewed changes

ahaldane mentioned this pull request May 1, 2016

BUG: masked-array median of 1d array should be scalar #7592

Closed

AmitAronovitch added a commit to AmitAronovitch/numpy that referenced this pull request May 21, 2016

STY: ma.extras.median: avoid indexing with list

a4cc361

Recommended type for nd-indexing is a tuple. See numpy#4434

charris pushed a commit to charris/numpy that referenced this pull request May 22, 2016

STY: ma.extras.median: avoid indexing with list

1f80666

Recommended type for nd-indexing is a tuple. See numpy#4434

seberg mentioned this pull request Aug 5, 2016

Fancy slicing inconsistent behaviour with lists of lists and arrays of arrays #7909

Closed

seberg mentioned this pull request Nov 15, 2016

ENH: Add index_tricks.as_index_tuple #8276

Closed

eric-wieser mentioned this pull request Nov 15, 2016

MAINT: Make the refactor suggested in prepare_index #8278

Merged

eric-wieser reviewed Nov 15, 2016

View reviewed changes

eric-wieser reviewed Apr 7, 2017

View reviewed changes

eric-wieser mentioned this pull request Apr 10, 2017

ENH: add padding options to diff #8206

Merged

eric-wieser mentioned this pull request Sep 14, 2017

DEP: Deprecate non-tuple nd-indices #9686

Merged

eric-wieser force-pushed the force-tuple branch 2 times, most recently from 175e0f1 to 81aa766 Compare September 14, 2017 04:43

seberg closed this Sep 25, 2017

Conversation

seberg commented Mar 4, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

njsmith commented Mar 4, 2014

Uh oh!

njsmith commented Mar 4, 2014

Uh oh!

argriffing commented Mar 4, 2014

Uh oh!

njsmith commented Mar 4, 2014

Uh oh!

seberg commented Mar 5, 2014

Uh oh!

seberg commented Aug 31, 2014

Uh oh!

seberg commented Sep 15, 2014

Uh oh!

homu commented Jun 21, 2015

Uh oh!

seberg commented Jul 8, 2015

Uh oh!

homu commented Jul 25, 2015

Uh oh!

seberg commented Dec 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

homu commented Jan 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser commented Apr 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seberg commented Apr 7, 2017

Uh oh!

eric-wieser commented Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seberg commented Apr 7, 2017

Uh oh!

eric-wieser commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

charris commented Sep 25, 2017

Uh oh!

seberg commented Sep 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

eric-wieser commented Apr 7, 2017 •

edited

Loading

eric-wieser commented Sep 14, 2017 •

edited

Loading