Make Axes.stem take at least one argument. #1139
Conversation
| """ | ||
| Create a stem plot. | ||
|
|
||
| Call signature:: | ||
| Valid call signatures:: |
There was a problem hiding this comment.
This change makes this docstring inconsistent with almost all others I am aware of. My feeling is that "Valid call signatures" should be reverted to "Call signature".
There was a problem hiding this comment.
Probably the closest thing would be the docstring for plot.
|
The method |
|
This is a nice fix. |
|
@mdboom, is this waiting for master to switch to 1.3? |
|
Does it need a changelog entry? |
|
Or better, api_changes.rst, if it is targeted for 1.2. If it missed the freeze and is therefore delayed until 1.3, then it is just as well to wait on the documentation changes, since they will likely need to be rebased anyway if done now. I'm just not clear on what can be merged now, and what should wait. I don't like having this PR backlog. |
|
Ok, no problem. I will wait for further instruction :) As always, thanks for the feedback, you guys are really helpful. |
|
What needs to be done here? |
|
I updated the |
|
This change can break code. Consider the following: This was a perfectly valid call before that will no longer work. |
When it takes only one, the abscissae default to np.arange(len(y))
|
@WeatherGod Thank you for trying this. I believe I have fixed this now. I also added a small test to check the kwargs as well, nothing huge. Just sanity checks. |
| try: | ||
| second = np.asarray(args[1], dtype=np.float) | ||
| x, y = y, second | ||
| except: |
There was a problem hiding this comment.
At the very least, catch Exception, but I would rather catch IndexError and whatever else might come from numpy (ValueError, maybe?)
There was a problem hiding this comment.
Yes, I wanted to catch both so didn't specify -- perhaps catching Exception also catches some magical python ponies that I am unaware of?
There was a problem hiding this comment.
The IndexError comes from args[1] being out of bounds. The ValueError comes from the casting to np.float being invalid. This is the case in your example of stem(x, 'r--').
There was a problem hiding this comment.
If you want to catch both, then do except (IndexError, ValueError):. There is a difference between except: and except Exception:. See here: http://docs.python.org/2/howto/doanddont.html#except
There was a problem hiding this comment.
That's helpful -- thank you!
| try: | ||
| basefmt = kwargs.pop('basefmt', args[2]) | ||
| except IndexError: | ||
| basefmt = kwargs.pop('basefmt', 'r-') |
There was a problem hiding this comment.
I don't like this big try/except block. Is there a better way of doing this?
There was a problem hiding this comment.
Just a thought... just how different are the call signatures for stem() and
plot()? Perhaps we can utilize the same mechanism for processing arguments?
|
Finally got some time to look at this. I did some method chasing and made the following three observations:
There's quite a bit of heavy lifting done inside the I'll play and see what I can come up with over the weekend. |
|
Ok, I've been playing a little bit with this. Basically, the way Now, I think what @WeatherGod is suggesting is to basically pass everything to I guess the question is: is it worth it? The way it is currently, with three separate calls to @WeatherGod I'd especially like your opinion on this summary since you were the one to suggest this originally. |
|
Actually, I do have an idea of how to improve this. I'll report back shortly. |
|
Scratch that; my idea didn't work. |
|
yeah, looking again, it is probably not worth it to go that unified plot route. |
|
@WeatherGod Depends what you want out of the code. I can see benefits to both approaches, however I think this one is probably the most appropriate. This is fine to merge, in my opinion. |
|
Can someone push the green button, please? :) |
Make Axes.stem take at least one argument.
When it takes only one, the abscissae default to
np.arange(len(y))This is my attempt at addressing #331.