BUG: view with fieldless dtype should raise if itemsize != 0 #14393
Conversation
|
Does this need a backport? |
|
This bug is so niche that a backport doesn't really seem worth the effort to me, but it would also be harmless to backport. |
d4ddfaa to
847aefc
Compare
|
I think this now warrants a few more tests, since this fixed a lot of bugs. >>> np.dtype((np.dtype([]), 10))
dtype({'names':[], 'formats':[], 'offsets':[], 'itemsize':10}) # before
dtype(([], (10,))) # afterAnd another >>> np.fromiter((() for i in range(10)), [])
ValueError: Must specify length when using variable-size data-type. # before
array(...) # after |
|
Probably worth a |
| @@ -1200,7 +1200,8 @@ _void_compare(PyArrayObject *self, PyArrayObject *other, int cmp_op) | |||
| } | |||
| } | |||
| if (res == NULL && !PyErr_Occurred()) { | |||
| PyErr_SetString(PyExc_ValueError, "No fields found."); | |||
| /* these dtypes had no fields */ | |||
| return cmp_op == Py_EQ ? Py_True : Py_False; | |||
There was a problem hiding this comment.
This change isn't correct - it doesn't produce a correctly broadcast array. Let's leave this to a future PR - the rest looks great.
There was a problem hiding this comment.
Fixed by falling through to the string compare case which does the right thing here.
Edit: Oh wait, I forgot about itemsize differences.. Never mind, it does work because V0 and V8 empty strings are equal.
|
Thanks for great reviews, as usual. I updated I have some hesitation on doing it this way, so consider this a WIP. First, in some of the places calling |
| * compare as a string. Assumes self and | ||
| * other have same descr->type | ||
| * compare as a string. Assumes self and other have same descr->type. | ||
| * Note that fieldless structured datatypes (itemsize 0) fall through |
There was a problem hiding this comment.
What about fieldless types with padding?
c1d3f73 to
c2b2c00
Compare
c2b2c00 to
7c3adfb
Compare
| @@ -1200,15 +1200,18 @@ _void_compare(PyArrayObject *self, PyArrayObject *other, int cmp_op) | |||
| } | |||
| } | |||
| if (res == NULL && !PyErr_Occurred()) { | |||
| PyErr_SetString(PyExc_ValueError, "No fields found."); | |||
| /* these dtypes had no fields */ | |||
| res = PyArray_NewLikeArray(self, NPY_ANYORDER, | |||
There was a problem hiding this comment.
This doesn't broadcast with other, unless the broadcasting has already happened here
There was a problem hiding this comment.
Ah, yeah. I think I've got it finally by using a multiiter.
| @@ -1200,15 +1200,28 @@ _void_compare(PyArrayObject *self, PyArrayObject *other, int cmp_op) | |||
| } | |||
| } | |||
| if (res == NULL && !PyErr_Occurred()) { | |||
| PyErr_SetString(PyExc_ValueError, "No fields found."); | |||
| @@ -1861,7 +1861,7 @@ array_reduce_ex(PyArrayObject *self, PyObject *args) | |||
| PyDataType_FLAGCHK(descr, NPY_ITEM_HASOBJECT) || | |||
| (PyType_IsSubtype(((PyObject*)self)->ob_type, &PyArray_Type) && | |||
| ((PyObject*)self)->ob_type != &PyArray_Type) || | |||
| PyDataType_ISUNSIZED(descr)) { | |||
| descr->elsize == 0) { | |||
There was a problem hiding this comment.
It's not clear to me why this condition is needed at all, although it admittedly looks harmless.
There was a problem hiding this comment.
not clear to me either. These lines previously discussed here: https://github.com/numpy/numpy/pull/12748/files#r249258808 and introduced in #12011, but not discussed there.
|
I think an |
|
@ahaldane Ping. |
|
About to push as you wrote that! |
d33c5d2 to
5e7bca5
Compare
5e7bca5 to
d6f7524
Compare
|
Thanks @ahaldane and reviewers |
Fixes #14344
Minor bugfix caused by treating the "dtype([])" structured type as if it was the "V0" flexible type.