Handle single color in ContourSet #28786
Conversation
|
Based on discussion in #28763, we may need to change this again to always pass a sequence of colours of length |
lib/matplotlib/contour.py
Outdated
| @@ -816,6 +816,11 @@ def __init__(self, ax, *args, | |||
| self._extend_min = self.extend in ['min', 'both'] | |||
| self._extend_max = self.extend in ['max', 'both'] | |||
| if self.colors is not None: | |||
| if isinstance(self.colors, str): | |||
There was a problem hiding this comment.
| if isinstance(self.colors, str): | |
| mcolors.is_color_like(self.colors): |
I'm wondering if this would allow us to drop the documented restriction that a single colour has to be specified as a string. I'm not sure why that restriction was there: with v3.9.1 or v3.7.3 passing an RGB or RBGA tuple seems to work fine (as long as I don't pass extend). It does currently break when passing a (color, alpha) pair, but I think that form is much newer than this restriction.
Edit: it helps to test with a variety of level numbers 🤦♀️ but so far I haven't found a combination that would break with this change.
There was a problem hiding this comment.
This looks right. I'm also 95% confident there aren't any weird edge cases - I think they would require an expression that can be used both as single color and list of colors.
58b63cc to
2b2f195
Compare
lib/matplotlib/contour.py
Outdated
| if isinstance(self.colors, str): | ||
| colors = [colors] | ||
| else: | ||
| colors = colors |
There was a problem hiding this comment.
Why don't you just normalize self.colors? Having both self.colors and colors with potentially different content is confusing and may lead to bugs later. Also, the else clause does nothing?!?
| if isinstance(self.colors, str): | |
| colors = [colors] | |
| else: | |
| colors = colors | |
| if isinstance(self.colors, str): | |
| self.colors = [self.colors] |
There was a problem hiding this comment.
I left self.colors as is because it's public so a user might access it expecting to find whatever they passed in.
Also, the else clause does nothing?!?
Ooops. I wrote these as = [self.colors] and = self.colors and must have been too thorough with the find/replace. I also forgot that I'm still in __init__ so have colors lying around.
There was a problem hiding this comment.
I left
self.colorsas is because it's public so a user might access it expecting to find whatever they passed in.
Ok. AFAIK we don't have a clear policy on that. Personally, I think that we can/should do input normalization. I consider a single color a convenience input API, but not a valid core state of the Artist. But that can be discussed separately. Your PR keeps the current behavior unchanged. 👍
There was a problem hiding this comment.
Off topic, but I don't think we should be worried about keeping track of the exact form of the users arguments for them. The caveat would be if self.colors turned into something that was not easily queried, which I don't think is the case.
There was a problem hiding this comment.
Arguably this shouldn't be a public attribute because changing it after instantiation has no effect.
There was a problem hiding this comment.
You are right. AFAICS we never use it outside of __init__. Therefore, we can simply deprecate it.
2b2f195 to
77b21f8
Compare
77b21f8 to
5ff1f17
Compare
| @@ -702,6 +702,11 @@ def __init__(self, ax, *args, | |||
| self._extend_min = self.extend in ['min', 'both'] | |||
| self._extend_max = self.extend in ['max', 'both'] | |||
| if self.colors is not None: | |||
| if mcolors.is_color_like(self.colors): | |||
| color_sequence = [self.colors] | |||
There was a problem hiding this comment.
👍 for using a new name and not modifying colors. This makes it clear it's a local name and prevents users from thinking its the argument colors, which also gets stored as self.colors.
lib/matplotlib/tests/test_contour.py
Outdated
| @@ -171,6 +171,18 @@ def test_given_colors_levels_and_extends(): | |||
| plt.colorbar(c, ax=ax) | |||
|
|
|||
|
|
|||
| @pytest.mark.parametrize('color', ['green', ('r', 0.5), (0.1, 0.2, 0.5, 0.3)]) | |||
| @pytest.mark.parametrize('extend', ['neither', 'both']) | |||
| @pytest.mark.parametrize('levels', [[0.5, 1, 1.5], [0.5, 0.75, 1, 1.25, 1.5]]) | |||
There was a problem hiding this comment.
Why do we need levels parametrization? I would not expect that to have an impact now that we accept all color specs. Due to the generalization to arbitrary color specs, every "reasonable" implementation must first detect single colors and normalize to a list of colors. This should exclude the previous special behavior when color strings match in length with levels. Note also that you test three sequence-like single colors, so you already can check both cases, these having same or different lengths with levels with one fixed set of levels.
Generally, I'd try to avoid multiple parametrizations when they are basically independent, because the number of tests in the product space increases quickly. Here you create 12 tests, when in fact you only 4-5
- Three colors, to test different color specs (extent=neigher)
- one additional test for the extent=both
- optionally one additional test for another levels setting
There was a problem hiding this comment.
Fair point. How's this? The point about "green" is that it is two characters longer than the number of levels and ends with a character that is not itself a colour. So I picked "darkred" instead. More levels were needed to produce the failure with the RBGA case.
5ff1f17 to
8920c62
Compare
8920c62 to
5db8071
Compare
PR summary
Within
ContourSet.__init__, explicitly check whether colors has been passed as a color type and if so make it a single element list. This removes the restriction that a single color can only be passed as a string, which was somewhat inconsistent with other places that take a single color.Fixes #28782 since
'green'becomes['green']so we no longer slice the string.PR checklist