Skip to content
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

[Bug]: Argument types for handles and labels are too strict for method Axes.legend #27507

Closed
Daverball opened this issue Dec 12, 2023 · 5 comments · Fixed by #27535
Closed

[Bug]: Argument types for handles and labels are too strict for method Axes.legend #27507

Daverball opened this issue Dec 12, 2023 · 5 comments · Fixed by #27535
Milestone

Comments

@Daverball
Copy link

Bug summary

Axes.legend is annotated too strictly and incorrectly necessitates Sequence for handles and labels, even though any Iterable works. This also contradicts the docstring on the method.

Code for reproduction

handles, labels = ax.get_legend_handles_labels()
ax.legend(iter(handles), iter(labels))

Actual outcome

No overload variant of "legend" of "Axes" matches argument types "Iterator[Artist]", "Iterator[Any]" [call-overload]

Expected outcome

Passes without type error

Additional information

No response

Operating system

No response

Matplotlib Version

3.8.2

Matplotlib Backend

No response

Python version

3.10

Jupyter version

No response

Installation

pip

@Daverball Daverball changed the title [Bug]: Argument types for handles and labels are too strict for method Axes.legend` [Bug]: Argument types for handles and labels are too strict for method Axes.legend Dec 12, 2023
@ksunden
Copy link
Member

ksunden commented Dec 13, 2023

The docstring does call out "sequence", which matches the type hint, at least for the parameter description of handles... though admittedly the labels description is even more narrow as it specifically calls out list, and I think I avoided getting that specific with type hints...

That said, earlier in the body of the docstring, it does in fact say "iterable", so I think we should be consistent...

Speaking more generally, I'd argue that just because the current implementation works doesn't mean we must guarantee it going forward. The type hints are describing (to the best of our ability, note that they are still provisional) what we consider to be the formal API, deviating (well, at least narrowing), would be considered an API change.

@timhoffm
Copy link
Member

Note that we do not systematically make a distinction between, list, sequence and iterable in the docstring type

https://matplotlib.org/devdocs/devel/document.html#parameter-type-descriptions

Non-numeric homogeneous sequences are described as lists

We use this simplification to make the docs easier to read for less experienced python users.

This does not hold for type hints, which should be rather exact. IMO the problem with iterations us that they can be infinite, which of course is not a valid input. AFAIK, the type system cannot distinguish between finite and infinite iterators. So Sequence is too strict and Iterator would be too loose.

@Daverball
Copy link
Author

Daverball commented Dec 13, 2023

Sequence is also technically not strictly finite, while the implementation does need to provide __len__ and __getitem__, the __iter__ could technically still yield an infinite Iterator. While this would be a really esoteric thing to do, I'm just using this as an example to demonstrate that Python is a dynamic language, so the level of type safety you can get will always be somewhat limited.

I think the guiding principle for typing Python code should generally be the same as for good API design, namely the robustness principle "be conservative in what you send, be liberal in what you accept".

My main motivation for this change is memory efficiency, if I want to e.g. reverse the order of labels/handles in the legend I can currently do the following, if I ignore the type annotations:

handles, labels = ax.get_legend_handles_labels()
ax.legend(reversed(handles), reversed(labels))

If I follow the restriction I have to do:

handles, labels = ax.get_legend_handles_labels()
ax.legend(list(reversed(handles)), list(reversed(labels)))

Which creates two unnecessary lists.

Another use-case would be aggregating the legend from multiple subplots into a shared legend. Rather than creating a new list, I would like to be able to use itertools.chain.

Annotations imho should generally be guided by implementation first, and documentation second, especially if the terminology that was used is vague. I don't think the argument of guaranteeing forward compatibility through type annotations holds any water, if it worked previously, you will break people's code if you decide to go with a more narrow implementation and you will get bug reports.

So I think it's generally more productive for the type annotations on parameters to be as broad as possible and reflect reality. That way, if you do, for whatever reason, decide to make a breaking change, people that do use type checkers will know whether or not they are affected, and where they need to make changes.

@rcomer
Copy link
Member

rcomer commented Dec 13, 2023

At #27004 we added a test to ensure iterators would work…

@rcomer
Copy link
Member

rcomer commented Dec 15, 2023

IMO the problem with iterations us that they can be infinite, which of course is not a valid input. AFAIK, the type system cannot distinguish between finite and infinite iterators. So Sequence is too strict and Iterator would be too loose.

I'm not sure this should be a problem. If we have a parameter that has to be a float between 0 and 1, we would still type it as float regardless of the fact that some floats are invalid input. Anyway an infinite iterable will work OK for either handles or labels, as long as the other is finite.

I would vote for changing the hints to Iterable. Ultimately, we are just looping through these inputs

if handles and labels:
handles, labels = zip(*zip(handles, labels))
elif handles is not None and labels is None:
labels = [handle.get_label() for handle in handles]
elif labels is not None and handles is None:
# Get as many handles as there are labels.
handles = [handle for handle, label
in zip(_get_legend_handles(axs, handlers), labels)]

@Daverball seems to have a reasonable use-case, and we have a test so we will not accidentally break it.

@QuLogic QuLogic added this to the v3.8.3 milestone Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants