two gridspec patches (fix getitem bug and docs)

Development matplotlib-devel
1

Hi matplotlib developers,

I was answering a question on the -users list and came across a
gridspec __getitem__ bug which causes an infinite loop if the
user tries to iterate over it, because getitem never did a
sanity-check when given an integer key.

  from matplotlib import gridspec
  gs = gridspec.GridSpec(1,2)
  gs[100] # no error is given, a SubplotSpec is returned
  [x for x in gs] # causes infinite loop before applying patch

  # after applying the patch - which is just lines 171-172 in the
  # traceback below
  In [9]: gs[100]

gridspec_doc.diff (2.3 KB)

gridspec_getitem.diff (459 Bytes)

···

----------------------------------------------------------
  IndexError Traceback (most recent call last)
  
  ./matplotlib/<ipython console> in <module>()
  
  ./matplotlib/gridspec.pyc
  in __getitem__(self, key)
      170 key += total
      171 if key >= total or key < 0:
  --> 172 raise IndexError("index out of range")
      173 num1, num2 = key, None
      174
  
  IndexError: index out of range
  
  In [10]: [x for x in gs]
  Out[10]:
  
  [<matplotlib.gridspec.SubplotSpec object at 0x9b7edcc>,
   <matplotlib.gridspec.SubplotSpec object at 0x9b8834c>]

I'm also including a patch for the gridspec docs which create a
grid-of-grids using gridspec in a colorful manner. Note that
there, I explicitly create indexes in the right range, like so:

  outer_grid = GridSpec(4, 4)
  for i in xrange(16):
    inner_grid = GridSpecFromSubplotSpec(3, 3, subplot_spec=outer_grid[i])
    for j in xrange(9):
      ax = plt.Subplot(f, inner_grid[j])
      ...

because before applying the getitem patch described above, one
*can't* currently iterate over the spec itself like this:

  outer_grid = GridSpec(4, 4)
  for cell in outer_grid:
    inner_grid = GridSpecFromSubplotSpec(3, 3, subplot_spec=cell)
    for subcell in inner_grid:
      ax = plt.Subplot(f, subcell)
      ...

The doc patch also fixes the currently broken .. _gridspec-guide:
anchor name which can be seen peeking out at the top of:
http://matplotlib.sourceforge.net/users/gridspec.html

because there was previously a misplaced leading \ on line 1 of
doc/users/gridspec.rst

best,
--
Paul Ivanov
314 address only used for lists, off-list direct email at:
http://pirsquared.org | GPG/PGP key id: 0x0F3E28F7

2

2011/1/4 Paul Ivanov <pivanov314@…149…>

Hi matplotlib developers,

I was answering a question on the -users list and came across a

gridspec getitem bug which causes an infinite loop if the

user tries to iterate over it, because getitem never did a

sanity-check when given an integer key.

from matplotlib import gridspec

gs = gridspec.GridSpec(1,2)

gs[100] # no error is given, a SubplotSpec is returned

[x for x in gs] # causes infinite loop before applying patch

after applying the patch - which is just lines 171-172 in the

traceback below

In [9]: gs[100]

IndexError Traceback (most recent call last)

./matplotlib/ in ()

./matplotlib/gridspec.pyc

in getitem(self, key)

  170            key += total

  171        if key >= total or key < 0:

→ 172 raise IndexError(“index out of range”)

  173        num1, num2 = key, None

  174

IndexError: index out of range

Does this patch prevent the use of negative indexes? If so, then we might want to rethink that in order to be more consistent with numpy arrays and python lists…

Ben Root

3

Benjamin Root, on 2011-01-04 19:48, wrote:

2011/1/4 Paul Ivanov <pivanov314@...149...>

> Hi matplotlib developers,
>
> I was answering a question on the -users list and came across a
> gridspec __getitem__ bug which causes an infinite loop if the
> user tries to iterate over it, because getitem never did a
> sanity-check when given an integer key.
>
> from matplotlib import gridspec
> gs = gridspec.GridSpec(1,2)
> gs[100] # no error is given, a SubplotSpec is returned
> [x for x in gs] # causes infinite loop before applying patch
Does this patch prevent the use of negative indexes? If so, then we might
want to rethink that in order to be more consistent with numpy arrays and
python lists...

