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

Fix Image.fromarray with NumPy arrays #224

Merged
merged 2 commits into from
May 23, 2013

Conversation

jiffyclub
Copy link
Contributor

Image.fromarray attempts to call a method called tobytes() on the passed in object, but NumPy arrays don't have a tobytes() method, they have a tostring() method. (See http://docs.scipy.org/doc/numpy/reference/generated/numpy.ndarray.tostring.html).

I think this was changed accidentally in a Python 3 compatibility update in which this call was confused with the tobytes and frombytes methods of Image objects.

Image.fromarray attempts to call a method called `tobytes()` on the passed in object, but NumPy arrays don't have a `tobytes()` method, they have a `tostring()` method. (See http://docs.scipy.org/doc/numpy/reference/generated/numpy.ndarray.tostring.html).

I think this was changed accidentally in a Python 3 compatibility update in which this call was confused with the `tobytes` and `frombytes` methods of Image objects.
@wiredfool
Copy link
Member

The python 2.6 (and only 2.6) test run is failing (https://travis-ci.org/python-imaging/Pillow/builds/7372719) with:

running test_image_array ...
/home/travis/virtualenv/python2.6/lib/python2.6/site-packages/Pillow-2.1.0-py2.6-linux-x86_64.egg/PIL/Image.py:1947: DeprecationWarning: tostring() is deprecated. Please call tobytes() instead.
obj = obj.tostring()

Which indicates that in (only) the python 2.6 case, we're getting an image object there instead of ndarray.

I'm a bit confused with this, as these tests were passing prior, so either we're not testing Numpy adequately to catch a basic problem or we don't have a problem. That one test is failing now points to inadequate tests.

Could you add a test that fails before, and succeeds after your patch?

@jiffyclub
Copy link
Contributor Author

So fromarray needs to support both Image objects and NumPy arrays? The source indicates that the .tostring() method of Image objects is deprecated in favor of .tobytes(), but NumPy is (so far) sticking with .tostring(). Does this need an isinstance check? Or a try/except?

@wiredfool
Copy link
Member

I don't know, but my gut reaction is that we really should be expecting one type of object there. However, prior to changes I'd like real understanding, rather than a gut feeling. Also tests.

What I don't understand is that if this is patch is correct, why does the test_numpy.py set of tests work even without it. There's a fromarray call there:

            a = numpy.array(data, dtype=dtype)
            a.shape = 10, 10
            i = Image.fromarray(a)

@jiffyclub
Copy link
Contributor Author

Interesting, I'll have a look.

@jiffyclub
Copy link
Contributor Author

I ran this through coverage and the reason the tests don't discover this is that none of them actually trigger the if strides is not None: line, so the obj.tobytes() call is never actually tested with NumPy arrays. I'll try to come up with a test that does.

@jiffyclub
Copy link
Contributor Author

I've added a test to test_numpy.py that trips the if strides is not None line and errs in master, but passes with my change.

@wiredfool
Copy link
Member

Ok. Looking at the details here, it appears that we're getting warnings on Py2.6 for the tostring that happens from test_image_array.py:28. We're not getting warnings from Py2.7 from apparently the same object calling the tostring. (as checked in pdb).

    assert_equal(test("L"), ("L", (128, 100), True))                                        

What's more, it appears that warnings aren't being generated by default on (Ubuntu 12.04 system python) py2.7:

Python 2.7.3 (default, Aug  1 2012, 05:14:39) 
[GCC 4.6.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import warnings
>>> warnings.warn('foo', DeprecationWarning, stacklevel=2)
>>> 

Where on 2.6, (Ubuntu 10.04 system python) there is a warning:

Python 2.6.5 (r265:79063, Oct  1 2012, 22:07:21) 
[GCC 4.4.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import warnings
>>> warnings.warn('foo', DeprecationWarning, stacklevel=2)
sys:1: DeprecationWarning: foo
>>> 

So, our travis builds aren't generating the warnings, except on 2.6. This is suboptimal.
(Also, system 12.04 py3.2 doesn't have them enabled).

So that explains the python 2.6 difference.

So, the stated requirement of the function is that obj provides an array interface, the array_interface spec doesn't say anything about tostring or tobytes. So, I'd say we should duck type it. Check for tobytes, fall back to tostring.

I've checked https://github.com/wiredfool/Pillow/tree/pr224 against python 2.6, and it's passing Travis on all the versions with the ducktyping in.

@acviana
Copy link

acviana commented May 23, 2013

Thanks for fixing this. I work with @jiffyclub and was confused why this was working on my laptop (Ubuntu 12.04, 2.7) but not on my work machine (OSX 10.6, 2.7), your explanation makes sense. Thanks again.

@jiffyclub
Copy link
Contributor Author

The original PIL still has obj.tostring() here (I checked), so it's also possible you had original PIL one place and Pillow in the other. I'm not sure which one our work uses.

aclark4life added a commit that referenced this pull request May 23, 2013
Fix Image.fromarray with NumPy arrays: Supersedes PR #224
@aclark4life aclark4life merged commit c3d6b05 into python-pillow:master May 23, 2013
@jiffyclub jiffyclub deleted the patch-1 branch June 8, 2013 22:55
radarhere pushed a commit to radarhere/Pillow that referenced this pull request Sep 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants