matplotlib-devel

[matplotlib-devel] Bug in imsave

[gary r. <gar...@gm...>]

Hi, I haven't looked at this list for a long time, so hi to all the active
devs.
I just came across a problem in the image.py imsave() function.

Problem:
At an ipython --pylab prompt, typing

a = rand(29,29)
imsave('test.png', a)

incorrectly generates a 28x28 pixel image.
This happens for several array sizes (I initially saw it for 226x226).

Line 1272 of image.py is
figsize = [x / float(dpi) for x in (arr.shape[1], arr.shape[0])]

figsize interacts with the bounding box code somehow to create off-by-one
results for certain values of array shape.


Possible solution:
To fix this properly, I think the float cast should be removed and
integer-based math should be used, but I thought that since it was rounding
down in the cases I saw, a change to the following should fix this:

figsize = [(x + 0.5) / float(dpi) for x in (arr.shape[1], arr.shape[0])]

and indeed this seems to work.
To verify this, I ran the following brute-force test of all image sizes up
to 1024x1024 at some common dpi values:

import matplotlib.pyplot as plt
import numpy as np
import os

for dpi in [75, 100, 150, 300, 600, 1200]:
    for i in range(1,1025):
        print dpi, i,
        im = np.random.random((i,i))
        fname = 'test{:03d}.png'.format(i)
        plt.imsave(fname, im, dpi=dpi)
        im = plt.imread(fname)[:,:,0]
        print im.shape
        assert im.shape == (i, i)
        os.remove(fname)

and everything passed.

For completeness I also explored the dpi-space with these loop ranges and
the above loop body:

for dpi in range(1, 101):
    for i in range(25, 36):
        ...

and all was still well.


Workaround:
For the moment I've set dpi=1 in my call to imsave which effectively
reverts its behaviour to the original imsave code prior to the introduction
of the dpi parameter.

I think this testing is rigorous enough to make this change.
If you agree, has anyone got time to change this, or shall I do a pull
request when I have time?

thanks,
Gary

Re: [matplotlib-devel] Bug in imsave

[Benjamin R. <ben...@ou...>]

On Sun, Apr 7, 2013 at 4:39 AM, gary ruben <gar...@gm...> wrote:

> Hi, I haven't looked at this list for a long time, so hi to all the active
> devs.
> I just came across a problem in the image.py imsave() function.
>
> Problem:
> At an ipython --pylab prompt, typing
>
> a = rand(29,29)
> imsave('test.png', a)
>
> incorrectly generates a 28x28 pixel image.
> This happens for several array sizes (I initially saw it for 226x226).
>
> Line 1272 of image.py is
> figsize = [x / float(dpi) for x in (arr.shape[1], arr.shape[0])]
>
> figsize interacts with the bounding box code somehow to create off-by-one
> results for certain values of array shape.
>
>
> Possible solution:
> To fix this properly, I think the float cast should be removed and
> integer-based math should be used, but I thought that since it was rounding
> down in the cases I saw, a change to the following should fix this:
>
> figsize = [(x + 0.5) / float(dpi) for x in (arr.shape[1], arr.shape[0])]
>
> and indeed this seems to work.
> To verify this, I ran the following brute-force test of all image sizes up
> to 1024x1024 at some common dpi values:
>
> import matplotlib.pyplot as plt
> import numpy as np
> import os
>
> for dpi in [75, 100, 150, 300, 600, 1200]:
>     for i in range(1,1025):
>         print dpi, i,
>         im = np.random.random((i,i))
>         fname = 'test{:03d}.png'.format(i)
>         plt.imsave(fname, im, dpi=dpi)
>         im = plt.imread(fname)[:,:,0]
>         print im.shape
>         assert im.shape == (i, i)
>         os.remove(fname)
>
> and everything passed.
>
> For completeness I also explored the dpi-space with these loop ranges and
> the above loop body:
>
> for dpi in range(1, 101):
>     for i in range(25, 36):
>         ...
>
> and all was still well.
>
>
> Workaround:
> For the moment I've set dpi=1 in my call to imsave which effectively
> reverts its behaviour to the original imsave code prior to the introduction
> of the dpi parameter.
>
> I think this testing is rigorous enough to make this change.
> If you agree, has anyone got time to change this, or shall I do a pull
> request when I have time?
>
> thanks,
> Gary
>
>
Good catch on this.  So you are saying that this bug was introduced
relatively recently with the dpi kwarg?  I would suggest you make a PR so
that this doesn't get lost (or at the very least file a bug report).  As
for the fix itself, off the top of my head, wouldn't math.ceil() do what we
want?  I prefer to be explicit in any intents rather than just simply (x +
0.5) / float(dpi).  As for the test, I would wonder if some sort of
restricted version of that test might not be good to do?  I am thinking
about 10 different sized plots and we wouldn't even need to have a the
assert check or file removal test, as the image comparison framework would
throw an exception when attempting to compare two images of different sizes
(I think...).

