Better argspecs for Axes.stem #11271
Conversation
| @@ -1131,7 +1131,7 @@ def test_detrend_str_linear_2d_slope_off_axis0(self): | |||
| res = mlab.detrend(input, key='linear', axis=1) | |||
| assert_allclose(res, targ, atol=self.atol) | |||
|
|
|||
| def test_detrend_detrend_linear_1d_slope_off_axis1(self): | |||
| def test_detrend_detrend_linear_1d_slope_off_axis1_notranspose(self): | |||
There was a problem hiding this comment.
Was there a particular need to change these test names?
There was a problem hiding this comment.
Yes - there's another test with this name, so before renaming this test could never be run!
|
Thanks for the tip (and early review) @WeatherGod; I've just taken out the |
|
Hi @tacaswell @anntzer - can I get a quick review for this 15-line PR? |
lib/matplotlib/axes/_axes.py
Outdated
| x, y = y, np.asarray(args[0], dtype=float) | ||
| except (IndexError, ValueError): | ||
| # The second array doesn't make sense, or it doesn't exist | ||
| x, *args = args |
There was a problem hiding this comment.
That seems to be a slightly convoluted way to do if len(args) > 1 (yes, I know, the original version is just as bad).
Moreover it is far from obvious to me that we want to drop support for passing just y (in which case x is implicitly defined as range(len(y))). Yes, it's a questionable API, but it's also present in plot() and I don't think it makes sense to remove it in a piecemeal fashion; instead this should be done in a consistent manner.
Of course, there's the additional complication of categorical data (although I think categorical y's in a stem plot don't make much sense either).
TLDR: I don't think the API change is as benign as the PR suggests, and warrants more discussion. Anyone can dismiss this review once a reasonable consensus is reached.
There was a problem hiding this comment.
Agreed, a check if len(args) > 1 is preferable to this try logic.
Support for y-only is not dropped. See the except clause.
Misunderstood code logic. Still, a simpler version (per comments) would be nice.
lib/matplotlib/axes/_axes.py
Outdated
| @@ -2350,17 +2350,19 @@ def stem(self, *args, linefmt=None, markerfmt=None, basefmt=None, | |||
| which inspired this method. | |||
|
|
|||
| """ | |||
| if len(args) > 5: | |||
There was a problem hiding this comment.
You may as well do
if not 1 <= len(args) <= 5:
raise TypeError("stem expected between 1 and 5 positional arguments, got {}".format(len(args))
which lets you get rid of the comment immediately below.
🎉 |
Another entry in my long quest for explicit argspecs for Matplotlib. Now with bonus fix for an unrelated test, because I happened to see it.