Repository navigation
Make _get_rgba_face actually always return a RGBA. - #9911
Conversation
56c8925 to
2d962a6
Compare
| edgecolor = self.get_markeredgecolor() | ||
| if (isinstance(edgecolor, six.string_types) | ||
| and edgecolor.lower() == 'none'): | ||
| if cbook._str_equal(edgecolor, "none"): |
There was a problem hiding this comment.
Original code has edgecolor.lower(), I do not see any normalization in set_markeredgecolor/get_markeredgecolor. Did I miss something?
| else: | ||
| rgbaFace = mcolors.to_rgba(facecolor, self._alpha) | ||
| return rgbaFace | ||
| return mcolors.to_rgba(self._get_markerfacecolor(alt=alt), self._alpha) |
There was a problem hiding this comment.
to_rgba already handles the lowercasing (https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/colors.py#L146).
8743dce to
85191a0
Compare
|
|
||
| This helper solely exists to handle the case where *obj* is a numpy array. | ||
| """ | ||
| return isinstance(obj, six.string_types) and obj.lower() == s |
85191a0 to
038306a
Compare
038306a to
47e25e9
Compare
efiring
left a comment
efiring
left a comment
There was a problem hiding this comment.
Apart from the docstrings I flagged, it looks fine.
| def _str_equal(obj, s): | ||
| """Return whether *obj* is a string equal to string *s*. | ||
|
|
||
| This helper solely exists to handle the case where *obj* is a numpy array. |
There was a problem hiding this comment.
I find this confusing. How does it handle that case? It returns False, so that np.array('abc') is not equal to 'abc'. Where is this needed? And why is it only the numpy array case that needs to be handled? I can imagine there might be other places in the code where this might be useful because 'obj' might be None, or a number, or...
There was a problem hiding this comment.
if obj is a number or None or etc., then, obj == s will return False just fine. The problem with ndarrays is that np.array(...) == s will return an array, which cannot be used in a boolean switch context.
The fact that np.array("abc") will compare unequal to "abc" did not change with this PR, we were previously already using isinstance().
Perhaps the docstring can be clarified to
Return whether obj is a string equal to string s.
This helper solely exists to handle the case where obj is a numpy array, because in such cases, a naive
obj == swould yield an array, which cannot be used in a boolean context.
How does that sound? Feel free to propose further edits.
There was a problem hiding this comment.
edited accordingly (both instances)
_get_rgba_face is an internal function that is only use to set the graphicscontext's foreground color (in Line2D.draw); it is always effectively called as `gc.set_foreground(line._get_rgba_face(...), isRGBA=True)`. So it makes sense to have it actually always return a RGBA quadruplet, including when the color is "none" (in which case `mcolors.to_rgba` returns (0, 0, 0, 0) regardless of alpha, which works just fine). This removes the need for third party graphicscontexts to handle non-RGBA-quadruplets (specifically, None) even when set_foreground is actually called with isRGBA=True.
47e25e9 to
ee13342
Compare
_get_rgba_face is an internal function that is only use to set the
graphicscontext's foreground color (in Line2D.draw); it is always
effectively called as
gc.set_foreground(line._get_rgba_face(...), isRGBA=True). So it makes sense to have it actually always return aRGBA quadruplet, including when the color is "none" (in which case
mcolors.to_rgbareturns (0, 0, 0, 0) regardless of alpha, which worksjust fine).
This removes the need for third party graphicscontexts to handle
non-RGBA-quadruplets (specifically, None) even when set_foreground is
actually called with isRGBA=True.
PR Summary
PR Checklist