Skip to content

Spurious C contiguity issues on git master #2956

Closed
pv opened this Issue Feb 1, 2013 · 8 comments

4 participants

@pv
NumPy member
pv commented Feb 1, 2013

This Cython code fails on Numpy git master (4600b2f):

import numpy as np
def foo(y):
    cdef double[:,::1] data
    y = y.reshape(y.shape[0], -1).T
    y = np.ascontiguousarray(y, dtype=np.double)
    data = y   # <- raises ValueError: Buffer not C contiguous.

called with

import numpy as np
import asd
asd.foo(np.zeros([90])[:,None])

ascontiguousarray should always return C contiguous arrays with correct flags, but that doesn't seem to be the case currently. Doesn't fail on 1.7.0rc1 and 1.7.x branch.

Observed in Scipy tests with Numpy master:

http://projects.scipy.org/scipy/ticket/1832
http://projects.scipy.org/scipy/ticket/1830

@nouiz
nouiz commented Feb 1, 2013

I have this issue about reshape: #2949 @seberg have a PR that fix this.

I don't know why ascoutiguousarray didn't fix the problem.

@seberg
NumPy member
seberg commented Feb 1, 2013

@pv what you are probably getting what gh-2735 is about. Since you are using arr[:,None] that seems likely since that is one example for the change.

The thing should be that when you have only one element in the dimension, the stride does not really matter (since it should never be used). But some checks for contiguity seem to be wrong, if this is in cython, it is not really to worry about (I think cython could be made aware of the issue), however if it inside the numpy buffer protocol it would be even an inconsistency with how numpy works on master.

Anyway, I will check this out later, but am pretty busy right now, though I am thinking that maybe the check is in cython itself...

@pv
NumPy member
pv commented Feb 1, 2013

@seberg: yes, the check is on Cython side, and gh-2735 seems indeed related.

Namely, Cython produces

            Py_ssize_t stride = 1;
            for (i = ndim-1; i>-1; i--) {
                if(stride * buf->itemsize != buf->strides[i]) {
                    PyErr_SetString(PyExc_ValueError,
                        "Buffer not C contiguous.");
                    goto fail;
                }
                stride = stride * buf->shape[i];
            }

which will also check the stride when dim <= 1.

@seberg
NumPy member
seberg commented Feb 1, 2013

Yes... so that is an inconsistency between master numpy and cython... I really think this should be changed in cython for the future (this also may apply to the usage with np.ndarray[...] and subsequent access to the array.stride to protect users from doing something stupid, but I am not sure how to best approach this issue).

Numpy will have to revert it of course for backward compatibility in any case, I am aware of that, though I am not quite sure how to best resolve it, so if anyone has an opinion...

Not sure, there may also be a 0-d special case missing in Cython (but I am not sure about that)

@njsmith
NumPy member
njsmith commented Feb 26, 2013

@pv: Yeah, that should be if (buf->shape[i] > 1 && stride * buf->itemsize != buf->strides[i])

@seberg
NumPy member
seberg commented Feb 26, 2013

I am begging to think about doing a mini fix here (not temporary though, but a first step). This interpretation is probably common, and while we can argue that someone using numpy should (at some point) fix it, maybe we should go out of our way and set the strides nicely for users of the python buffer interface. Through it numpy might interact with libraries it cannot educate. Also for the buffer interface there is not much incentive to allow for C and F contiguity at the same time, etc...

Since cython uses the buffer interface for this (I believe), that should actually fix this one issue at hand. However I will say again that for some special cases numpy's strides were never set nicely, so that for backward compatibility with older numpy versions a more relaxed check would be good in any case.

And because nobody will head my warning without an example :) :

def add_one(array):
    cdef double[::1] a = array
    a[0] += 1.
    return array

you get:

>>> add_one(np.ascontiguousarray(np.arange(10.)[::100]))
ValueError: Buffer and memoryview are not contiguous in the same dimension.

you should be able to construct another class with 0-sized arrays.

@seberg
NumPy member
seberg commented Feb 26, 2013

ooops, this does only half help cython, since it probably does not request the buffer as contiguous, numpy cannot (always) guess if it wants C or F-contiguity. If cython requests it as contiguous but still checks the strides, in that case it would be fine though.

@seberg
NumPy member
seberg commented Apr 3, 2013

This is "fixed" in numpy master now. If you have an eye on Cython, it would be cool if it can be fixed on the cython end (as also noted on the mailing list), so that we can maybe even change the behaviour for an RC release forcing users to be aware of the issue (without breaking almost all cython projects).

The behaviour that broke things can now be enabled by setting the NPY_RELAXED_STRIDES_CHECKING environment variable during compilation and should be the long term goal. If set np.ones((10,1), order='C').flags.f_contiguous is True (if it isn't something went wrong...).

@seberg seberg closed this Apr 3, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.