-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix bugs in legend positioning with loc='best' #1640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ertices. This bug was not triggered before because count_contains() was never called due to the empty vertex list in Legend._find_best_position().
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): Can you have a look at this? Cheers, |
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
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 Thanks, 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? |
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? |
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. |
…e indeed taken into account.
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, |
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I'm not sure what my original thinking was here, but it doesn't make much sense to me now.
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.