Better argspecs for Axes.stem#11271
Conversation
WeatherGod
left a comment
WeatherGod
left a comment
There was a problem hiding this comment.
You might want to hold off on the plot_surface() changes for the moment. There is another PR that makes some extensive changes to it and would conflict with this.
| 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!
Zac-HD
changed the title
|
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? |
| 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.
anntzer
dismissed
their stale review
Misunderstood code logic. Still, a simpler version (per comments) would be nice.
| 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.
🎉 |
6 participants
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.