Skip to content
/ matplotlib Public

  • Sponsor matplotlib/matplotlib

  • Notifications You must be signed in to change notification settings
  • Fork 7.9k
  • Star 21.3k

[MNT]: Confusing edgecolor behavior for Patch and Collection #29261

New issue
New issue
Open
#29690
Open
[MNT]: Confusing edgecolor behavior for Patch and Collection#29261
#29690
Labels
Maintenance
@timhoffm

Description

@timhoffm
timhoffm
opened on Dec 8, 2024

Noted while reviewing documentation on the current behavoir in #29252.

Summary

Patches and Collections do not draw edges by default. This was introduced in #5596 for matplotlib 2.0. It seems that this was first achieved by a linewidth 0, but it this was changed to using an edgecolor of "none" while maintaining a finite linewidth.

As a consequence rcParams["patch.edgecolor"] does not have an effect by default. There are, however two cases when rcParams["patch.edgecolor"] becomes relevant:

  1. If facecolor is "none", Patch and Collection take rcParams["patch.edgecolor"] as edgecolor.
    I assume, this is done to prevent the artist being completely invisible (no facecolor and no edgecolor).
  2. If rcParams["patch.force_edgecolor"] is set to true, rcParams["patch.edgecolor"] is used instead of "none" as the

This is overall very confusing.

Ping @efiring, who introduced rcParams["patch.force_edgecolor"] in #6904. Can you comment on the background and whether the proposal below is a reasonable alternative?

Proposed fix

Can we switch to the following logic: patch.edgecolor is generally used as edgecolor and now defaults to "none" (to achieve the same behavior as before). To handle case 1. (prevent invisible artists) introduce a new rcParams["patch.edgecolor_fallback"] that has a special color only for that case.

Needed changes to default rcParams

matplotlibrc before:

#patch.edgecolor: black
#patch.force_edgecolor: False

matplotlibrc after

#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

Needed changes to styles

Existing styles would migrate

  • If patch.force_edgecolor was False:
    from: 
    patch.edgecolor: COLOR
#patch.force_edgecolor: False
    to 
    #patch.edgecolor: "none"
patch.edgecolor_fallback:  COLOR
  • If patch.force_edgecolor was True:
    from: 
    patch.edgecolor: COLOR
patch.force_edgecolor: True
    to 
    patch.edgecolor: COLOR
patch.edgecolor_fallback:  COLOR

Transition path

  • patch.force_edgecolor gets deprecated and raises a warning when used on rcParams.
  • styles that don't use any of patch.edgecolor or patch.force_edgecolor do not have to do anything.
  • styles that have set patch.edgecolor but not patch.force_edgecolor are a very edge case. They either have overlooked that the default force_edgecolor=False makes the setting ineffective, or they have intentionally only configured the somewhat arcane edge color fallback for non-filled artists. In the former case, they get "auto-fixed" through the new behavior. I'd argue that we can affort to not runtime-warn on the latter case (but if really wanted, one could add a check at least for loading stylesheets).

Activity

timhoffm
added
Maintenance
on Dec 8, 2024
efiring

efiring commented on Dec 12, 2024

@efiring
efiring
on Dec 12, 2024
Member

Your analysis and proposal look fine to me.

timhoffm
mentioned this on Dec 25, 2024
Kaustbh

Kaustbh commented on Feb 2, 2025

@Kaustbh
Kaustbh
on Feb 2, 2025 · edited by Kaustbh
Contributor

Hi @timhoffm ,

While testing the behavior of patch.edgecolor and patch.force_edgecolor, I encountered an problem where the artist becomes completely invisible when facecolor="None", which should not happen according to the case you mentioned above

If facecolor is "none", Patch and Collection take rcParams["patch.edgecolor"] as edgecolor.
I assume, this is done to prevent the artist being completely invisible (no facecolor and no edgecolor).

What I Did:
I created a simple Rectangle patch with facecolor="None", expecting the default patch.edgecolor to apply:

import matplotlib.pyplot as plt
import matplotlib.patches as patches

