Skip to content

Clarify meaning of facecolors for LineCollection #19170

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

Merged
merged 1 commit into from
Dec 25, 2020

Conversation

timhoffm
Copy link
Member

PR Summary

In response to #19168. Hopefully this is now better to understand.

@efiring
Copy link
Member

efiring commented Dec 24, 2020

The clarification looks good to me. I think the fundamental problem is that filling the polygons is a mistake. It is overloading LineCollection with functionality that it wasn't designed for, and that doesn't fit the name, because "we can". We could split that functionality out into a separate subclass with a better name and deprecate the filling functionality of LineCollection. This is in the spirit of @brunobeltran's effort to define the API we would like to end up with.

@timhoffm
Copy link
Member Author

I agree that LineCollection should not have facecolors, and the implemented meaning is odd at best.

For now, I'm not takeing up the topic though. This is just a small PR to document the oddness in the hope that it's less confusing. I'd like to get this merged without much fuss and move on.

Others are welcome to dig into it and propose API improvements in separate PRs. For example, I'm unclear if PathCollection is sufficient or if we really want something like a FilledLineCollection subclass. It could well be that it's ok to just deprecate facecolors in LineCollection.

@QuLogic QuLogic added this to the v3.4.0 milestone Dec 25, 2020
@QuLogic QuLogic merged commit 28b1f0a into matplotlib:master Dec 25, 2020
@timhoffm timhoffm deleted the doc-linecollection-facecolor branch December 25, 2020 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants