Fix bugs in legend positioning with loc='best' #1640

Merged
merged 10 commits into from Feb 19, 2013

Projects

None yet

4 participants

@maxalbert
Contributor

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
Contributor
NelleV commented Jan 7, 2013

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 NelleV commented on the diff Jan 7, 2013
lib/matplotlib/legend.py
@@ -108,7 +108,7 @@ class Legend(Artist):
'upper center' : 9,
'center' : 10,
- loc can be a tuple of the noramilzed coordinate values with
+ loc can be a tuple of the normalized coordinate values with
@NelleV
NelleV Jan 7, 2013 Contributor

Good catch!

@maxalbert
Contributor

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
Contributor

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
Contributor
NelleV commented Jan 8, 2013

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
Contributor

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 mdboom commented on the diff Jan 16, 2013
lib/matplotlib/transforms.py
@@ -645,7 +645,7 @@ def count_contains(self, vertices):
dy0 = np.sign(vertices[:, 1] - y0)
dx1 = np.sign(vertices[:, 0] - x1)
dy1 = np.sign(vertices[:, 1] - y1)
- inside = (abs(dx0 + dx1) + abs(dy0 + dy1)) <= 2
+ inside = ((abs(dx0 + dx1) + abs(dy0 + dy1)) == 0)
@mdboom
mdboom Jan 16, 2013 Member

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

@efiring efiring merged commit d179f4b into matplotlib:master Feb 19, 2013

1 check failed

default The Travis build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment