Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Add test cases for patch.force_edgecolor behavior with facecolor="none" #29690

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 2 commits into from
Mar 1, 2025

Conversation

Kaustbh
Copy link
Contributor

@Kaustbh Kaustbh commented Feb 28, 2025

Fixes: #29261

This commit adds test cases to verify the behavior of patch.force_edgecolor when facecolor="none".

@timhoffm
Copy link
Member

This codifies current behavior. It does not fix #29690, but is a baseline so that we can do a reliable refactoring to fix #29690 without accidentally changing current behavior (see #29261 (comment)).

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@QuLogic QuLogic merged commit 03b74ea into matplotlib:main Mar 1, 2025
40 of 41 checks passed
@QuLogic QuLogic added this to the v3.11.0 milestone Mar 1, 2025
@Kaustbh
Copy link
Contributor Author

Kaustbh commented Mar 2, 2025

@timhoffm In the current behavior, when patch.force_edgecolor=False and facecolor="none", the artist becomes fully transparent (invisible). Should we first address this specific issue in the existing patch.force_edgecolor logic, or should we focus on #29261?

@timhoffm
Copy link
Member

timhoffm commented Mar 4, 2025

Sorry, I always get a knot in my brain when I try to think though all cases 😵. The fundamental goal of #29261 is to get rid of the confusing patch.force_edgecolor. I'm not clear whether we need to to fix a behavior that depends on that first. Ultimately, we want new parameters

#patch.edgecolor: "none"            # default edgecolor for Patch and Collection
#patch.edgecolor_fallback: "black"  # edgecolor to be used if facecolor is "none" to prevent completely invisible artists

and an as-smooth-as possible transition path to them - i.e. give themes/users with customized patch.force_edgecolor/patch_edgecolor alternatives to configure their behavior through the new parameters.

I suspect, but right now can't tell reliably, that changing part of the logic of patch.force_edgecolor before does not add a benefit because that'll be a breaking change, and we can as well do the break during the migration if needed. Therefore, I suggest that you move forward on #29261 and we'll see how well that goes. At worst, we'll have to take a step back, do the change patch.force_edgecolor and then come back to #29261 afterwards.

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.

[MNT]: Confusing edgecolor behavior for Patch and Collection
3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.