DEP: Drop deprecated boolean indexing behavior and update to new. by charris · Pull Request #8312 · numpy/numpy

charris · 2016-11-25T05:54:56Z

This affects mostly old boolean indexing behavior that was deprecated in NumPy 1.9. At the time the new behavior was causing errors in a number of downstream projects, hopefully that has been taken care of.

charris · 2016-11-25T06:02:20Z

Copied from post from @seberg.

OK, this seems to do the trick:


@@ -441,16 +441,6 @@ prepare_index(PyArrayObject *self, PyObject *index,
 
             if (PyArray_NDIM(arr) == 0) {
                 /*
-                 * TODO, WARNING: This code block cannot be used due to
-                 *                FutureWarnings at this time. So instead
-                 *                just raise an IndexError.
-                 */
-                PyErr_SetString(PyExc_IndexError,
-                        "in the future, 0-d boolean arrays will be "
-                        "interpreted as a valid boolean index");
-                Py_DECREF((PyObject *)arr);
-                goto failed_building_indices;
-                /*
                  * This can actually be well defined. A new axis is added,
                  * but at the same time no axis is "used". So if we have True,
                  * we add a new axis (a bit like with np.newaxis). If it is
@@ -459,7 +449,6 @@ prepare_index(PyArrayObject *self, PyObject *index,
 
                 index_type |= HAS_FANCY;
                 indices[curr_idx].type = HAS_0D_BOOL;
-                indices[curr_idx].value = 1;
 
                 /* TODO: This can't fail, right? Is there a faster way? */
                 if (PyObject_IsTrue((PyObject *)arr)) {
@@ -468,6 +457,7 @@ prepare_index(PyArrayObject *self, PyObject *index,
                 else {
                     n = 0;
                 }
+                indices[curr_idx].value = n;
                 indices[curr_idx].object = PyArray_Zeros(1, &n,
                                             PyArray_DescrFromType(NPY_INTP), 0);
                 Py_DECREF(arr);
@@ -2450,6 +2440,10 @@ mapiter_fill_info(PyArrayMapIterObject *mit, npy_index_info *indices,
             mit->fancy_dims[j] = 1;
             /* Does not exist */
             mit->iteraxes[j++] = -1;
+            if (mit->dimensions[mit->nd_fancy-1] > 1) {
+                goto broadcast_error;
+            }
+            mit->dimensions[mit->nd_fancy-1] *= indices[i].value;
         }
 
         /* advance curr_dim for non-fancy indices */
@@ -2487,7 +2481,7 @@ mapiter_fill_info(PyArrayMapIterObject *mit, npy_index_info *indices,
     }
 
     for (i = 0; i < index_num; i++) {
-        if (indices[i].type != HAS_FANCY) {
+        if (!(indices[i].type & HAS_FANCY)) {
             continue;
         }
         tmp = convert_shape_to_string(

charris · 2016-11-27T02:21:56Z

@seberg, this test fails. Should it?

Sure, the automated test, could probably be fixed, but we could even think about just removing this part since the other tests are more comprehensive nowadays.

I had a glance at the code, I think I can probably fix it with two small modifications.

I thought about removing it, not least because it is complicated.

Good with me, it was very useful when there were not many tests, now its not awfully useful (hopefully). How about just removing this one case and maybe adding a comment that there is likely no point in supporting it indefinitely (could also move to its own file?). So that we can basically remove it whenever it should be too much trouble to tweak it?

Just am not sure if I like removing the test completely without a strong need. Who knows maybe it does test some things otherwise not tested, indexing is quite branched with many special cases.

charris · 2016-11-27T02:22:26Z

@seberg these two tests fail, should they?

The whole test should probably be replaced with new tests for the new behaviour. Sounds like it tests old behaviour

Just wondering if the tested behavior was no longer valid. I seems that now booleans and integers can be mixed.

charris · 2016-11-30T20:58:24Z

OK, I've removed the failing tests. This should be ready to go unless more tests are desired.

seberg · 2016-11-30T21:26:18Z

Should probably add tests for some of the wonkier behaviour like :/

arr = np.ones((2, 3, 4))
arr[True, [0, 1], True, True, [1], [[2]]].shape == (1, 2)  # I think, who knows ;)
arr[False, True, ...].shape == (0, 2, 3, 4)
arr[False, [0, 1], ...]  # IndexError, can't broadcast

I can try to write more some time.

seberg · 2016-11-30T21:27:14Z

Obviously all of those examples don't make any real sense at all, but they are valid combinations of the nonsense boolean indexing rules.

charris · 2016-11-30T22:13:27Z

OK, I added a def test_boolean_indexing_weirdness(): function. Your second example throws an IndexError.

seberg · 2016-11-30T22:29:12Z

That test I guessed the result wrong... If its a True, its like a dimension of 1 and that means it can broadcast:

if ((indices[i].value == 0) && (mit->dimensions[mit->nd_fancy - 1] > 1)) {

should fix it. Wonky stuff mixing boolean arrays with other indexes....

I guess the truth is, it is completely insane to do it anyway, but at least in my head there is a set of rules you can apply to get the result ;).

Oh, I gave the wrong one, this is the one that fails:

a[True, [0, 1], True, True, [1], [[2]]]

Still need the fix?

Yeah, I had pulled the PR to see the error message.

charris · 2016-12-03T03:04:49Z

OK, updated.

seberg · 2016-12-03T15:37:51Z

LGTM. Its good to put this in early, who knows what problems might crop up. If you want I can try to write more tests some time, though could do that also after this is put in (could open an issue in that case I guess). Go ahead and put it in (or I can do it).

seberg · 2016-12-03T15:38:25Z

Btw. thanks for taking care of all of this maintenance work!

This affects mostly old boolean indexing behavior that was deprecated in NumPy 1.9. At the time the new behavior was causing errors in a number of downstream projects, hopefully that has been taken care of.

charris · 2016-12-03T18:49:26Z

I've updated the release notes and will put this in. Thanks for the review. And of course more tests, and perhaps clarification in the release notes, are welcome ;)

charris added 07 - Deprecation 03 - Maintenance component: numpy._core labels Nov 25, 2016

charris added this to the 1.13.0 release milestone Nov 25, 2016

charris mentioned this pull request Nov 25, 2016

DEP: Handle expired deprecations. #8297

Merged

charris changed the title ~~WIP: Change expired index deprecations.~~ WIP: Deal with expired index deprecations. Nov 27, 2016

charris force-pushed the index-deprecation branch from c13e8a4 to 1265f86 Compare November 27, 2016 02:21

charris commented Nov 27, 2016

View reviewed changes

charris force-pushed the index-deprecation branch from 1265f86 to a92e6ed Compare November 30, 2016 20:56

charris changed the title ~~WIP: Deal with expired index deprecations.~~ DEP: Drop deprecated boolean indexing behavior and update to new. Nov 30, 2016

charris force-pushed the index-deprecation branch from a92e6ed to b005be2 Compare November 30, 2016 22:12

seberg reviewed Nov 30, 2016

View reviewed changes

charris force-pushed the index-deprecation branch from b005be2 to 2ac4537 Compare December 3, 2016 03:04

DEP: Drop deprecated boolean indexing behavior and update to new.

b80bcee

This affects mostly old boolean indexing behavior that was deprecated in NumPy 1.9. At the time the new behavior was causing errors in a number of downstream projects, hopefully that has been taken care of.

charris force-pushed the index-deprecation branch from 2ac4537 to b80bcee Compare December 3, 2016 18:47

charris merged commit 6429c6f into numpy:master Dec 3, 2016

charris deleted the index-deprecation branch December 3, 2016 19:29

halkk990 mentioned this pull request Feb 25, 2024

AA halkk990/docs#1

Open

Uh oh!

Uh oh!

Conversation

charris commented Nov 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

charris commented Nov 25, 2016

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charris commented Nov 30, 2016

Uh oh!

seberg commented Nov 30, 2016

Uh oh!

seberg commented Nov 30, 2016

Uh oh!

charris commented Nov 30, 2016

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!

Choose a reason for hiding this comment

Uh oh!

charris commented Dec 3, 2016

Uh oh!

seberg commented Dec 3, 2016

Uh oh!

seberg commented Dec 3, 2016

Uh oh!

charris commented Dec 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

charris commented Nov 25, 2016 •

edited

Loading