Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
ENH: Change logging to warning when creating a legend with no labels#27172
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
ENH: Change logging to warning when creating a legend with no labels#27172
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Fixes#27145Also add test_legend_nolabels_warning()
This comment was marked as outdated.
This comment was marked as outdated.
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.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join uson gitter for real-time discussion.
For details on testing, writing docs, and our review process, please seethe developer guide
We strive to be a welcoming and open project. Please follow ourCode of Conduct.
This comment was marked as duplicate.
This comment was marked as duplicate.
With modified explanation where appropriate
Jacob-Stevens-Haas commentedOct 24, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Ok, I think I'm at a place I need help from a reviewer.
|
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.
You can ignore coverage. It's sometimes doing funny things because of our CI setup.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Also:DOC: Correct Axes docstring when no labels in legend.
Hey is there anything that I need to do to merge this? |
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.
Sorry this slipped through. Thanks for the reminder.
lib/matplotlib/tests/test_legend.py Outdated
| @@ -1072,7 +1073,7 @@ def test_legend_labelcolor_rcparam_markerfacecolor_short(): | |||
| def test_get_set_draggable(): | |||
| legend = plt.legend() | |||
| legend = plt.legend(labels=["mock data"]) | |||
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.
I'm somewhat uneasy about this pattern of providing labels when there is no data.
- Passing labels only is supported but discouraged.
- While currently supported, it feels, that passing more labels than we have data should possibly warn as well. At least it's non-sensical. - But that'd be a separate PR. For here, we should just not use this pattern.
I suggest to silence the warning explicitly rather than through a sketchy code construct: Decorate the function with
@pytest.mark.filterwarnings("ignore:No artists with labels found to put in legend")instead.
Also in the following test functions.
lib/matplotlib/tests/test_legend.py Outdated
| plt.plot([1, 2, 3]) | ||
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("ignore") | ||
| plt.legend() |
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.
Let's at least make sure, a legend is created and attached to the Axes:
| plt.legend() | |
| plt.legend() | |
| assertplt.gca().get_legend()isnotNone |
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.
Gotcha, makes sense! In addition, is pattern you suggested above of@pytest.mark.filterwarnings(...) preferable towarnings.catch_warnings()+simplefilter pattern?
Also assert that a legend is at least created
Thanks, and congratulations on your first contribution to Matplotlib! We hope to see you back. 😄 |
Uh oh!
There was an error while loading.Please reload this page.
Fixes#27145
Also add test_legend_nolabels_warning()
PR summary
After discussion in the linked issue, it was determined that
warnings.warn()was better thanlog.warning()for alerting users that we can't draw a legend if there are no labels. This PR implements the change fromlog.warning()towarnings.warn(actually,_api.warn_external())PR checklist