-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
BUG: do not change size 0 description when viewing data #8971
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Looks good to me, Probably should test the |
0958146
to
3187457
Compare
added a test |
3187457
to
ff1b5c0
Compare
numpy/core/tests/test_multiarray.py
Outdated
@@ -1073,6 +1073,9 @@ def test_zero_width_string(self): | |||
xx = x['S'].reshape((2, 2)) | |||
assert_equal(xx.itemsize, 0) | |||
assert_equal(xx, [[b'', b''], [b'', b'']]) | |||
# check for no uninitialized memory due to viewing S0 array | |||
repr_str = ''.join(repr(xx).replace('array', 'np.array').splitlines()) | |||
assert_array_equal(eval(repr_str), xx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a assert_equal(xx.ravel().dtype, xx.dtype)
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ravel makes a copy here which converts it to 'S1'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a test with a simple indexing view
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure ravel makes a copy? I've found a bunch of places with zeros-sized items that it returns a view, and generally does the right thing.
Either way, the indexing tests the right thing, so no changes needed.
ff1b5c0
to
2103ab1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good
numpy/core/tests/test_multiarray.py
Outdated
# check for no uninitialized memory due to viewing S0 array | ||
assert_equal(xx[:].dtype, xx.dtype) | ||
repr_str = ''.join(repr(xx).replace('array', 'np.array').splitlines()) | ||
assert_array_equal(eval(repr_str), xx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about eval(repr_str, dict(array=np.array))
? I don't think you need any of the line-mangling either
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed that is nicer, thanks
@juliantaylor Needs release note. |
this is a bugfix, no release notes needed |
If the array being viewed is a zero sized array, e.g. 'S0' from structured types, do not change the description to itemsize 1 as this would expose uninitialized data, e.g. in TestStructured.test_zero_width_string
2103ab1
to
6373dc0
Compare
My bad, no release note needed. Merged, thanks @juliantaylor @eric-wieser . |
If the array being viewed is a zero sized array, e.g. 'S0' from
structured types, do not change the description to itemsize 1 as this
would expose uninitialized data, e.g. in
TestStructured.test_zero_width_string
See also #6430