classification
Title: Memoryviews require more strict contiguous checks then necessary
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: belopolsky, pitrou, python-dev, seberg, skrah
Priority: low Keywords: patch

Created on 2014-09-19 19:00 by seberg, last changed 2015-02-01 14:28 by skrah. This issue is now closed.

Files
File name Uploaded Description Edit
relaxed-strides-checking.patch seberg, 2014-09-19 19:00 Patch relaxing the buffer contiguity checks (tests missing; untested) review
relaxed-strides-checking.patch seberg, 2014-09-21 11:33 Added missing shape==NULL paths.
relaxed-strides-checking.patch seberg, 2014-09-21 12:14 Further fix some empty buffer cases.
contiguous.py seberg, 2014-09-21 12:20 Python code for changed contiguity checking (not simpler then C-code, but maybe nice for testing)
contiguous.py seberg, 2014-09-21 13:07 Smaller fix, some restructuring.
issue22445.diff skrah, 2014-09-26 13:40 review
Messages (23)
msg227113 - (view) Author: Sebastian Berg (seberg) * Date: 2014-09-19 19:00
In NumPy we decided some time ago that if you have a multi dimensional buffer, shaped for example 1x10, then this buffer should be considered both C- and F-contiguous. Currently, some buffers which can be used validly in a contiguous fashion are rejected.

CPython does not support this currently possibly creating smaller nuisance if/once we change it fully in NumPy, see for example https://github.com/numpy/numpy/issues/5085

I have attached a patch which should (sorry I did not test this at all yet) relax the checks as much as possible. I think this is right, but we did some subtle breaks in user code (mostly cython code) when we first tried changing it, and while numpy arrays may be more prominently C/F-contiguous, compatibility issues with libraries checking for contiguity explicitly and then requesting a strided buffer are very possible.

If someone could give me a hint about adding tests, that would be great.
Also I would like to add a small note to the PEP in any case regarding this subtlety, in the hope that more code will take care about such subtleties.
msg227114 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-09-19 20:16
There is another oddity: #12845.  Does NumPy have a formal definition of
array contiguity somewhere?
msg227115 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-09-19 20:23
BTW, if you have NumPy installed and run test_buffer in Python3.3+,
numpy.ndarray has many tests against memoryview and _testbuffer.ndarray
(the latter is our exegesis of PEP-3118).
msg227116 - (view) Author: Sebastian Berg (seberg) * Date: 2014-09-19 20:25
#12845 should be closed, seems like a bug in some old version. The definition now is simply that the array is contiguous if you can legally access it in a contiguous fashion. Which means first stride is itemsize, second is itemsize*shape[0] for Fortran, inverted for C-order.
msg227118 - (view) Author: Sebastian Berg (seberg) * Date: 2014-09-19 20:39
To be clear, the important part here, is that to me all elements *can* be accessed using that scheme. It is not correct to assume that `stride[-1]` or `stride[0]` is actually equal to `itemsize`.

In other words, you have to be able to pass the pointer to the start of a c-contiguous array into some C-library that knows nothing about strides without any further thinking. The 0-strides badly violate that.
msg227128 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-09-19 22:00
Thanks, #12845 is indeed fixed in NumPy.


Why does NumPy consider an array with a stride that will almost
certainly lead to undefined behavior (unless you compile with
-fwrapv) as valid?

In CPython we try to eliminate these kinds of issues (though
they may still be present).


>>> import numpy as np
import io

x = np.arange(10)
y = np.array([x])

print(y.strides)
(9223372036854775807, 8)
>>> 
>>> 
>>> y.flags
  C_CONTIGUOUS : True
  F_CONTIGUOUS : True
  OWNDATA : True
  WRITEABLE : True
  ALIGNED : True
  UPDATEIFCOPY : False
msg227129 - (view) Author: Sebastian Berg (seberg) * Date: 2014-09-19 22:12
Well, the 9223372036854775807 is certainly no good for production code and we would never have it in a release version, it is just there currently to expose if there are more problems. However I don't care what happens on overflow (as long as it is not an error).

Note that the stride here is on a dimension with shape 1. The only valid index is thus always 0 and 0*9223372036854775807=0, so the stride value does not actually matter when calculating offsets into the array. You could simply set it to 80 to get something that would be considered C-contiguous or to 8 to get something that is considered F-contiguous. But both is the case in a way, so just "cleaning up" the strides does not actually get you all the way.
msg227153 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-09-20 11:23
Ok, so it is a debug thing in the current NumPy sources.

