Skip to content

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

Merged
QuLogic merged 2 commits into
matplotlib:mainfrom
Kaustbh:patch
Mar 1, 2025
Merged

QuLogic merged 2 commits into
matplotlib:mainfrom
Kaustbh:patch

Conversation

@Kaustbh

@Kaustbh Kaustbh commented Feb 28, 2025

Copy link
Copy Markdown
Contributor

Fixes: #29261

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

@timhoffm

Copy link
Copy Markdown
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)).

Comment thread lib/matplotlib/tests/test_patches.py Resolved
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@QuLogic
QuLogic merged commit 03b74ea into matplotlib:main Mar 1, 2025
@QuLogic QuLogic added this to the v3.11.0 milestone Mar 1, 2025
@Kaustbh

Kaustbh commented Mar 2, 2025

Copy link
Copy Markdown
Contributor Author

@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

timhoffm commented Mar 4, 2025 •

Copy link
Copy Markdown
Member

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