BF: stride_tricks produces unbroadcastable array #358

effigies · 2015-10-15T01:42:03Z

Hopefully this will fix the Travis errors.

matthew-brett · 2015-10-15T01:46:41Z

nibabel/parrec.py

@@ -609,7 +609,7 @@ def __getitem__(self, slicer):
                                 'F')
        # Broadcast scaling to shape of original data
        slopes, inters = self._slice_scaling
-        fake_data = strided_scalar(self._shape)


Ah - no - that was deliberate to save memory - what is the error?

I figured that was the case. The error is here: https://travis-ci.org/nipy/nibabel/jobs/85392218#L977

Seems to be an immutability issue, but hard to diagnose, so this PR was me trying to use Travis to figure out if my guess was right.

Ouch. I guess that's numpy 1.10.

Yeah. Does it make sense to try to find another low-memory option, here, or is this something that needs to be addressed in numpy? And in the meantime, should we skip this test on numpy >= 1.10, use np.zeros, or put up with failing builds?

I think you just need to create a new array, as self._shape is probably read-only. Perhaps

fake_data = strided_scalar(self._shape.copy())

will work.

Just tried. self._shape is a tuple, so that doesn't work. (Failed to try the right tests, locally. Want test_proxy_api, if you're playing along at home.)

This works:
_, slopes, inters = np.broadcast_arrays(fake_data.copy(), slopes.copy(), inters.copy())

Just gotta narrow down from there.

On Wed, Oct 14, 2015 at 7:30 PM, Chris Markiewicz [email protected]
wrote:

In nibabel/parrec.py
#358 (comment):

@@ -609,7 +609,7 @@ def getitem(self, slicer):
'F')
# Broadcast scaling to shape of original data
slopes, inters = self._slice_scaling

fake_data = strided_scalar(self._shape)

Just tried. self._shape is a tuple, so that doesn't work. (Failed to try
the right tests, locally. Want test_proxy_api, if you're playing along at
home.)

—
Reply to this email directly or view it on GitHub
https://github.com/nipy/nibabel/pull/358/files#r42077733.

Yup, but that creates a writeable array. Unsetting the writeable flag was the needed trick.

effigies · 2015-10-15T02:48:52Z

Of course Travis would pick now to start failing to even get to the tests...

Anyway, what we're looking at is this line which seems like it can be skipped by setting the fake_data.flags.writeable flag to False. We can either do this in PARRECArrayProxy.__getitem__ as I show here or in strided_scalar. It seems safe to do the latter, since the array should never be writable, but figured I'd (a) check this works before digging around farther afield (b) get your thoughts.

Edit: Okay, one of the previously failing checks passed, so it looks like we can move on this.

effigies · 2015-10-15T03:05:31Z

The remaining AppVeyor error is completely different, though possibly numpy 1.10 related. Seems to be 32-bit Windows specific, and out of scope.

If the Travis builds succeed, I plan to just ignore that failure for Ben's PRs.

matthew-brett · 2015-10-15T04:35:57Z

nibabel/parrec.py

@@ -610,6 +610,7 @@ def __getitem__(self, slicer):
        # Broadcast scaling to shape of original data
        slopes, inters = self._slice_scaling
        fake_data = strided_scalar(self._shape)
+        fake_data.flags.writeable = False


How about

# Create broadcast array using writeable memory for numpy 1.10 fake_data = strided_scalar(np.array(self._shape))

? Only because the writeable line looks a bit frightening.

I can try, but I really don't think the mutability of the shape tuple is relevant.

On October 15, 2015 12:35:57 AM EDT, Matthew Brett [email protected] wrote:

@@ -610,6 +610,7 @@ def getitem(self, slicer):
# Broadcast scaling to shape of original data
slopes, inters = self._slice_scaling
fake_data = strided_scalar(self._shape)

fake_data.flags.writeable = False

How about

# Create broadcast array using writeable memory for numpy 1.10 fake_data = strided_scalar(np.array(self._shape))

? Only because the writeable line looks a bit frightening.

Reply to this email directly or view it on GitHub:
https://github.com/nipy/nibabel/pull/358/files#r42082838

Chris Markiewicz

effigies · 2015-10-15T13:01:15Z

