Memoryviews require more strict contiguous checks then necessary

[seberg]

BPO	22445
Nosy	@abalkin, @pitrou, @skrah, @seberg
Files	relaxed-strides-checking.patch: Patch relaxing the buffer contiguity checks (tests missing; untested) relaxed-strides-checking.patch: Added missing shape==NULL paths. relaxed-strides-checking.patch: Further fix some empty buffer cases. contiguous.py: Python code for changed contiguity checking (not simpler then C-code, but maybe nice for testing) contiguous.py: Smaller fix, some restructuring. issue22445.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2015-02-01.14:28:35.921>
created_at = <Date 2014-09-19.19:00:49.974>
labels = ['type-feature', 'library']
title = 'Memoryviews require more strict contiguous checks then necessary'
updated_at = <Date 2015-02-01.14:28:35.920>
user = 'https://github.com/seberg'

bugs.python.org fields:

activity = <Date 2015-02-01.14:28:35.920>
actor = 'skrah'
assignee = 'none'
closed = True
closed_date = <Date 2015-02-01.14:28:35.921>
closer = 'skrah'
components = ['Library (Lib)']
creation = <Date 2014-09-19.19:00:49.974>
creator = 'seberg'
dependencies = []
files = ['36663', '36676', '36677', '36678', '36680', '36738']
hgrepos = []
issue_num = 22445
keywords = ['patch']
message_count = 23.0
messages = ['227113', '227114', '227115', '227116', '227118', '227128', '227129', '227153', '227155', '227168', '227213', '227233', '227262', '227263', '227616', '228228', '228283', '228361', '229443', '229474', '229485', '229728', '235171']
nosy_count = 5.0
nosy_names = ['belopolsky', 'pitrou', 'skrah', 'python-dev', 'seberg']
pr_nums = []
priority = 'low'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue22445'
versions = ['Python 3.5']

[seberg]

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 numpy/numpy#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.

[skrah]

There is another oddity: bpo-12845. Does NumPy have a formal definition of
array contiguity somewhere?

[skrah]

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).

[seberg]

bpo-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.

[seberg]

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.

[skrah]

Thanks, bpo-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

[seberg]

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.

[skrah]

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?

[seberg]

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).

[skrah]

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).

[seberg]

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.

[skrah]

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.

[skrah]

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?