IMO ultimately the getbufferproc needs to return valid strides, even
if the first value isn't used.


For that matter, the getbufferproc is free to translate the multi-
dimensional corner case array to a one-dimensional array that is
automatically C and F-contiguous.

Does it matter if you lose some (irrelevant?) information about
the original array structure?
msg227155 - (view) Author: Sebastian Berg (seberg) * Date: 2014-09-20 11:43
An extra dimension is certainly not irrelevant! The strides *are* valid
and numpy currently actually commonly creates such arrays when slicing.
The question is whether or not we want to ignore them for contiguity
checks even if they have no effect on the memory layout.

So there are three options I currently see:

1. Python also generalizes like I would like numpy to end up in the
future (the current patch should do that) and just don't care about such
strides, because the actual memory layout is what matters.
2. We say it is either too dangerous (which may very well be) or you
want to preserve Fortran/C-order information even when it does not
matter to the memory layout.

This leads to this maybe:
2a) we just keep it as it is and live with minor inconsistencies (or
never do the relaxed strides in numpy)
2b) We let these buffers return False on checking for contiguity but
*allow* allow fetching a buffer when C-/F-contiguous is explicitly asked
for when getting the buffer. Which is a weird middle way, but it might
actually be a pretty sane solution (have to think about it).
msg227168 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-09-20 19:10
I think it would help discussing your options if the patch passes test_buffer
first.  Currently it segfaults because shape can be NULL.  Also, code in
memoryobject.c relies on the fact that ndim==0 means contiguous.

Then, it would help enormously if you give Python function definitions of
the revised C and F-contiguity.

I mean something like verify_structure() from Lib/test/test_buffer.py -- that
function definition was largely supplied by Pauli Virtanen, but I may have
added the check for strides-is-multiple-of-itemsize (which 2**63-1 usually
isn't, so the new debug numpy strides don't pass that test).
msg227213 - (view) Author: Sebastian Berg (seberg) * Date: 2014-09-21 11:33
I am very sorry. The attached patch fixes this (not sure if quite right, but if anything should be more general then necessary). One test fails, but it looks like exactly the intended change.
msg227233 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-09-21 20:15
Thanks!  I still have to review the patch in depth, but generally
I'm +1 now for relaxing the contiguity check.

Curiously enough the existing code already considered e.g. shape=[1], strides=[-5] as contiguous.
msg227262 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-09-22 12:20
Since the functions in abstract.c have been committed by Travis Oliphant:

Could there have been a reason why the {shape=[1], strides=[-5]}
case was considered but the general case was not?


Or is it generally accepted among the numpy devs that not considering
the general case was just an oversight?
msg227263 - (view) Author: Sebastian Berg (seberg) * Date: 2014-09-22 12:27
Yeah, the code does much the same as the old numpy code (at least most of the same funny little things, though I seem to remember the old numpy code had something yet a bit weirder, would have to check).

To be honest, I do not know. It isn't implausible that the original numpy code dates back 15 years or more to numeric. I doubt whoever originally wrote it thought much about it, but there may be some good reason, and there is the safety considerations that people use the strides in a way they should not, which may trip us here in any case.
msg227616 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-09-26 13:40
Ok, here's my take on the situation:

1) As far as Python is concerned, shape[0] == 1 was already special-cased, so
   people could not rely on canonical Fortran or C strides anyway.


2) Accessing an element via strides should be done using PyBuffer_GetPointer(),
   which can of course handle non-canonical strides.


3) Breakage will only affect NumPy users, since practically no one else is
   using multidimensional arrays.


Regarding your option 2b):  I think it may be confusing, the buffer protocol
is already so complicated.


So, I think purity wins here.  If you are sure that all future NumPy versions
will ship with precise contiguity checks, then I'll commit the new patch in 3.5 (earlier versions should not be changed IMO).


I've moved the checks for 0 in shape[i] to the beginning (len == 0).  I hope
there are no applications that set len incorrectly, but they would be severely
broken anyway.
msg228228 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-10-02 14:24
FWIW, I think it would be good to make this change early in the
3.5 release cycle, so issues can be found.  Sebastian, do you
have an idea when the change will be decided in numpy?


Regarding the discussion here ...

   https://github.com/numpy/numpy/issues/5085

... about the special stride marker:


