-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix collection legend handlers #7832
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
Fix collection legend handlers #7832
Conversation
As part of scaling dashes with linewidth, unscaled versions of the dash pattern are now tracked. Make sure we copy these as well.
To prevent double-scaling of the pattern in the legend Closes matplotlib#7814
@@ -788,6 +788,7 @@ def update_from(self, other): | |||
self._facecolors = other._facecolors | |||
self._linewidths = other._linewidths | |||
self._linestyles = other._linestyles | |||
self._us_linestyles = other._us_linestyles |
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.
Is there anywhere that documents that 'us' means unscaled? It's rather an opaque prefix to me.
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.
no, but I would prefer that be done in a seperate PR (created #7837 to track this).
@@ -17,7 +17,7 @@ | |||
import matplotlib as mpl | |||
import matplotlib.patches as mpatches | |||
import matplotlib.transforms as mtrans | |||
|
|||
import matplotlib.collections as mc |
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.
It's not very consistent, but this is generally imported as mcoll
or mcollections
. Also, second blank line should be re-added.
h1, h2, h3 = leg.legendHandles | ||
|
||
for oh, lh in zip((lc1, lc2, lc3), (h1, h2, h3)): | ||
assert oh.get_linestyles()[0][1] == lh._dashSeq |
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.
Since this is going in 2.0, it probably should be assert_equal
.
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.
assert
will work fine, though it is not as elegant.
h1, h2, h3 = leg.legendHandles | ||
|
||
for oh, lh in zip((lc1, lc2, lc3), (h1, h2, h3)): | ||
assert oh.get_linestyles()[0][1] == lh._dashSeq |
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.
assert
will work fine, though it is not as elegant.
I have the same concerns as @QuLogic concerning the opaque prefix. It looks good to me. |
Despite having written it I am un-willing to defend the |
going to leave the asserts to save the effort of converting them back to asserts in the near-to-mid future. |
No description provided.