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]: deprecate auto hatch fallback to patch.edgecolor when edgecolor='None' #29380

Open
story645 opened this issue Dec 25, 2024 · 2 comments
Open

Comments

@story645
Copy link
Member

story645 commented Dec 25, 2024

Summary

Since #28104 now separates out hatchcolor, users should not be allowed to specifically ask to fallback to edgecolor and also explicitly set that edgecolor to none because silently falling back on edgecolor="None" introduces the problem that:

  1. because of eager color resolution and frequently setting "none" by knocking out the alpha, the way to check for none is checking the alpha, which leads to fallback being dependent on the interplay between alpha and edgecolor:
    Figure_1

  2. fallback to edgecolor rcParams doesn't check if the rcParam is also none so could sink the problem a layer deeper

Proposed fix

Deprecate this fallback behavior and raise a warning on

Rectangle( (0,0), .5, .5, hatchcolor='edge', edgecolor='None')

The alternatives are:

  • set a hatchcolor
  • don't set edgecolor at all, .i.e. Rectangle( (0,0), .5, .5, hatchcolor='edge') falls back to the rcParam in get_edgecolor
@timhoffm
Copy link
Member

I think the most common case is that the style hides the edge (as does our default style). As a result, since hatch and edge have been the same color so far, the hatch would be hidden as well, which would be very surprising. I believe the fallback was necessary to avoid the surprise and be able to get hatching without edges.

The argument does not hold anymore now that the hatch color is separate. So, a new discussion is possible.

users should not be allowed to specifically ask to fallback to edgecolor and also explicitly set that edgecolor to none

I'm stumbling over specifically and explicitly here. I suspect you want to detect whether the user did this intentionally. But there are possibly multiple input sources: rcParams, the cycler, kwargs, setters. We are not able to track whether the effective value is an intentional combination of both or partly a results of defaults. The only sane approach is to handle the situation from the actual values, no matter where they cane from. We can then debate how hatchcolor="edge" with an invisible edge is handled.

  • as in the past with a further fallback value
  • Or by waning/error that one attempts to draw a hatch with an invisible color.

possibly related

  • the strange edgecolor behavior [MNT]: Confusing edgecolor behavior for Patch and Collection #29261
  • One could debate whether hatchcolor="edge" should be allowed in the long run. For a smooth and compatible introduction of hatchcolor, we need that. But I'm not sure I would otherwise design it in up-front. These indirections complicate code and logic. OTOH one can argue, that matching edge and hatch should be easy to express, and we have a similar precedence for edgecolor="face".

@story645
Copy link
Member Author

story645 commented Dec 26, 2024

I suspect you want to detect whether the user did this intentionally.

All I mean by this is that we should not fallback on call signatures of Rectangle( (0,0), .5, .5, hatchcolor='edge', edgecolor='None').

The only sane approach is to handle the situation from the actual values, no matter where they came from.

As far as I can tell, by the time this code sees the color it's been resolved such that none can't be distinguished from an arbitrary color with it's alpha knocked out, which is why there's the inconsistency 'caused by alpha values above.

Or by waning/error that one attempts to draw a hatch with an invisible color.

That's what I think should happen now that hatchcolor is a separate parameter and edge and hatch can be decoupled, but I think this should happen as part of deprecating this fallback. I don't like the fallback because it's dependent on edge and alpha.
The use cased I'd been thinking of was animations - user may want to start with edge + hatch being ("Green, 0") and then up the alpha as the animation progresses.

Or start at hatch='edge', edge='None' and then set the color. Which is the call signature I'd started by saying we should warn on/disallow, and now I'm thinking we should allow but also assume that this is intentional since the user is setting all these values and therefore not fallback but maybe warn the users that what they're doing is gonna lead to an invisible hatch.

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