Bug in imsave

Development matplotlib-devel
1

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

2

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

···

On Sun, Apr 7, 2013 at 4:39 AM, gary ruben <gary.ruben@...149...> 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

3

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

···

On 04/08/2013 02:38 PM, Benjamin Root
wrote:

        On Sun, Apr 7, 2013 at 4:39 AM, gary

ruben <gary.ruben@…149…>
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…).

4

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 <mdroe@…31…> wrote:

  On 04/08/2013 02:38 PM, Benjamin Root

wrote:

I think this bug goes back at least to hash 17192518, by me in May
  1. 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

Matplotlib-devel@lists.sourceforge.net

https://lists.sourceforge.net/lists/listinfo/matplotlib-devel

        On Sun, Apr 7, 2013 at 4:39 AM, gary

ruben <gary.ruben@…149…>
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…).