Use standard property alias machinery in contour().#30433
Conversation
anntzer
added
the
API: argument checking
anntzer
force-pushed
the
ck
branch
2 times, most recently
from
a9055a1 to
6b001dc
Compare
efiring
left a comment
efiring
left a comment
There was a problem hiding this comment.
Wouldn't it be more internally consistent to let "color" be an alias for "colors" instead of treating it as an error? edgecolor and facecolor are both accepted.
|
|
||
| def test_contour_singular_color(): | ||
| with pytest.raises(TypeError): | ||
| plt.figure().add_subplot().contour([[0]], color="r") |
There was a problem hiding this comment.
The TypeError is coming from the wrong place, the first argument rather than the second. To fix, use
plt.figure().add_subplot().contour([[0, 1], [2, 3]], color="r").
There was a problem hiding this comment.
Not a blocker, but it’s typically a good idea to use pytest.raises(…, match=…) to make the test more specific. This would likely have caught the issue of catching the wrong exception.
There was a problem hiding this comment.
I think the issue is rather that contour([[0]]) should raise a ValueError rather than a TypeError (the input has the right type, after all).
There was a problem hiding this comment.
I agree that ValueError would be more appropriate. But that doesn’t reduce the value of narrowing down tests via match.
There was a problem hiding this comment.
I'll leave this one as it's for now, changing pytest.raises to more generally test the error message seems like a good idea but also seems a separate issue.
Note that 'color(s)' remains special-cased because the ContourSet constructor needs to do a fair bit of special wrangling for it.
|
I intentionally chose to not support 'color' as alias for 'colors' because 'colors' is actually not a ContourSet property (there's no ContourSet.set_colors, unlike 'edgecolors' and 'facecolors', which are aliases for the respective singular forms). Moreover, whereas ContourSet.set_color (singular) does exist, it doesn't really work -- try e.g. from pylab import *
contourf(np.arange(100).reshape(10, 10), levels=[20, 50, 80]).set_color(["c", "m", "y", "k"])which leaves the spanned region with the default colors and tries to set colors on the boundaries instead. Turning 'color'/'colors' into a real ContourSet property could be a followup issue, though. |
efiring
left a comment
efiring
left a comment
There was a problem hiding this comment.
Unrelated test failures? Otherwise, looks OK to me.
|
Seems unrelated indeed. |
4 participants
Note that 'color(s)' remains special-cased because the ContourSet constructor needs to do a fair bit of special wrangling for it.
Closes #1963.
PR summary
PR checklist