[issue22445] Memoryviews require more strict contiguous checks then necessary
Roundup Robot added the comment: New changeset 369300948f3f by Stefan Krah in branch 'default': Issue #22445: PyBuffer_IsContiguous() now implements precise contiguity https://hg.python.org/cpython/rev/369300948f3f -- nosy: +python-dev ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Changes by Stefan Krah ste...@bytereef.org: -- resolution: - fixed stage: patch review - resolved status: open - closed ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Sebastian Berg added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Antoine Pitrou added the comment: Is this related to the NPY_RELAXED_STRIDES_CHECKING compilation flag? -- nosy: +pitrou ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Sebastian Berg added the comment: @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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Antoine Pitrou added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Stefan Krah added the comment: 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. -- priority: normal - low ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Stefan Krah added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Sebastian Berg added the comment: 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)?! -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Stefan Krah added the comment: 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. -- stage: - patch review versions: +Python 3.5 Added file: http://bugs.python.org/file36738/issue22445.diff ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Stefan Krah added the comment:
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?
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue22445
___
___
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Sebastian Berg added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Sebastian Berg added the comment: 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. -- Added file: http://bugs.python.org/file36676/relaxed-strides-checking.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Changes by Sebastian Berg sebast...@sipsolutions.net: Added file: http://bugs.python.org/file36677/relaxed-strides-checking.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Changes by Sebastian Berg sebast...@sipsolutions.net: Added file: http://bugs.python.org/file36678/contiguous.py ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Changes by Sebastian Berg sebast...@sipsolutions.net: Added file: http://bugs.python.org/file36680/contiguous.py ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Stefan Krah added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Stefan Krah added the comment: 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? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Sebastian Berg added the comment: 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). -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Stefan Krah added the comment: 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). -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
New submission from Sebastian Berg: 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. -- components: Library (Lib) files: relaxed-strides-checking.patch keywords: patch messages: 227113 nosy: seberg priority: normal severity: normal status: open title: Memoryviews require more strict contiguous checks then necessary type: enhancement Added file: http://bugs.python.org/file36663/relaxed-strides-checking.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Stefan Krah added the comment: There is another oddity: #12845. Does NumPy have a formal definition of array contiguity somewhere? -- nosy: +skrah ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Stefan Krah added the comment: 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). -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Sebastian Berg added the comment: #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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Sebastian Berg added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Stefan Krah added the comment: 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 -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Sebastian Berg added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22445] Memoryviews require more strict contiguous checks then necessary
Changes by Alexander Belopolsky alexander.belopol...@gmail.com: -- nosy: +belopolsky ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22445 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