fig, ax = plt.subplots()
rect = patches.Rectangle((0.1, 0.1), 0.4, 0.4, facecolor="None")
ax.add_patch(rect)
plt.show()

Is this the expected behavior? Or did I miss something in my setup? Further, I’d like to contribute to this issue.

timhoffm

timhoffm commented on Feb 2, 2025

@timhoffm
timhoffm
on Feb 2, 2025
MemberAuthor

I can’t really tell on the expectation. I’ve just tried to deduce the current behavior, but I may have done a mistake.

I don’t have time to dig into this deeper right now. Anyway the way forward on this issue is

  1. Find out in which cases patch.edgecolor is used, and what patch.force_edgecolor exactly does.
  2. When we have this, we can come up with a migration strategy.
Kaustbh

Kaustbh commented on Feb 3, 2025

@Kaustbh
Kaustbh
on Feb 3, 2025
Contributor

Okay, I am working on it, If I encounter any specific questions or uncertainties during this process, I’ll reach out to you.

Kaustbh

Kaustbh commented on Feb 5, 2025

@Kaustbh
Kaustbh
on Feb 5, 2025 · edited by timhoffm
Contributor

Hi @timhoffm,
I analyzed how patch.edgecolor and patch.force_edgecolor are used in _set_edgecolor() and here’s what I found:

If color is explicitly provided, edgecolor uses that color directly.
If color is None, the function decides which edge color to use:

  • If patch.force_edgecolor=True, patch.edgecolor is applied .
  • If the patch is not filled (not self._fill) or the subclass enables _edge_default, patch.edgecolor is applied.
  • Otherwise, edgecolor is set to "none" (meaning no visible edge).

if we want change default patch.edgecolor we should set patch.force_edgecolor=True

the problem occurs when facecolor="None", and patch.force_edgecolor=False, we get an invisible artist, to fix this we can add a condition that if facecolor="None", then edgecolor will be default patch.edgecolor

Kaustbh

Kaustbh commented on Feb 9, 2025

@Kaustbh
Kaustbh
on Feb 9, 2025
Contributor

Hi @timhoffm, just checking in on my previous comment. Let me know if you have any thoughts on it or if I should clarify anything. Thanks!

Kaustbh

Kaustbh commented on Feb 10, 2025

@Kaustbh
Kaustbh
on Feb 10, 2025
Contributor

@timhoffm can you please take a look at the above comments.

timhoffm

timhoffm commented on Feb 11, 2025

@timhoffm
timhoffm
on Feb 11, 2025
MemberAuthor

Sorry, I don't have enough quiet time right now to really think this through. It's tying a knot in my brain when I try to have a quick look.

From what it seems, patch.force_edgecolor is quite used, so we need to be extra careful if we change this. I'm unclear what would be better: just explicitly documenting the behavior (and finding a good way of describing it) or coming up with a more natural logic.

Kaustbh

Kaustbh commented on Feb 17, 2025

@Kaustbh
Kaustbh
on Feb 17, 2025 · edited by Kaustbh
Contributor

Hi @timhoffm currently, if facecolor="None" and patch.force_edgecolor=False, the artist becomes invisible. For now, we can fix this.

timhoffm

timhoffm commented on Feb 17, 2025

@timhoffm
timhoffm
on Feb 17, 2025
MemberAuthor

I believe the best way forward is to codify the current (intended) behavior in tests. Then refactor and make sure the resulting edgecolor stays the same.

Kaustbh
mentioned this on Feb 28, 2025
Kaustbh

Kaustbh commented on Feb 28, 2025

@Kaustbh
Kaustbh
on Feb 28, 2025
Contributor

@timhoffm I have made the PR, containing tests cases for patch.force_edgecolor when facecolor="none", please review and suggest required changes.

QuLogic
closed this as completedin #29690on Mar 1, 2025

2 remaining items

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Maintenance

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions
      [MNT]: Confusing edgecolor behavior for Patch and Collection · Issue #29261 · matplotlib/matplotlib