_label suppressing a label is documented behavior, so I think we only want to warn on it in the case that handles are empty, as a "hey, did you mean to put an _` here?

Eta: thanks for this PR, I think it's a good idea to separate these out and be more specific (and @anntzer also says it'd probably be the better move in the original PR), so my concern is more about placement then content.

Hey, sure that makes sense. While the original intention is correct from a developer perspective, it felt a good idea to give the user a better idea of whats going on. Maybe even a link to the same documentation you posted.

If it is about the placement of the log message, I might have missed something but it seemed that detecting this in the _parse_legend_args was possible but could have impacted from a computational standpoint, especially when compared to the alternative. I'd love to know your thoughts on this.

but it seemed that detecting this in the _parse_legend_args was possible but could have impacted from a computational standpoint

Yeah, that was Anthony's point too. I honestly don't know if we're allowed to assume things like the list of labels must all be strings, b/c then I wonder if you could do something like (and rereading the original issue, I think the warning is then if there are handles and no labels)?

if handles and np.char.startswith(labels, '_').all():
   log,warning("....")

and what the computational cost would be for that?

I think where I'm tripping a little is we don't want to warn just on the use of '_' b/c we have an example that uses that property for grouping

As an aside, the numpy method works with unicode:

l1 = ['_a', '_ඖ', '_c']
l2 = ['a', '_ඖ', '_c']
print(np.char.startswith(l1, '_').all()) # returns True
print(np.char.startswith(l2, '_').all()) # returns False

I looked into numpy and while that was crazy fast, I felt integrating the check within the for-loop would serve to minimize code. I realized that the distinction we needed to create is the case when we have all our labels starting with an underscore vs no labels found at all. In both cases the handle is empty.

The design of this flag condition took an embarrassingly long time time but I believe this should do the trick. Let me know if I have missed something here, my brain gets muddled working with too many boolean thoughts at a time.

I felt integrating the check within the for-loop would serve to minimize code.

Can you please explain this reasoning a bit more?

Can you please explain this reasoning a bit more?

Sure, for any number of handles, since we are already looping over them, it made better sense to have already gotten our required information as soon as the loop has ended.

Another method would have been to get a temp variable that adds the labels regardless of the underscore and then use the numpy function. This would probably require an added check for empty list before calling np.char.startswith().

This PR needs more context if possible. What issue are you trying to fix? Please include a before and after example that shows the changes and/or add a test.

It looks like that there is not a case where a label does not exist. The get_label() function adds placeholder labels prefixed with _child to any cases where the user does not provide a label. This removes the distinction between a user not giving labels vs accidentally giving labels prefixed with underscores. I'll take a deeper look to see if this is feasible. Would appreciate any ideas here.

This PR needs more context if possible. What issue are you trying to fix? Please include a before and after example that shows the changes and/or add a test.

Please keep in mind that I first encountered this as a user so I'll try to explain from that perspective. What I basically want is a distinction between the two cases

# case1
plt.plot(x, y1)
plt.legend()

# case2
plt.plot(x, y1, label="_label1")
plt.plot(x, y2, label="_label2")
plt.legend()

I feel there is a difference between the two users in helping them find where the error lies, hence there should be different error messages. Moreover, its possible the second user knows the way around better but simply forgot to add a non-suppressing label. There is a distinction to be had here which I am trying to create.

I think it's easier to just have one verbose warning that covers both cases, which is what we currently have.

It looks like that there is not a case where a label does not exist.

Yeah that's why the test is on whether handles exist, b/c if you don't have handles than that's caught by the existing message.

It's kind of I think:

if not handles:
    log.warn("no handles")
elif np.char.startswith(labels, "_").all():
    log.warn("all _") 

I think we are unfortunately stuck here, the time when we know if the user passed a label is at creation time (when we do not know if the user intends to make a legend or not) and the time when we check if it is a label we should pay attention to is at legend creation time (when we no longer know if they passed a label explicitly).

If the user does not pass a label we, as @AALAM98mod100 noted, generate an quasi-unique label prefixed by '_'. This has a couple of nice design features in that the label is always a simple string, we do not have to carry any extra state that it should be ignored, and the convention of things prefixed with '_' "rhymes" with Python conventions. But the cost is at legend time we do not know of the label that starts with '_' is from us or from the user (pattern matching to see if it looks like a string we would produce is brittle and would erroneously pass a label the user did pass if it happened to match) to give a finer-grained warning.

Even if we could find away around this, I am with Jody on this, I think one warning that states "we found nothing with a label" and gives the hint "labels that start with underscores are ignored" is pretty good. A user story of why two warnings would be worse is first the user does plt.plot(...), get the "no handles found" warning and does plt.plot(..., label='_label') and then gets a different warning and is then frustrated why we did not give them good directions in the first warning.

I think the two cases we can tell apart is if there are no candidate artists and no candidate artists that have usable labels, so

fig, ax = plt.subplots()
ax.legend()

fig, ax = plt.subplots()
ax.plot(range(5))
ax.plot(range(5)[::-], label='_child1')
ax.legend()

but I am not sure that is worth two warnings.

One other consequence of this is because
_get_legend_handles_labels is called one other place, are we sure we want it to warn there as well?

first the user does plt.plot(...), get the "no handles found" warning and does plt.plot(..., label='_label') and then gets a different warning and is then frustrated why we did not give them good directions in the first warning.

I guess my question is what is the user intending to do that they would trigger both these warnings separately? Like why would they follow up plt.plot(..) with plt.plot(..., label='_label')?

I think it's easier to just have one verbose warning that covers both cases, which is what we currently have.

I've had some time to think on this and I am onboard with this line of thinking considering @tacaswell's reply as well (no pun intended).

But then I would like to take it up with the wording of the error message itself. Is it okay to expect an average user to understand what Artist means in the context? If not, I believe we can try improving that but I can't say what would be a better warning message here that is still generic enough.

In the case an average user can be expected to understand the error from the message, then we can close this PR.

Is it okay to expect an average user to understand what Artist means in the context? I

I think we sorta have to because a matplotlib artist is the core object (that's what ax.plot and ax.scatter is returning give or take) while words like plot can take on way too many meanings, like is it the plot the single line or the axes containing the line and scatter and all the other things.

But suggestions for making that clearer would be much appreciated.

I'm going to move this to draft, but feel to move back or request that it be moved back.

I think the general agreement among the devs is that we don't want to move forward with this. Feel free to reopen if you disagree.
Thanks for opening the discussion, though :)