Cheers!
Ben Root

Re: [matplotlib-devel] Bug in imsave

[Michael D. <md...@st...>]

On 04/08/2013 02:38 PM, Benjamin Root wrote:
>
>
>
> On Sun, Apr 7, 2013 at 4:39 AM, gary ruben <gar...@gm... 
> <mailto:gar...@gm...>> wrote:
>
>     Hi, I haven't looked at this list for a long time, so hi to all
>     the active devs.
>     I just came across a problem in the image.py imsave() function.
>
>     Problem:
>     At an ipython --pylab prompt, typing
>
>     a = rand(29,29)
>     imsave('test.png', a)
>
>     incorrectly generates a 28x28 pixel image.
>     This happens for several array sizes (I initially saw it for 226x226).
>
>     Line 1272 of image.py is
>     figsize = [x / float(dpi) for x in (arr.shape[1], arr.shape[0])]
>
>     figsize interacts with the bounding box code somehow to create
>     off-by-one results for certain values of array shape.
>
>
>     Possible solution:
>     To fix this properly, I think the float cast should be removed and
>     integer-based math should be used, but I thought that since it was
>     rounding down in the cases I saw, a change to the following should
>     fix this:
>
>     figsize = [(x + 0.5) / float(dpi) for x in (arr.shape[1],
>     arr.shape[0])]
>
>     and indeed this seems to work.
>     To verify this, I ran the following brute-force test of all image
>     sizes up to 1024x1024 at some common dpi values:
>
>     import matplotlib.pyplot as plt
>     import numpy as np
>     import os
>
>     for dpi in [75, 100, 150, 300, 600, 1200]:
>         for i in range(1,1025):
>             print dpi, i,
>             im = np.random.random((i,i))
>             fname = 'test{:03d}.png'.format(i)
>             plt.imsave(fname, im, dpi=dpi)
>             im = plt.imread(fname)[:,:,0]
>             print im.shape
>             assert im.shape == (i, i)
>             os.remove(fname)
>
>     and everything passed.
>
>     For completeness I also explored the dpi-space with these loop
>     ranges and the above loop body:
>
>     for dpi in range(1, 101):
>         for i in range(25, 36):
>             ...
>
>     and all was still well.
>
>
>     Workaround:
>     For the moment I've set dpi=1 in my call to imsave which
>     effectively reverts its behaviour to the original imsave code
>     prior to the introduction of the dpi parameter.
>
>     I think this testing is rigorous enough to make this change.
>     If you agree, has anyone got time to change this, or shall I do a
>     pull request when I have time?
>
>     thanks,
>     Gary
>
>
> Good catch on this.  So you are saying that this bug was introduced 
> relatively recently with the dpi kwarg?  I would suggest you make a PR 
> so that this doesn't get lost (or at the very least file a bug 
> report).  As for the fix itself, off the top of my head, wouldn't 
> math.ceil() do what we want?  I prefer to be explicit in any intents 
> rather than just simply (x + 0.5) / float(dpi).  As for the test, I 
> would wonder if some sort of restricted version of that test might not 
> be good to do?  I am thinking about 10 different sized plots and we 
> wouldn't even need to have a the assert check or file removal test, as 
> the image comparison framework would throw an exception when 
> attempting to compare two images of different sizes (I think...).

I think this bug goes back at least to hash 17192518, by me in May 
2010.  The suggested fix (probably using ceil to be more explicit as you 
suggest) seems right.

As for the test, I think just `imsave` to a buffer, and loading that 
buffer back in with `imread` and asserting the sizes are the same should 
be sufficient.

Mike

Re: [matplotlib-devel] Bug in imsave

[gary r. <gar...@gm...>]

ceil doesn't work for me
I tried
figsize = [np.ceil(x / float(dpi)) for x in (arr.shape[1], arr.shape[0])]
and it fails on my second test when dpi=2 and i=25
result=26x26

Gary


On 9 April 2013 07:03, Michael Droettboom <md...@st...> wrote:

>  On 04/08/2013 02:38 PM, Benjamin Root wrote:
>
>
>
>
> On Sun, Apr 7, 2013 at 4:39 AM, gary ruben <gar...@gm...> wrote:
>
>>   Hi, I haven't looked at this list for a long time, so hi to all the
>> active devs.
>>  I just came across a problem in the image.py imsave() function.
>>
>> Problem:
>> At an ipython --pylab prompt, typing
>>
>> a = rand(29,29)
>> imsave('test.png', a)
>>
>>  incorrectly generates a 28x28 pixel image.
>> This happens for several array sizes (I initially saw it for 226x226).
>>
>> Line 1272 of image.py is
>> figsize = [x / float(dpi) for x in (arr.shape[1], arr.shape[0])]
>>
>> figsize interacts with the bounding box code somehow to create off-by-one
>> results for certain values of array shape.
>>
>>
>> Possible solution:
>> To fix this properly, I think the float cast should be removed and
>> integer-based math should be used, but I thought that since it was rounding
>> down in the cases I saw, a change to the following should fix this:
>>
>> figsize = [(x + 0.5) / float(dpi) for x in (arr.shape[1], arr.shape[0])]
>>
>>  and indeed this seems to work.
>>  To verify this, I ran the following brute-force test of all image sizes
>> up to 1024x1024 at some common dpi values:
>>
>> import matplotlib.pyplot as plt
>> import numpy as np
>> import os
>>
>> for dpi in [75, 100, 150, 300, 600, 1200]:
>>     for i in range(1,1025):
>>         print dpi, i,
>>         im = np.random.random((i,i))
>>         fname = 'test{:03d}.png'.format(i)
>>         plt.imsave(fname, im, dpi=dpi)
>>         im = plt.imread(fname)[:,:,0]
>>         print im.shape
>>         assert im.shape == (i, i)
>>         os.remove(fname)
>>
>>  and everything passed.
>>
>>  For completeness I also explored the dpi-space with these loop ranges
>> and the above loop body:
>>
>> for dpi in range(1, 101):
>>     for i in range(25, 36):
>>         ...
>>
>> and all was still well.
>>
>>
>>  Workaround:
>>  For the moment I've set dpi=1 in my call to imsave which effectively
>> reverts its behaviour to the original imsave code prior to the introduction
>> of the dpi parameter.
>>
>>  I think this testing is rigorous enough to make this change.
>>  If you agree, has anyone got time to change this, or shall I do a pull
>> request when I have time?
>>
>>  thanks,
>>  Gary
>>
>>
>  Good catch on this.  So you are saying that this bug was introduced
> relatively recently with the dpi kwarg?  I would suggest you make a PR so
> that this doesn't get lost (or at the very least file a bug report).  As
> for the fix itself, off the top of my head, wouldn't math.ceil() do what we
> want?  I prefer to be explicit in any intents rather than just simply (x +
> 0.5) / float(dpi).  As for the test, I would wonder if some sort of
> restricted version of that test might not be good to do?  I am thinking
> about 10 different sized plots and we wouldn't even need to have a the
> assert check or file removal test, as the image comparison framework would
> throw an exception when attempting to compare two images of different sizes
> (I think...).
>
>
> I think this bug goes back at least to hash 17192518, by me in May 2010.
> The suggested fix (probably using ceil to be more explicit as you suggest)
> seems right.
>
> As for the test, I think just `imsave` to a buffer, and loading that
> buffer back in with `imread` and asserting the sizes are the same should be
> sufficient.
>
> Mike
>
>
> ------------------------------------------------------------------------------
> Minimize network downtime and maximize team effectiveness.
> Reduce network management and security costs.Learn how to hire
> the most talented Cisco Certified professionals. Visit the
> Employer Resources Portal
> http://www.cisco.com/web/learning/employer_resources/index.html
> _______________________________________________
> Matplotlib-devel mailing list
> Mat...@li...
> https://lists.sourceforge.net/lists/listinfo/matplotlib-devel
>
>