Skip to content
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

Make Image.__array__ take optional dtype argument #5572

Merged
merged 3 commits into from Jul 5, 2021

Conversation

t-vi
Copy link
Contributor

@t-vi t-vi commented Jul 1, 2021

This is required by the numpy protocol.

Fixes #5571

This is required by the numpy protocol.
@radarhere radarhere added the NumPy label Jul 1, 2021
src/PIL/Image.py Outdated Show resolved Hide resolved
@t-vi
Copy link
Contributor Author

t-vi commented Jul 1, 2021

To demystify what is needed by __array__, here are the two calls numpy does, one with dtype one without. (And in particular, you don't need to implement numpy.array 's signature.)

https://github.com/numpy/numpy/blob/45c57078fd599194b49148fb66808f03490ee423/numpy/core/src/multiarray/ctors.c#L2453-L2458

This has been very stable and the reason the bug shows up in Pillow 8.3 is that it implemented only the version without the dtype passed in when switching to providing __array__ in #5379.

I looked at numpy's ndarray __array__ for inspiration that providing a non-identical dtype casts/copies rather than failing.

Tests/test_image_array.py Outdated Show resolved Hide resolved
t-vi and others added 2 commits July 1, 2021 14:06
Thank you @radarhere.

Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
@t-vi
Copy link
Contributor Author

t-vi commented Jul 1, 2021

I'm not entirely sure what to do with the codecov. It seems implausible to me that the line it points out in the Image.__array__ isn't covered by tests and I don't know what to think about the idea of tests being covered by tests.

@radarhere
Copy link
Member

I expect that was just because some CI jobs had an intermittent failure and were not returning their coverage results - everything looks fine with your latest commit.

@t-vi
Copy link
Contributor Author

t-vi commented Jul 1, 2021

OK, I leave the it to you to take this forward, then. Thank you for your suggestions.

@radarhere radarhere merged commit d43d446 into python-pillow:master Jul 5, 2021
@hugovk hugovk mentioned this pull request Jul 5, 2021
23 tasks
jni added a commit to regro-cf-autotick-bot/scikit-image-feedstock that referenced this pull request Jul 7, 2021
czlee added a commit to czlee/jadeite that referenced this pull request Jul 29, 2021
There's currently a bug that interferes with the .to_tensor() function
in torch.

see: pytorch/pytorch#61125 (comment)
and: python-pillow/Pillow#5572
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pillow 8.3 and NumPy
3 participants