-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Re-instate BboxConnectorPatch fill #8075
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
Conversation
Did you check that the examples this change was introduced to fix are not re-broken? |
I have just checked, and all the examples mentioned in #5757 still work as expected. |
@@ -312,7 +312,8 @@ def __init__(self, bbox1, bbox2, loc1, loc2=None, **kwargs): | |||
raise ValueError("transform should not be set") | |||
|
|||
kwargs["transform"] = IdentityTransform() | |||
Patch.__init__(self, fill=False, **kwargs) | |||
fill = 'fc' in kwargs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to also check for 'facecolor' and 'color'. Use cbook.normalize_kwargs
to sort out the aliases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't use normalize_kwargs before; something like kwargs = cbook.normalize_kwargs(kwargs, alias_mapping={'fc': ['facecolor', 'color']})
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See
matplotlib/lib/matplotlib/axes/_axes.py
Line 1406 in e69ea36
kwargs = cbook.normalize_kwargs(kwargs, _alias_map) |
You can use normalize_kwargs
to sort out fc
vs facecolor
, etc . You will have to do the color / facecolor / edgecolor normalization by hand I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the example, I should have fixed this now.
The latest commit should fix the issue if facecolor or color is given (or an alias) but I see a few potential problems.
Suggested Fix: from matplotlib.cbook import normalize_kwargs
import matplotlib.patches as Patches
.
.
.
kwargs = normalize_kwargs(kwargs, Patches._patch_alias_map)
if 'fill' in kwargs:
Patch.__init__(self, **kwargs)
else:
fill = ('facecolor' in kwargs) or ('color' in kwargs)
Patch.__init__(self, fill=fill, **kwargs) AND For mark_inset something like: kwargs = normalize_kwargs(kwargs, Patches._patch_alias_map)
if 'fill' in kwargs:
pp = BboxPatch(rect, **kwargs)
else:
fill = ('facecolor' in kwargs) or ('color' in kwargs)
pp = BboxPatch(rect, fill=fill, **kwargs) That way any valid input should be handled correctly. |
@Mr-Ian-Ferguson could you put your suggested changes in a new pull request? |
@Mr-Ian-Ferguson @dstansby What is the status of this? |
I don't plan to pick this up, but thought I'd leave the PR open for reference. |
Sorry I've been without internet for a while. I probably won't be picking this up again, just something I noticed in passing while looking for a different issue. That said I stand by my above suggestions and your welcome to use as you see fit; I just don't have time to do proper testing right now or do a full PR. |
I still have no plans to pick this up, so going to close. |
Fixes #8059. Also adds a simple image test for
BboxConnectorPatch
, which is currently not tested.