BUG: Fix iteration over reversed subspaces in mapiter_@name@ #8284

rainwoodman · 2016-11-17T00:10:23Z

This PR will fix 8264. It is a WIP.

rainwoodman · 2016-11-17T00:38:03Z

numpy/core/src/multiarray/lowlevel_strided_loops.c.src

@@ -1707,10 +1712,22 @@ mapiter_@name@(PyArrayMapIterObject *mit)
                }

                /*
-                 * Resetting is slow, so skip if the subspace iteration has
-                 * only a single inner loop.
+                 * Resetting is slow, so skip if the subspace iteration is


@seberg I am not sure if the prediction works correctly for higher dimensional subspace.
Saying the fastest dimension has strides == +-itemsize, but the last-fastest dimension doesn't. Not sure if I can quickly come up with a test case.

Let me try to explain nditer a bit, I think it will make things clear why it should work. I am not sure it might help but...

The advanced iterator will always do some "iteration", after each of these there is an inner loop optimization. This inner loop can be iterated trivially by a single stride. Now in principle the inner stride you need can change of course.
This is why there is the check that the innerloop is actually the complete iteration. If the innerloop spans the whole iteration there is no reason for the innermost stride to change later on (assuming no buffering, buffering might add weirdness). The only reason why this could happen is if the strided layout is different for different "subspaces/inner iteration positions", but that is not compatible to numpy's memory layout.
All in all, even if you change the iterator to do a different iteration order, these things should still work as far as I can tell.

I think I get your point. It sounds like the subspace is compatible to 1d -- I mean .ravel() can returns a reference not a copy. Is there a name in numpy for this property? ravellable?

Yes, precisely. And no, I don't know a nice word for that property, since its not contiguous....

We probably want to give it a name to properly refer to it. I see code appears to be dealing with this in numpy.reshape too.

rainwoodman · 2016-11-17T00:39:54Z

Linking the PR to #8264

seberg · 2016-11-18T09:35:24Z

numpy/core/src/multiarray/lowlevel_strided_loops.c.src

@@ -1724,6 +1741,10 @@ mapiter_@name@(PyArrayMapIterObject *mit)
                        NPY_AUXDATA_FREE(transferdata);
                        return -1;
                    }
+                    if(subspace_ptrs[0] == self_ptr &&
+                       subspace_ptrs[1] == mit->extra_op_ptrs[0]) {
+                        skip <<= 1;


Not sure why two iterations without checking closer, but can't the bit shift overflow to 0?

For the second part of your question: shifting stops after skip == 4 -- if skip started with 1. Otherwise skip started with 0 and shifting never do anything. So it won't overflow.

The first part of your question I don't really understand.

skip = 1(fit the offset), 2 (twice, fit the stride), 4(the linear model is fitted, use predictions from this point on)

Still don't get it. Why stride? This is purely an offset in the subspace (from the point of view of the outer iteration), the inner stride is already known? In fact, even if they were different (and skip was 1) you could save the difference and just apply it.

I think you are right. I changed it to 1 and all tests passes too. The offset from selfptr and the first iteration is always a constant, and we can calculate it on the first iteration.

seberg

Looks good. It seems that defining reset_offsets (or maybe we can even think of a new name) is pretty understandable. It would be cool if we can make the comments a bit clearer, I tried to suggest/drop some words, maybe you can think of something nicer too.

seberg · 2016-11-19T14:47:41Z

numpy/core/src/multiarray/lowlevel_strided_loops.c.src

@@ -1632,7 +1632,8 @@ mapiter_@name@(PyArrayMapIterObject *mit)
        char *subspace_baseptrs[2];
        char **subspace_ptrs = mit->subspace_ptrs;
        npy_intp *subspace_strides = mit->subspace_strides;
-        int skip = 0;
+        int is_reset_trivial = 1;
+        npy_intp reset_offsets[2];


Should be initialized to avoid compile time warning.

Is it proper numpy C to initialize with {}?

seberg · 2016-11-19T14:51:48Z

numpy/core/src/multiarray/lowlevel_strided_loops.c.src

+        if (*counter != PyArray_SIZE(mit->subspace)) {
+           /*
+            * if the subspace iterator skips, we cannot avoid resetting
+            */


Remembered the word, its "trivially iterable" (at least within much of our code). Just trying to make it a bit clearer (only some thougths): If the subspace iterator is not trivially iterable (so not in a single stride), it has to be reset to the correct start point in every outer iteration. If it is trivially iterable we can avoid using it alltogether (the actual loop does nothing).

seberg · 2016-11-19T14:52:47Z

numpy/core/src/multiarray/lowlevel_strided_loops.c.src

+                 * because the internal iteration of each external iterations,
+                 * share the same structure, if we are correct once we know
+                 * future iterations we are always correct.
+                 *


No need for extra blank line. The comment is outdated with this version of the code.

seberg · 2016-11-19T14:54:33Z

numpy/core/src/multiarray/lowlevel_strided_loops.c.src

+                        reset_offsets[0] = subspace_ptrs[0] - self_ptr;
+                        reset_offsets[1] = subspace_ptrs[1] - mit->extra_op_ptrs[0];
+                        /* use the faster adjustment */
+                        is_reset_trivial ++;


Nitpick: I wouldn't put the space, though I guess it really does not matter, nor am I sure its part of any style guideline.

seberg · 2016-11-19T14:58:33Z

numpy/core/src/multiarray/lowlevel_strided_loops.c.src