Tried a few strategies to create writeable arrays, and none of those work in 1.10. (See below for why I think that is.TLDR: It's not that fake_data is unwriteable but that when broadcast it creates an unwriteable array.) Now trying to make the shape tuple writeable, but since it's immediately cast to tuple, I don't anticipate improvement.

I think I have misstated the problem, so let me try my new understanding:

as_strided returns a writeable array. broadcast_arrays calls _broadcast_to on each array, and the fake_data array is the one it's getting hung up on, as demonstrated by 5576c23 (on the test that actually ran, at any rate). So when this check is called, it's because there is no opportunity to set readonly in broadcast_arrays, and fake_data.writeable == True, but for some reason result cannot be set writeable. (I suspect it's because np.nditer(..., op_flags['readonly'], ...) was used to construct result.)

It may be worthwhile to submit a PR, and we can use this as a test case, but I don't see anything on the nibabel end that will resolve this except by unsetting the writeable flag. I would be inclined to do this in strided_array itself, because it is simply a bunch of views on a scalar, so any changes to a single location change the entire array, which is almost certainly unwanted: if a new array with one memory address is wanted, then call strided_array again with the new scalar; if only one location is to be changed, a copy needs to be made anyway.

effigies · 2015-10-15T20:44:59Z

@matthew-brett Just a ping, since I want to get this in before moving on Ben's PRs.

Since I don't see much alternative, the current version just follows my above reasoning and modifies strided_array, adds immutability to its documentation, and updates tests (which specifically tested mutability). Let me know if you disagree with that reasoning and what you think the best way forward is.

Left commits so you can see the changes and their effects on tests. Will squash to a single commit if you give the go-ahead.

matthew-brett · 2015-10-16T19:41:41Z

nibabel/tests/test_fileslice.py

@@ -639,8 +639,10 @@ def test_strided_scalar():
        assert_equal(observed.shape, shape)
        assert_equal(observed.dtype, expected.dtype)
        assert_array_equal(observed.strides, 0)
-        observed[..., 0] = 99
-        assert_array_equal(observed, expected * 0 + 99)
+        def setval(x):


Add comment explaining reason for test?

matthew-brett · 2015-10-16T19:42:40Z

I think you're right, setting the flag is the only way. I put in a numpy bug report : numpy/numpy#6491

effigies · 2015-10-16T20:33:25Z

Added a comment, one more test, and squashed to a single commit.

bcipolli · 2015-10-16T20:59:47Z

nibabel/tests/test_fileslice.py

+        # Strided scalars are set as not writeable
+        # This addresses a numpy 1.10 breakage of broadcasting a strided
+        # array without resizing (see GitHub PR #358)
+        assert_false(x.flags.writeable)


@effigies this line is causing a test failure; x is not defined. Perhaps this should be inside setval?

Woops. s/x/observed/

effigies · 2015-10-16T21:38:24Z

Fixed that new test and Travis is passing.

matthew-brett · 2015-10-16T21:40:37Z

nibabel/fileslice.py

@@ -767,9 +767,12 @@ def strided_scalar(shape, scalar=0.):
    strided_arr : array
        Array of shape `shape` for which all values == `scalar`, built by
        setting all strides of `strided_arr` to 0, so the scalar is broadcast
-        out to the full array `shape`.
+        out to the full array `shape`. `strided_arr` is flagged as not
+        `writeable`.


Still feel that we need a comment here - otherwise it's not obvious why this is true.

What kind of comment are you thinking? Noting the numpy bug (with some level of detail, or reference to this PR) or the reasoning that setting one element of the array would set all (and thus is probably not the intended behavior)?

Just something like We set the array read-only to avoid a numpy error when broadcasting - see https://github.com/numpy/numpy/issues/6491.

Numpy 1.10 cannot broadcast a strided array that is set as writeable

matthew-brett · 2015-10-17T00:04:30Z

Thanks for your patience.

MRG: stride_tricks produces unbroadcastable array A probable bug in numpy 1.10.1 causes an error when trying to broadcast an array constructed using stride tricks. See : numpy/numpy#6491 for a full description. To work round, set the stride_tricks array as read-only, so that numpy does not try and set the output to writeable - that is the step that appears to cause the error.

effigies · 2015-10-17T00:07:42Z

Thanks!

MRG: stride_tricks produces unbroadcastable array A probable bug in numpy 1.10.1 causes an error when trying to broadcast an array constructed using stride tricks. See : numpy/numpy#6491 for a full description. To work round, set the stride_tricks array as read-only, so that numpy does not try and set the output to writeable - that is the step that appears to cause the error.

cf. numpy/numpy#6491 and nipy/nibabel#358.

matthew-brett reviewed Oct 15, 2015
View reviewed changes

effigies force-pushed the numpy1.10 branch from 7ddfbf3 to 5576c23 Compare October 15, 2015 02:43

matthew-brett reviewed Oct 15, 2015
View reviewed changes

effigies force-pushed the numpy1.10 branch from 50805f5 to 7b00bd4 Compare October 15, 2015 13:29

matthew-brett mentioned this pull request Oct 16, 2015

Error in broadcasting stride_tricks array numpy/numpy#6491

Closed

matthew-brett reviewed Oct 16, 2015
View reviewed changes

effigies force-pushed the numpy1.10 branch from 8203478 to 70c96f9 Compare October 16, 2015 20:29

bcipolli reviewed Oct 16, 2015
View reviewed changes

effigies force-pushed the numpy1.10 branch from 70c96f9 to 6061d69 Compare October 16, 2015 21:17

matthew-brett reviewed Oct 16, 2015
View reviewed changes

effigies force-pushed the numpy1.10 branch from 6061d69 to d192a37 Compare October 16, 2015 23:40

BF: Set strided_scalar as not writeable

34fbbf9

Numpy 1.10 cannot broadcast a strided array that is set as writeable

effigies force-pushed the numpy1.10 branch from d192a37 to 34fbbf9 Compare October 16, 2015 23:55

matthew-brett merged commit 593dce2 into nipy:master Oct 17, 2015

effigies mentioned this pull request Oct 17, 2015

TST: Add GIFTI tests #355

Merged

effigies deleted the numpy1.10 branch October 19, 2015 16:47

This was referenced Oct 31, 2015

Tests: validate_proxy_slicing: ValueError: cannot set WRITEABLE flag to True of this array #370

Closed

Bug fix release #373

Closed

QuLogic added a commit to QuLogic/biggus that referenced this pull request Jul 3, 2016

Workaround bug in NumPy 1.10.1.

c590b4b

cf. numpy/numpy#6491 and nipy/nibabel#358.

cpelley pushed a commit to cpelley/biggus that referenced this pull request Nov 14, 2016

Workaround bug in NumPy 1.10.1.

f99c186

cf. numpy/numpy#6491 and nipy/nibabel#358.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BF: stride_tricks produces unbroadcastable array #358

BF: stride_tricks produces unbroadcastable array #358

effigies commented Oct 15, 2015

matthew-brett Oct 15, 2015

effigies Oct 15, 2015

matthew-brett Oct 15, 2015

effigies Oct 15, 2015

bcipolli Oct 15, 2015

effigies Oct 15, 2015

bcipolli Oct 15, 2015

effigies Oct 15, 2015

effigies commented Oct 15, 2015

effigies commented Oct 15, 2015

matthew-brett Oct 15, 2015

effigies Oct 15, 2015

effigies commented Oct 15, 2015

effigies commented Oct 15, 2015

matthew-brett Oct 16, 2015

matthew-brett commented Oct 16, 2015

effigies commented Oct 16, 2015

bcipolli Oct 16, 2015

effigies Oct 16, 2015

effigies commented Oct 16, 2015

matthew-brett Oct 16, 2015

effigies Oct 16, 2015

matthew-brett Oct 16, 2015

effigies Oct 16, 2015

matthew-brett commented Oct 17, 2015

effigies commented Oct 17, 2015

BF: stride_tricks produces unbroadcastable array #358

BF: stride_tricks produces unbroadcastable array #358

Conversation

effigies commented Oct 15, 2015

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

effigies commented Oct 15, 2015

effigies commented Oct 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Oct 15, 2015

effigies commented Oct 15, 2015

Choose a reason for hiding this comment

matthew-brett commented Oct 16, 2015

effigies commented Oct 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Oct 16, 2015

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

matthew-brett commented Oct 17, 2015

effigies commented Oct 17, 2015