-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
New textcolor kwarg for legend #15898
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
This is awesome. Is it possible to include an option to hide the handles? |
@Tillsten: Glad you like it! Yes, if the text color matches the line, then the next logical step for some graphs is to remove the handles (lines and markers). This Stackoverflow post discusses various methods for removing the handles from the legend, so clearly there is some desire for users to do this. But, I think that suppressing the handles should be a separate discussion and pull request. First, I don't currently know the best way to do this. Second, I think that it would require a separate kwarg. Shall I go ahead and open a new issue for us to discuss this feature request? Also, I see that the circleCI tests are failing. As far as I can tell, the only error is:
So, I'm not quite sure how to fix this. Help would be appreciated. |
Here is some code to test the various options for import numpy as np
import matplotlib.pyplot as plt
fig, axs = plt.subplots(2,3,figsize=(10,6), tight_layout=True)
x = np.linspace(0,2,5)
textcolors = 'linecolor', 'mfc', 'mec', 'red', 'blue', ['r','g','b']
for ax, textcolor in zip(axs.ravel(), textcolors):
for n in np.arange(1,4):
ax.plot(x, n*x, label='Line #%i'%n, marker='o', mec='m', mfc='b')
if type(textcolor) is str:
ax.legend(title='textcolor=\'%s\''%textcolor, textcolor=textcolor)
else:
ax.legend(title='textcolor=%s'%textcolor, textcolor=textcolor)
plt.show() And the output: |
If you are at the point of hiding the handles, I'd argue you don't need Overall, this looks nice and compact so seems to me to be a good addition... |
If I have only few solid colored lines, the handles could be seen as unnecessary clutter... 😈 |
I think this looks like a great idea! However, I think this needs a bit more work to make sure it will handle more artist types (it should at least behave correctly for Lines if other artist types are in the legend) and to make sure all of the valid ways we pass colors through (list / tuple / ndarray of RGB(A)) are supported. |
Okay, I reworked this to accommodate patches (it now uses Also, this version should allow for RGB(A) tuples, np.ndarrays, and lists of RGB(A) tuples. Here is a new version of my example code, which includes a rectangle patch: import numpy as np
import matplotlib.pyplot as plt
import matplotlib.patches as patches
fig, axs = plt.subplots(2,3,figsize=(10,6), tight_layout=True)
x = np.linspace(0,2,5)
textcolors = ['linecolor', 'mfc', 'mec', 'blue', ['r','g','b','m'], np.array((1,0,0,1))]
labels = ['\'linecolor\'', '\'mfc\'', '\'mec\'', '\'blue\'', '[\'r\',\'g\',\'b\',\'m\']', 'np.array((1,0,0,1))']
for ax, textcolor, label in zip(axs.ravel(), textcolors, labels):
rect = patches.Rectangle((1.3,0),.5,.7,linewidth=1,edgecolor='r',facecolor='g', label='Rect patch')
ax.add_patch(rect)
for n in np.arange(1,4):
ax.plot(x, n*x, label='Line #%i'%n, marker='o', mec='m', mfc='b')
ax.legend(title='textcolor=%s'%label, textcolor=textcolor)
plt.show() As you can see, in the case where Also, the circleCI tests are still failing, and I'm not sure why... |
Download the log - you have a bunch of warnings and we fail on warnings. I'm now a little leery of all the special casing we have to do now to meet all the possible input types. For the other devs: is there no way to abstract this logic so its done consistently in places that accept a list of colors in various forms? |
The first part is special to legends and it's quite involved because one has to obtain the color from the handle in the right way. It can be reduced to:
I've left out "mfc" and "mec" because I'm not sure we want to support the abbreviations as well. But they could be added to the For explict colors (single color or iterable)
should probably work. |
I re-worked the code to use the suggestions by @timhoffm, and I think that it's much more concise and readable now. I used Tim's code nearly verbatim, with the additions of a check to ensure that that elif type(textcolor) is str and textcolor in color_getters: I also included the |
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.
Looks much better 👍 Just some small comments left.
Thanks @timhoffm! I made the changes that you suggested. |
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 to go! Should be squashed before merging - either by the author (squash+force-push) or by the second reviewer when merging though the github UI.
Thanks @timhoffm! If it's okay, I'll leave the squashing to the second reviewer (@tacaswell?). Rebasing has always been a confusing process for me. |
For info: I usually do |
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.
Given the typo with edgecolor/facecolor, I think this needs tests for those as well.
Thanks for all of the good suggestions @QuLogic! I made all of the changes that you suggested, and I included the tests for |
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.
Just two minor things, LGTM otherwise.
@DanHickstein are you interested in picking this up? It just needs a rebase and a small typo fix. |
Thanks for the reminder! I made the changes. Can you squash when you merge? I always mess things up when I try to rebase. (I've been using Git for several years, but somehow I am still a noob. :) ) |
1efbafb
to
c6fd288
Compare
This had conflicts, so it needed to be rebased before merging. I've done that now. |
Looking back at the original issue, I see a list of several other 'color' values:
Only a few of those are handled, do we want to add those others as well? And also change the name from |
+1 for I would also like the other color value options, but that may also be added in a later PR. |
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.
Blocking to make sure we switch to labelcolor
before merging.
Modified legend.py to include the new labelcolor kwarg. Also included a short description in the "whats new" documentation and tests of the new functionality.
There's probably no need to hold this up for a quick rename. I went ahead and did the squash, rebased, and renamed the argument. |
Thanks @QuLogic! Sorry to have dropped the ball one this :) I think that the "labelcolor" keyword is fine. |
Hello, how to apply this with 3 y-axis and one common legend? |
@byetlinker - please discuss on discourse.matplotlib.org. Thanks, |
As discussed in #10720, I've made a first attempt to introduce a
textcolor
keyword argument for thelegend
method. The legend argument can either be:'linecolor'
,'markerfacecolor'
, and'markeredgecolor'
.Also I've included a short description in the "whats new" documentation and tests of the new functionality.
Please have a look at what I have so far and let me know how it can be improved.
PR Checklist