In the case of slicing it would be nice to use the "organic" value
that would arise normally from computing the slice. That helps in checking other PEP-3118 implementations like Modules/_testbuffer.c
against numpy.
msg228283 - (view) Author: Sebastian Berg (seberg) * Date: 2014-10-03 00:04
Numpy 1.9. was only released recently, so 1.10. might be a while. If no
problems show up during release or until then, we will likely switch it
by then. But that could end up being a year from now, so I am not sure
if 3.6 might not fit better. The problems should be mostly mitigated on
our side. So bug-wise it shouldn't be a big issue I would guess.

I will try to look at it more soon, but am completly overloaded at least
for the next few days, and maybe some other numpy devs can chip in. Not
sure I get your last point, slicing should give the "organic" values
even for the mangled up thing with relaxed strides on (currently)?!
msg228361 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-10-03 18:27
Okay, the whole thing isn't that urgent either.


Sorry for the confusion w.r.t slicing: I misremembered what the
latest numpy version did:

a)

   >>> x = np.array([[1,2,3,]])
   >>> x.strides
   (9223372036854775807, 8)


b)
 
   >>> x = np.array([[1,2,3], [4,5,6]])[:1]
   >>> x.strides
   (24, 8)


Somehow I thought that case b) would also produce the special marker,
but it doesn't, so all is well.
msg229443 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-10-15 13:32
Is this related to the NPY_RELAXED_STRIDES_CHECKING compilation flag?
msg229474 - (view) Author: Sebastian Berg (seberg) * Date: 2014-10-15 16:49
@pitrou, yes of course. This would make python do the same thing as numpy does (currently only with that compile flag given).
About the time schedule, I think I will try to see if some other numpy dev has an opinion. Plus, should look into documenting it for the moment, so that someone who reads up on the buffer protocol should get things right.
msg229485 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-10-15 17:49
Like Stefan I think this would be good to go in 3.5. The PyBuffer APIs are relatively new so there shouldn't be a lot of breakage.
msg229728 - (view) Author: Sebastian Berg (seberg) * Date: 2014-10-20 15:34
Antoine, sounds good to me, I don't mind this being in python rather sooner then later, for NumPy itself it does not matter I think. I just wanted to warn that there were problems when we first tried to switch in NumPy, which, if I remember correctly, is now maybe 2 years ago (in a dev version), though.
msg235171 - (view) Author: Roundup Robot (python-dev) Date: 2015-02-01 13:55
New changeset 369300948f3f by Stefan Krah in branch 'default':
Issue #22445: PyBuffer_IsContiguous() now implements precise contiguity
https://hg.python.org/cpython/rev/369300948f3f
History
Date User Action Args
2015-02-01 14:28:35skrahsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2015-02-01 13:55:02python-devsetnosy: + python-dev
messages: + msg235171
2014-10-20 15:34:13sebergsetmessages: + msg229728
2014-10-15 17:49:27pitrousetmessages: + msg229485
2014-10-15 16:49:34sebergsetmessages: + msg229474
2014-10-15 13:32:07pitrousetnosy: + pitrou
messages: + msg229443
2014-10-03 18:27:08skrahsetpriority: normal -> low

messages: + msg228361
2014-10-03 00:04:36sebergsetmessages: + msg228283
2014-10-02 14:24:24skrahsetmessages: + msg228228
2014-09-26 13:40:06skrahsetfiles: + issue22445.diff

stage: patch review
messages: + msg227616
versions: + Python 3.5
2014-09-22 12:27:18sebergsetmessages: + msg227263
2014-09-22 12:20:33skrahsetmessages: + msg227262
2014-09-21 20:15:32skrahsetmessages: + msg227233
2014-09-21 13:07:57sebergsetfiles: + contiguous.py
2014-09-21 12:20:02sebergsetfiles: + contiguous.py
2014-09-21 12:14:46sebergsetfiles: + relaxed-strides-checking.patch
2014-09-21 11:33:51sebergsetfiles: + relaxed-strides-checking.patch

messages: + msg227213
2014-09-20 19:10:14skrahsetmessages: + msg227168
2014-09-20 11:43:45sebergsetmessages: + msg227155
2014-09-20 11:23:57skrahsetmessages: + msg227153
2014-09-19 23:20:30belopolskysetnosy: + belopolsky
2014-09-19 22:12:38sebergsetmessages: + msg227129
2014-09-19 22:00:35skrahsetmessages: + msg227128
2014-09-19 20:39:52sebergsetmessages: + msg227118
2014-09-19 20:25:45sebergsetmessages: + msg227116
2014-09-19 20:23:40skrahsetmessages: + msg227115
2014-09-19 20:16:21skrahsetnosy: + skrah
messages: + msg227114
2014-09-19 19:00:49sebergcreate