#24050 No error was thrown even number of handles mismatched labels #24061

tusharkulkarni008 · 2022-10-01T06:25:34Z

user named haferburg. used plot method bunch of times and then called legend. with less handles than labels. program didn't raise any error. now it should raise Value error

PR Summary

PR Checklist

Tests and Styling

Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

New features are documented, with examples if plot related.
New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
Documentation is sphinx and numpydoc compliant (the docs should build without error).

…d labels user named haferburg. used plot method bunch of times and then called legend. with less handles than labels. program didn't raise any error. now it should raise Value error

oscargus · 2022-10-01T13:26:51Z

Your PR would need a test that checks that the exception is raised. This should probably go into https://github.com/matplotlib/matplotlib/blob/main/lib/matplotlib/tests/test_legend.py

Here is a similar test to see how to check for an exception:

matplotlib/lib/matplotlib/tests/test_legend.py

Lines 255 to 260 in fd5cf5c

    
           def test_legend_positional_handles_only(self): 
        
               lines = plt.plot(range(10)) 
        
               with pytest.raises(TypeError, match='but found an Artist'): 
        
                   # a single arg is interpreted as labels 
        
                   # it's a common error to just pass handles 
        
                   plt.legend(lines)

(Also, note that #24063 solves the same issue.)

tusharkulkarni008 · 2022-10-01T14:20:53Z

@oscargus is ValueError the right type of error to raise?
I have never written tests before I will analyse the suggested tests and write accordingly

jklymak · 2022-10-01T15:18:37Z

So raising here is an API change. Are we ok raising without deprecating mismatched list lengths?

tacaswell · 2022-10-01T18:23:02Z

lib/matplotlib/legend.py

@@ -1260,6 +1260,8 @@ def _parse_legend_args(axs, *args, handles=None, labels=None, **kwargs):
    elif len(args) >= 2:
        handles, labels = args[:2]
        extra_args = args[2:]
+    elif len(handles)!= len(labels):


This condition will never be evaluated because the one of the other conditionals in this if/elif chain will always be True first.

Yes I see that now.
An entirely new block would do the trick or we should rethink like you said. This issue was tagged as good first issue so and solution seemed pretty straightforward at the time.will analyslze along your directives

I think an entirely new block should be enough. This conditional statement (L1222:L1262) is to assign values to labels and handles. Checking the validity of handles and labels should come in a new conditional statement after that.

tacaswell · 2022-10-01T18:27:40Z

There is also an explicit bit of code on L1226 that ensures that if handles and labels are passed as keywords they are trimmed to the same length. Given that we currently are positively discarding excess labels or handles, we will need to warn for one version before we start to raise.

@tusharkulkarni008 Can you look back at the git-blame of when L1226 came in to see if we had any discussion about it at the time?

We may want to change our minds about supporting un-equal numbers of handles/labels but we should understand why we used to support it and make sure we no longer agree with those reasons. While there are some things in Matplotlib that are they way they are because that was the first way someone thought to do something, most things in Matplotlib are there because someone at one point had a good reason for wanting it that way. It is important to understand why things are they way they are before we change them to avoid unintended side effects.

tusharkulkarni008 · 2022-10-02T02:12:35Z

Will do

timhoffm · 2022-10-02T07:25:09Z

Also, currently we accept iterables, so you have to handle the case that handles and/or labels don’t support __len__.

Please add tests as well.

tacaswell · 2022-10-02T20:28:52Z

so you have to handle the case that handles and/or labels don’t support len.

Can probably do this with zip_longest + a object() sentinel + checking to make sure we do not see the sentinel. That said, if we take iterators there may some usage where the labels are an infinite generators for the labels. Currently

from itertools import count
import matplotlib.pyplot as plt
import numpy as np

fig, ax = plt.subplots()

for j in range(5):
    ax.plot(np.arange(5) + j)

ax.legend(ax.lines, (f'line {j}' for j in count()))

works.

timhoffm · 2022-10-03T07:39:36Z

so you have to handle the case that handles and/or labels don’t support len.

Can probably do this with zip_longest + a object() sentinel + checking to make sure we do not see the sentinel. That said, if we take iterators there may some usage where the labels are an infinite generators for the labels. Currently

IMHO the simplest solution is the following:

Postpone the len-check to the very end of the function, where handles and labels variables are resolved. That way, we cover all sources of the handles and labels.
Wrap the len-check in a try-except block, and do nothing if you cannot determine the length. We need to continue accepting iterators for compatibility, but we don't need to jump arbitrary complex hoops to try and find out how may elements they will give only to be able to give a warning.

rcomer · 2022-10-03T08:22:41Z

I wonder if a warning that an input is being truncated would be enough for the user who requested an exception? I think that would avoid worries about api changes, and any user who wants to pass e.g. a generator can just suppress the warning if they wish.

rcomer · 2022-10-03T11:34:36Z

look back at the git-blame

I think this is not easy in this case as the code is old and has moved around. The code to truncate when both handles and labels were passed was added within a bugfix to ensure passing both in was handled at all.

I can’t see any discussion of the truncation there but my guess is to make it consistent with the case when only labels are passed in. That code can be traced all the way back to here. The commit that produced that appears to be where Matplotlib moved from subversion to git.

tacaswell · 2022-10-03T19:55:34Z

The commit that produced that appears to be where Matplotlib moved from subversion to git.

When the svn -> git conversion was done we lost like the first 6 commits (the first commit we have has svn path=/trunk/matplotlib/; revision=7 in it). My current theory is that because svn only gives diffs we see a whole lot of commits adding docs and examples that claim to fix bugs with no code changes. That commit is one where all the files got moved so there was a huge diff and we finally start to see the library.

The original svn commits can be accessed at https://sourceforge.net/p/matplotlib/code/1 etc, but I think it is impossible to graft those back into the tree in a useful way at this point. It also looks like there was a CVS -> SVN transition before the SVN -> git transition.

I think the combination of @timhoffm and @rcomer 's comments is best. We should warn if labels and handles have different lengths only if they have lengths and leave it as just a warning rather than a deprecation warning.

aqeelat · 2022-11-04T21:02:57Z

lib/matplotlib/legend.py

@@ -1260,6 +1260,8 @@ def _parse_legend_args(axs, *args, handles=None, labels=None, **kwargs):
    elif len(args) >= 2:
        handles, labels = args[:2]
        extra_args = args[2:]
+    elif len(handles)!= len(labels):


I think an entirely new block should be enough. This conditional statement (L1222:L1262) is to assign values to labels and handles. Checking the validity of handles and labels should come in a new conditional statement after that.

aqeelat · 2022-11-04T21:08:43Z

lib/matplotlib/legend.py

@@ -1260,6 +1260,8 @@ def _parse_legend_args(axs, *args, handles=None, labels=None, **kwargs):
    elif len(args) >= 2:
        handles, labels = args[:2]
        extra_args = args[2:]
+    elif len(handles)!= len(labels):
+        raise ValueError("fewer handles than labels or vice versa")


It would be helpful to provide a better error message. Maybe include the length of handles and labels to make it easier for the developer to understand what they did wrong.

Suggested change

raise ValueError("fewer handles than labels or vice versa")

raise ValueError(f"Mismatched number of handles and labels. len(handles)={len(handles)} and len(labels)={len(lables)}.")

Sorry didn't mean to set myself as a reviewer. I just wanted to give my comments to @tusharkulkarni008 since it is their first PR ever.

@aqeelat the developer guide encourages you to review 🙂
https://matplotlib.org/stable/devel/triage.html#working-on-prs-to-help-review

Sorry didn't mean to set myself as a reviewer. I just wanted to give my comments to @tusharkulkarni008 since it is their first PR ever.

Yes @aqeelat this is my first PR ever and earlier in this conversation other senior member pointed out some important in points in very detailed explanation. Since then I am observing PRs other people make to gain some footing to make more meaningful PRs.
Certainly your error message is more consistent with raised issue

tacaswell · 2022-12-16T19:17:00Z

I converted this to draft and moved it to the 3.8 milestone. Re-reading the comments I think the work that needs to be done is:

only check the length if the inputs actually have length
only warn because this previously "worked" and we do not want to suddenly start raising exceptions in user code that was running
add tests of all of the behavior

timhoffm · 2022-12-21T08:10:33Z

@tusharkulkarni008 Are you still interested in working on this? If so can we help you to move this forward?

rcomer · 2023-10-08T08:29:32Z

Now replaced by #27004

matplotlib#24050 No error was thrown even number of handles mismatche…

eb11496

…d labels user named haferburg. used plot method bunch of times and then called legend. with less handles than labels. program didn't raise any error. now it should raise Value error

timhoffm linked an issue Oct 1, 2022 that may be closed by this pull request

No error message in matplotlib.axes.Axes.legend() if there are more labels than handles #24050

Closed

oscargus mentioned this pull request Oct 1, 2022

Raises ValueError on unequal lengths of labels and handles input #24063

Closed

6 tasks

oscargus added topic: legend status: needs tests labels Oct 1, 2022

jklymak added API: changes API: argument checking labels Oct 1, 2022

tacaswell reviewed Oct 1, 2022

View reviewed changes

tacaswell added this to the v3.7.0 milestone Oct 1, 2022

This was referenced Oct 2, 2022

Attempt to produce an error message to indicate that there are less l… #24068

Closed

No error message in matplotlib.axes.Axes.legend() if there are more labels than handles #24050

Closed

aqeelat reviewed Nov 4, 2022

View reviewed changes

tacaswell marked this pull request as draft December 16, 2022 19:15

tacaswell modified the milestones: v3.7.0, v3.8.0 Dec 16, 2022

rcomer added the status: orphaned PR label Mar 10, 2023

github-actions bot added the status: needs rebase label Jun 27, 2023

ksunden modified the milestones: v3.8.0, v3.9.0 Aug 9, 2023

borgesaugusto mentioned this pull request Oct 5, 2023

Warning if handles and labels have a len mismatch #27004

Merged

5 tasks

rcomer requested a review from aqeelat October 5, 2023 21:17

rcomer closed this Oct 8, 2023

QuLogic added the status: superseded label Dec 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#24050 No error was thrown even number of handles mismatched labels #24061

#24050 No error was thrown even number of handles mismatched labels #24061

tusharkulkarni008 commented Oct 1, 2022 •

edited

oscargus commented Oct 1, 2022

tusharkulkarni008 commented Oct 1, 2022

jklymak commented Oct 1, 2022

tacaswell Oct 1, 2022

tusharkulkarni008 Oct 2, 2022

aqeelat Nov 4, 2022

tacaswell commented Oct 1, 2022

tusharkulkarni008 commented Oct 2, 2022

timhoffm commented Oct 2, 2022 •

edited

tacaswell commented Oct 2, 2022

timhoffm commented Oct 3, 2022

rcomer commented Oct 3, 2022 •

edited

rcomer commented Oct 3, 2022

tacaswell commented Oct 3, 2022

aqeelat Nov 4, 2022

aqeelat Nov 4, 2022

aqeelat Nov 4, 2022

rcomer Nov 4, 2022

tusharkulkarni008 Nov 5, 2022

tacaswell commented Dec 16, 2022 •

edited by timhoffm

timhoffm commented Dec 21, 2022

rcomer commented Oct 8, 2023

	raise ValueError("fewer handles than labels or vice versa")
	raise ValueError(f"Mismatched number of handles and labels. len(handles)={len(handles)} and len(labels)={len(lables)}.")

#24050 No error was thrown even number of handles mismatched labels #24061

#24050 No error was thrown even number of handles mismatched labels #24061

Conversation

tusharkulkarni008 commented Oct 1, 2022 • edited

PR Summary

PR Checklist

oscargus commented Oct 1, 2022

tusharkulkarni008 commented Oct 1, 2022

jklymak commented Oct 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Oct 1, 2022

tusharkulkarni008 commented Oct 2, 2022

timhoffm commented Oct 2, 2022 • edited

tacaswell commented Oct 2, 2022

timhoffm commented Oct 3, 2022

rcomer commented Oct 3, 2022 • edited

rcomer commented Oct 3, 2022

tacaswell commented Oct 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Dec 16, 2022 • edited by timhoffm

timhoffm commented Dec 21, 2022

rcomer commented Oct 8, 2023

tusharkulkarni008 commented Oct 1, 2022 •

edited

timhoffm commented Oct 2, 2022 •

edited

rcomer commented Oct 3, 2022 •

edited

tacaswell commented Dec 16, 2022 •

edited by timhoffm