Skip to content

BUG: view with fieldless dtype should raise if itemsize != 0 - #14393

Merged
mattip merged 4 commits into
numpy:masterfrom
ahaldane:fix_fieldless_view_itemsize
Sep 8, 2019
Merged

BUG: view with fieldless dtype should raise if itemsize != 0#14393
mattip merged 4 commits into
numpy:masterfrom
ahaldane:fix_fieldless_view_itemsize

Conversation

@ahaldane

@ahaldane ahaldane commented Aug 28, 2019

Copy link
Copy Markdown
Member

Fixes #14344

Minor bugfix caused by treating the "dtype([])" structured type as if it was the "V0" flexible type.

@charris

charris commented Aug 28, 2019

Copy link
Copy Markdown
Member

Does this need a backport?

@ahaldane

ahaldane commented Aug 28, 2019

Copy link
Copy Markdown
Member Author

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.

eric-wieser
eric-wieser reviewed Aug 28, 2019
View reviewed changes
Comment thread numpy/core/src/multiarray/getset.c Resolved
@ahaldane
ahaldane force-pushed the fix_fieldless_view_itemsize branch from d4ddfaa to 847aefc Compare August 28, 2019 17:54
@eric-wieser

eric-wieser commented Aug 29, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

I think this now warrants a few more tests, since this fixed a lot of bugs.
Here's another that I think is fixed

>>> np.dtype((np.dtype([]), 10))
dtype({'names':[], 'formats':[], 'offsets':[], 'itemsize':10})  # before
dtype(([], (10,)))  # after

And another

>>> np.fromiter((() for i in range(10)), [])
ValueError: Must specify length when using variable-size data-type.  # before
array(...)  # after

@eric-wieser

eric-wieser commented Aug 29, 2019

Copy link
Copy Markdown
Member

Probably worth a ..versionchanged in doc/source/reference/c-api/array.rst too, where this macro is documented.

eric-wieser
eric-wieser reviewed Aug 31, 2019
View reviewed changes
Comment thread numpy/core/src/multiarray/arrayobject.c Outdated
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;

@eric-wieser eric-wieser Aug 31, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ahaldane ahaldane Aug 31, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ahaldane

ahaldane commented Aug 31, 2019

Copy link
Copy Markdown
Member Author

Thanks for great reviews, as usual.

I updated PyDataType_ISUNSIZED, and went through all the places it was used to check if this introduced any bugs. It did, which I fixed, and I added appropriate tests.

I have some hesitation on doing it this way, so consider this a WIP. First, in some of the places calling ISUNSIZED the code really did want to check just elsize == 0, fields or no fields. Also, with the same logic it seems like PyDataType_ISFLEXIBLE should also be False for structured 0-size types. However, when I make that change I get a bunch of segfaults, especially in the pickling code. .

eric-wieser
eric-wieser reviewed Aug 31, 2019
View reviewed changes
Comment thread numpy/core/src/multiarray/arrayobject.c Outdated

@eric-wieser eric-wieser Aug 31, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about fieldless types with padding?

@ahaldane
ahaldane force-pushed the fix_fieldless_view_itemsize branch 2 times, most recently from c1d3f73 to c2b2c00 Compare August 31, 2019 20:09
@ahaldane
ahaldane force-pushed the fix_fieldless_view_itemsize branch from c2b2c00 to 7c3adfb Compare August 31, 2019 21:17
eric-wieser
eric-wieser reviewed Aug 31, 2019
View reviewed changes
Comment thread numpy/core/src/multiarray/arrayobject.c Outdated
if (res == NULL && !PyErr_Occurred()) {
PyErr_SetString(PyExc_ValueError, "No fields found.");
/* these dtypes had no fields */
res = PyArray_NewLikeArray(self, NPY_ANYORDER,

@eric-wieser eric-wieser Aug 31, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't broadcast with other, unless the broadcasting has already happened here

@ahaldane ahaldane Sep 1, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah. I think I've got it finally by using a multiiter.

eric-wieser
eric-wieser reviewed Sep 4, 2019
View reviewed changes
Comment thread numpy/core/src/multiarray/arrayobject.c Outdated

@eric-wieser eric-wieser Sep 4, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes gh-13438, I think

eric-wieser
eric-wieser reviewed Sep 4, 2019
View reviewed changes
Comment thread numpy/core/src/multiarray/methods.c Outdated

@eric-wieser eric-wieser Sep 4, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me why this condition is needed at all, although it admittedly looks harmless.

@ahaldane ahaldane Sep 5, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

eric-wieser
eric-wieser reviewed Sep 4, 2019
View reviewed changes
Comment thread doc/source/reference/c-api/array.rst Resolved
eric-wieser
eric-wieser approved these changes Sep 4, 2019
View reviewed changes

@eric-wieser eric-wieser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good - one unimportant question, and a doc typo. Getting the comparison fix in here should have enabled me to push some of my remaining fixes, which were untestable without it - thanks!

@eric-wieser

eric-wieser commented Sep 4, 2019

Copy link
Copy Markdown
Member

I think an upcoming_changes/14393.c_api.rst note might be warranted about the macro behavior

@charris

charris commented Sep 5, 2019

Copy link
Copy Markdown
Member

@ahaldane Ping.

@ahaldane

ahaldane commented Sep 5, 2019

Copy link
Copy Markdown
Member Author

About to push as you wrote that!

@ahaldane
ahaldane force-pushed the fix_fieldless_view_itemsize branch from d33c5d2 to 5e7bca5 Compare September 5, 2019 20:57
@ahaldane
ahaldane force-pushed the fix_fieldless_view_itemsize branch from 5e7bca5 to d6f7524 Compare September 5, 2019 22:07
@mattip
mattip merged commit f786041 into numpy:master Sep 8, 2019
@mattip

mattip commented Sep 8, 2019

Copy link
Copy Markdown
Member

Thanks @ahaldane and reviewers

@mattip mattip mentioned this pull request Sep 8, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eric-wieser eric-wieser eric-wieser approved these changes

Assignees

No one assigned

Labels

00 - Bug component: numpy._core

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

viewing an array with a fieldless structured dtype does not update the dtype:

4 participants

@ahaldane @charris @eric-wieser @mattip