-                    subspace_ptrs[0] = self_ptr;
-                    subspace_ptrs[1] = mit->extra_op_ptrs[0];
+                    /*
+                     * will avoid resetting if the reset is trival.


Possibly could try to expand this a little along the same lines as above, especially an example might help. If the inner/subspace iterator is trivially iterable, we still may need to calculate the correct starting point of the iteration. The offset is typically zero unless reversing the iteration is more efficient (originally a negative stride) in reversed order in which case this is the offset to the last item.

seberg · 2016-11-19T14:59:05Z

numpy/core/tests/test_indexing.py

@@ -497,6 +497,14 @@ def test_indexing_array_weird_strides(self):
        zind = np.zeros(4, dtype=np.intp)
        assert_array_equal(x2[ind, zind], x2[ind.copy(), zind])

+    def test_indexing_array_weird_strides_8264(self):


Does not matter, but maybe negative strides explicitly is nicer ;).

seberg · 2016-11-21T11:35:31Z

Thanks, looks all good. If you can squash it, I will put this in.

rainwoodman · 2016-11-21T19:48:30Z

squashed.

charris · 2016-11-21T20:29:26Z

The summary line could be improved, something like

BUG: Fix iteration over subspaces in mapiter_@name@.

Then put the Fixes #8264 at the bottom of the commit message,

charris · 2016-11-21T20:31:09Z

Also more explanation of what the problem was, e.g., visible failure. Currently is all "what" and no "why".

As stated in numpy#8264, before this patch numpy crashes when the subspace of iterator has negative strides on the faster resetting branch for trivially iterable subspaces in mapiter_@name@. Noticing the offset between ptr and first item in subspace is constant, we calculate the offset from the first iteration and use it onwards. Fixes numpy#8264

rainwoodman · 2016-11-22T00:14:06Z

Indeed. Poorly written. How does it look now?

On Mon, Nov 21, 2016 at 12:31 PM, Charles Harris notifications@github.com
wrote:

Also more explanation of what the problem was, e.g., visible failure.
Currently is all what and no why.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#8284 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAIbTAmuNUEZ2gf-ehxnImJ3UyvFaL0Aks5rAf-RgaJpZM4K0py5
.

charris · 2016-11-22T16:17:21Z

LGTM. I'll let @seberg finish it up.

seberg · 2016-11-22T18:39:02Z

LGTM, thanks a lot @rainwoodman.

rainwoodman · 2016-11-22T22:07:49Z

my pleasure!

rainwoodman commented Nov 17, 2016

View reviewed changes

charris added 00 - Bug component: numpy._core labels Nov 17, 2016

rainwoodman force-pushed the fix-8264 branch from 2946106 to 2b5d473 Compare November 17, 2016 21:49

seberg reviewed Nov 18, 2016

View reviewed changes

seberg reviewed Nov 19, 2016

View reviewed changes

charris added this to the 1.12.0 release milestone Nov 20, 2016

rainwoodman force-pushed the fix-8264 branch from c76044d to 4cf14dc Compare November 21, 2016 19:48

rainwoodman force-pushed the fix-8264 branch from 4cf14dc to cce86d6 Compare November 22, 2016 00:13

charris changed the title ~~WIP: fix #8264.~~ BUG: Fix iteration over reversed subspaces in mapiter_@name@ Nov 22, 2016

seberg merged commit ce6d3ff into numpy:master Nov 22, 2016

charris mentioned this pull request Nov 22, 2016

BUG: Fix iteration over reversed subspaces in mapiter_@name@. #8296

Merged

charris removed this from the 1.12.0 release milestone Nov 22, 2016

rainwoodman deleted the fix-8264 branch November 22, 2016 22:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fix iteration over reversed subspaces in mapiter_@name@ #8284

BUG: Fix iteration over reversed subspaces in mapiter_@name@ #8284

rainwoodman commented Nov 17, 2016

rainwoodman Nov 17, 2016

seberg Nov 17, 2016

rainwoodman Nov 17, 2016

seberg Nov 18, 2016

rainwoodman Nov 18, 2016

rainwoodman commented Nov 17, 2016

seberg Nov 18, 2016

rainwoodman Nov 18, 2016

rainwoodman Nov 18, 2016

seberg Nov 18, 2016

rainwoodman Nov 19, 2016

seberg left a comment

seberg Nov 19, 2016

rainwoodman Nov 20, 2016

seberg Nov 19, 2016

seberg Nov 19, 2016

rainwoodman Nov 20, 2016

seberg Nov 19, 2016

seberg Nov 19, 2016

seberg Nov 19, 2016

seberg commented Nov 21, 2016

rainwoodman commented Nov 21, 2016

charris commented Nov 21, 2016

charris commented Nov 21, 2016 •

edited

rainwoodman commented Nov 22, 2016

charris commented Nov 22, 2016

seberg commented Nov 22, 2016

rainwoodman commented Nov 22, 2016

BUG: Fix iteration over reversed subspaces in mapiter_@name@ #8284

BUG: Fix iteration over reversed subspaces in mapiter_@name@ #8284

Conversation

rainwoodman commented Nov 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainwoodman commented Nov 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seberg commented Nov 21, 2016

rainwoodman commented Nov 21, 2016

charris commented Nov 21, 2016

charris commented Nov 21, 2016 • edited

rainwoodman commented Nov 22, 2016

charris commented Nov 22, 2016

seberg commented Nov 22, 2016

rainwoodman commented Nov 22, 2016

charris commented Nov 21, 2016 •

edited