Fix bugs in legend positioning with loc='best'

[maxalbert]

This PR fixes a couple of bugs which caused the legend positioning algorithm to ignore vertices when figuring out the best location of the legend box.

[NelleV]

Hi @maxalbert,

Thanks for this patch. I ran the tests on your branch, and unfortunately they don't pass. Here is the traceback:

Traceback (most recent call last):
File "/home/nelle/.work/local/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
self.test(_self.arg)
File "/home/nelle/.work/local/lib/python2.7/site-packages/matplotlib/testing/decorators.py", line 39, in failer
result = f(_args, *_kwargs)
File "/home/nelle/.work/local/lib/python2.7/site-packages/matplotlib/testing/decorators.py", line 145, in do_test
figure.savefig(actual_fname, *_self._savefig_kwarg)
File "/home/nelle/.work/local/lib/python2.7/site-packages/matplotlib/figure.py", line 1363, in savefig
self.canvas.print_figure(_args, *_kwargs)
File "/home/nelle/.work/local/lib/python2.7/site-packages/matplotlib/backend_bases.py", line 2127, in print_figure
*_kwargs)
File "/home/nelle/.work/local/lib/python2.7/site-packages/matplotlib/backends/backend_agg.py", line 491, in print_png
FigureCanvasAgg.draw(self)
File "/home/nelle/.work/local/lib/python2.7/site-packages/matplotlib/backends/backend_agg.py", line 439, in draw
self.figure.draw(self.renderer)
File "/home/nelle/.work/local/lib/python2.7/site-packages/matplotlib/artist.py", line 54, in draw_wrapper
draw(artist, renderer, *args, *_kwargs)
File "/home/nelle/.work/local/lib/python2.7/site-packages/matplotlib/figure.py", line 999, in draw
func(_args)
File "/home/nelle/.work/local/lib/python2.7/site-packages/matplotlib/artist.py", line 54, in draw_wrapper
draw(artist, renderer, *args, *_kwargs)
File "/home/nelle/.work/local/lib/python2.7/site-packages/matplotlib/axes.py", line 2093, in draw
a.draw(renderer)
File "/home/nelle/.work/local/lib/python2.7/site-packages/matplotlib/artist.py", line 54, in draw_wrapper
draw(artist, renderer, _args, *_kwargs)
File "/home/nelle/.work/local/lib/python2.7/site-packages/matplotlib/legend.py", line 465, in draw
bbox = self._legend_box.get_window_extent(renderer)
File "/home/nelle/.work/local/lib/python2.7/site-packages/matplotlib/offsetbox.py", line 244, in get_window_extent
px, py = self.get_offset(w, h, xd, yd, renderer)
File "/home/nelle/.work/local/lib/python2.7/site-packages/matplotlib/offsetbox.py", line 197, in get_offset
return self._offset(width, height, xdescent, ydescent, renderer)
File "/home/nelle/.work/local/lib/python2.7/site-packages/matplotlib/legend.py", line 428, in _findoffset_best
ox, oy = self._find_best_position(width, height, renderer)
File "/home/nelle/.work/local/lib/python2.7/site-packages/matplotlib/legend.py", line 916, in _find_best_position
verts, bboxes, lines = self._auto_legend_data()
File "/home/nelle/.work/local/lib/python2.7/site-packages/matplotlib/legend.py", line 752, in _auto_legend_data
vertices = np.concatenate([l.vertices for l in lines])
ValueError: concatenation of zero-length sequences is impossible

Can you have a look at this?

Cheers,
N

[NelleV]

Good catch!

[maxalbert]

Hi @NelleV,

many thanks for the lightning-speed reply! :) (And apologies in advance if my responses come a bit slower, I'm on a rather intermittent/flaky internet connection for at least another week or so.)

I tried to take a look at this, but (un?)-fortunately can't reproduce the error as the test suite passes for me. Maybe I'm doing something wrong in running the tests? I'm calling python tests.py from the toplevel directory, is there something else I need to do?

Thanks,
Max

P.S.: It just occurred to me that I have multiple versions of matplotlib lying around but ran the test suite in a fresh checkout of my branch which I didn't install first. I'll check if this could be the cause of the missing error. Do you know if the test suite picks up the version of matplotlib from the repository that it is being run in, or does it use the system-wide installation?

[maxalbert]

Update: looks like the latter is true, so it was indeed a case of one installation interfering with another. I now get the same error and will take a look. Btw, would it be worth to make the test suite automatically pick up the matplotlib version from the repository that it is being run in, or am I missing any hidden pitfalls that this might lead to?

[NelleV]

I think it is possible, if the code is build inplace. That's something I'd like to have a look at, as soon as I'm finished with all my open branches (or someone else could have a look at it). I think it may require some code refactoring.

[maxalbert]

Hi @NelleV,

the test failure was trivial to fix (as expected from the error message), but it took me a while to upload it because I wanted to add at least one test case. Could you try again and see if it works for you now, too?

Originally I had planned to add a few more tests, but unfortunately in preparing them I realised that the code is still not optimal (see the FIXME comment in commit 49ee4e8). Alas, a proper fix might result in increased running times in case there are a lot of vertices in a plot, although this may not be an issue. I do not have time to look into this at the moment or to do some profiling, but wanted to at least mention it.

Thanks again for your time, any further comments/suggestions are appreciated.

Cheers,
Max

[mdboom]

Good catch! I'm not sure what my original thinking was here, but it doesn't make much sense to me now.