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

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

Open
timhoffm opened this issue Dec 8, 2024 · 1 comment
Open

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

timhoffm opened this issue Dec 8, 2024 · 1 comment

Comments

@timhoffm
Copy link
Member

timhoffm commented 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).
@efiring
Copy link
Member

efiring commented Dec 12, 2024

Your analysis and proposal look fine to me.

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

No branches or pull requests

2 participants