No, it does not prevent the use of negative indexes, and ensures
that the negative index is within the allowed bounds, so we do
the right thing. Here's the full patch for context:

lib/matplotlib/gridspec.py
169 else:
170 if key < 0:
171 key += total
172+ if key >= total or key < 0:
173+ raise IndexError("index out of range")
174 num1, num2 = key, None

As you can see, the reason line 172 has the key < 0 check is
because the allowed range of negative indexes gets dealt with on
the two preceding lines. So for gs=gridspec.GridSpec(1,2);
Both gs[-1] and gs[-2] are legal, but gs[-3] will raise an
IndexError.

···

--
Paul Ivanov
314 address only used for lists, off-list direct email at:
http://pirsquared.org | GPG/PGP key id: 0x0F3E28F7

4

Thanks for the patches.
I just committed the getitem patch to the trunk with slight modifications.
As I think this is not a serious bug, I didn't push it to the
maintenance branch.
I'll review the doc patch and commit it later this week.

On the other hand, I don't think it is a good idea to have the
GridSpec iterable.

While the code below could be handy,

outer_grid = GridSpec(4, 4)
for cell in outer_grid:
    ...

It could be confusing as the code below would not work.

for cell in outer_grid[:]:
   ...

In other words,

iter(outer_grid) != iter(outer_grid[:]).

outer_grid[:] is not iterable actually.
So my current inclination is to force the GridSpec also not iterable.
How others think?

Regards,

-JJ

5

Ah, I see… that makes much more sense then.

Thanks for clearing that up.

Ben Root

···

On Wed, Jan 5, 2011 at 12:11 AM, Paul Ivanov <pivanov314@…322…9…> wrote:

Benjamin Root, on 2011-01-04 19:48, wrote:

2011/1/4 Paul Ivanov <pivanov314@…149…>

Hi matplotlib developers,

I was answering a question on the -users list and came across a

gridspec getitem bug which causes an infinite loop if the

user tries to iterate over it, because getitem never did a

sanity-check when given an integer key.

from matplotlib import gridspec

gs = gridspec.GridSpec(1,2)

gs[100] # no error is given, a SubplotSpec is returned

[x for x in gs] # causes infinite loop before applying patch

Does this patch prevent the use of negative indexes? If so, then we might

want to rethink that in order to be more consistent with numpy arrays and

python lists…

No, it does not prevent the use of negative indexes, and ensures

that the negative index is within the allowed bounds, so we do

the right thing. Here’s the full patch for context:

lib/matplotlib/gridspec.py

169 else:

170 if key < 0:

171 key += total

172+ if key >= total or key < 0:

173+ raise IndexError(“index out of range”)

174 num1, num2 = key, None

As you can see, the reason line 172 has the key < 0 check is

because the allowed range of negative indexes gets dealt with on

the two preceding lines. So for gs=gridspec.GridSpec(1,2);

Both gs[-1] and gs[-2] are legal, but gs[-3] will raise an

IndexError.

–

Paul Ivanov

314 address only used for lists, off-list direct email at:

http://pirsquared.org | GPG/PGP key id: 0x0F3E28F7

6

Ben do you want to handle this submission? Since it is a bug, it can go into the branch.

Paul, would you like to have commit access? If so, please send me your sf id.

Thanks!
JDH

···

On Wed, Jan 5, 2011 at 9:45 AM, Benjamin Root <ben.root@…553…> wrote:

Ah, I see… that makes much more sense then.

Thanks for clearing that up.

7

Looks like Jae-Joon beat me to it.

Ben Root

···

On Wed, Jan 5, 2011 at 9:51 AM, John Hunter <jdh2358@…149…> wrote:

On Wed, Jan 5, 2011 at 9:45 AM, Benjamin Root <ben.root@…553…> wrote:

Ah, I see… that makes much more sense then.

Thanks for clearing that up.

Ben do you want to handle this submission? Since it is a bug, it can go into the branch.

Paul, would you like to have commit access? If so, please send me your sf id.

Thanks!
JDH