Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Make Axes.stem take at least one argument. #1139

Merged
merged 5 commits into from Jan 4, 2013

Conversation

Projects
None yet
5 participants
Member

dmcdougall commented Aug 22, 2012

When it takes only one, the abscissae default to np.arange(len(y))

This is my attempt at addressing #331.

@pelson pelson and 1 other commented on an outdated diff Aug 27, 2012

lib/matplotlib/axes.py
"""
Create a stem plot.
- Call signature::
+ Valid call signatures::
@pelson

pelson Aug 27, 2012

Member

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".

@WeatherGod

WeatherGod Aug 27, 2012

Member

Probably the closest thing would be the docstring for plot.

Member

dmcdougall commented Aug 28, 2012

The method pcolor has two call signatures, so I just copied that. Thanks for pointing out the inconsistency.

Owner

mdboom commented Aug 28, 2012

This is a nice fix.

Owner

efiring commented Sep 2, 2012

@mdboom, is this waiting for master to switch to 1.3?

Member

pelson commented Sep 2, 2012

Does it need a changelog entry?

Owner

efiring commented Sep 2, 2012

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.

Member

dmcdougall commented Sep 2, 2012

Ok, no problem. I will wait for further instruction :) As always, thanks for the feedback, you guys are really helpful.

Member

dmcdougall commented Nov 5, 2012

What needs to be done here?

Member

dmcdougall commented Nov 13, 2012

I updated the CHANGELOG. Let me know if there's anything else I can do.

Member

WeatherGod commented Nov 13, 2012

This change can break code. Consider the following:

plt.stem(x, y, 'r--')

This was a perfectly valid call before that will no longer work.

Member

dmcdougall commented Nov 13, 2012

@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.

@WeatherGod WeatherGod and 1 other commented on an outdated diff Nov 16, 2012

lib/matplotlib/axes.py
@@ -5104,6 +5106,25 @@ def stem(self, x, y, linefmt='b-', markerfmt='bo', basefmt='r-',
if not self._hold: self.cla()
self.hold(True)
+ # Assume there's at least one data array
+ y = np.asarray(args[0], dtype=np.float)
+
+ # Try a second one
+ try:
+ second = np.asarray(args[1], dtype=np.float)
+ x, y = y, second
+ except:
@WeatherGod

WeatherGod Nov 16, 2012

Member

At the very least, catch Exception, but I would rather catch IndexError and whatever else might come from numpy (ValueError, maybe?)

@dmcdougall

dmcdougall Nov 16, 2012

Member

Yes, I wanted to catch both so didn't specify -- perhaps catching Exception also catches some magical python ponies that I am unaware of?

@dmcdougall

dmcdougall Nov 16, 2012

Member

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--').

@WeatherGod

WeatherGod Nov 16, 2012

Member

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

@dmcdougall

dmcdougall Nov 17, 2012

Member

That's helpful -- thank you!

@WeatherGod WeatherGod commented on the diff Nov 16, 2012

lib/matplotlib/tests/test_axes.py
@@ -901,6 +901,20 @@ def test_hist_stacked_weighted():
ax = fig.add_subplot(111)
ax.hist( (d1, d2), weights=(w1,w2), histtype="stepfilled", stacked=True)
+@cleanup
+def test_stem_args():
+ fig = plt.figure()
+ ax = fig.add_subplot(1, 1, 1)
+
+ x = range(10)
+ y = range(10)
+
+ # Test the call signatures
+ ax.stem(y)
+ ax.stem(x, y)
+ ax.stem(x, y, 'r--')
@WeatherGod

WeatherGod Nov 16, 2012

Member

This call (and the next) "works", but never actually gets the linefmt. It is left behind in *args. plot() and scatter() has to do this sort of crap too. Essentially, you go through all of the remaining positional args, supplying them for the defaults to the pops of the kwargs.

@dmcdougall dmcdougall commented on the diff Nov 17, 2012

lib/matplotlib/axes.py
+ second = np.arange(len(y))
+ x = second
+
+ # Popping some defaults
+ try:
+ linefmt = kwargs.pop('linefmt', args[0])
+ except IndexError:
+ linefmt = kwargs.pop('linefmt', 'b-')
+ try:
+ markerfmt = kwargs.pop('markerfmt', args[1])
+ except IndexError:
+ markerfmt = kwargs.pop('markerfmt', 'bo')
+ try:
+ basefmt = kwargs.pop('basefmt', args[2])
+ except IndexError:
+ basefmt = kwargs.pop('basefmt', 'r-')
@dmcdougall

dmcdougall Nov 17, 2012

Member

I don't like this big try/except block. Is there a better way of doing this?

@WeatherGod

WeatherGod Nov 19, 2012

Member

Just a thought... just how different are the call signatures for stem() and
plot()? Perhaps we can utilize the same mechanism for processing arguments?

Member

dmcdougall commented Dec 15, 2012

Finally got some time to look at this. I did some method chasing and made the following three observations:

  1. Axes.plot(self, *args, **kwargs) calls self._get_lines(*args, **kwargs)
  2. self._get_lines = _process_plot_var_args(self)
  3. _process_plot_var_args is a class to do a bunch of heavy lifting for the args and kwargs passed to plot

There's quite a bit of heavy lifting done inside the _process_plot_var_args class. I guess I'm just not really sure how to use it effectively. I guess the end goal would be to have Axes.stem just make a call to self._get_lines and proceed with the returned line's properties.

I'll play and see what I can come up with over the weekend.

Member

dmcdougall commented Dec 16, 2012

Ok, I've been playing a little bit with this. Basically, the way stem works at the moment is to parse a bunch of stuff and pass the baseline, markers, and stemlines all to plot. Then the three resulting line collections are returned as part of a StemContainer for easy post-processing.

Now, I think what @WeatherGod is suggesting is to basically pass everything to plot in a single call. While that is totally possible due to plot's ninja argument handling skills, I'd still have to parse the list of lines returned from plot and put them into a StemContainer to preserve the return type of stem (presumably, preserving the return type of stem is desirable since it doesn't break backwards compatibility). That is fine and dandy and is probably a good way to refactor the stem function to make only one call to plot. However, I see no other benefit to doing that over what is currently implemented in this PR. I still have to pop the kwargs to stem and get the defaults set correctly like I have done here, the only difference is, by refactoring to a single call to plot, extra foo needs to be done to process the stuff returned from plot and massage it into StemContainer.

I guess the question is: is it worth it? The way it is currently, with three separate calls to plot, it's very easy to keep the marker, base, and stems separate and lump them together into a StemContainer. I think I prefer this approach. Others may disagree.

@WeatherGod I'd especially like your opinion on this summary since you were the one to suggest this originally.

Member

dmcdougall commented Dec 16, 2012

Actually, I do have an idea of how to improve this. I'll report back shortly.

Member

dmcdougall commented Dec 16, 2012

Scratch that; my idea didn't work.

Member

WeatherGod commented Dec 17, 2012

yeah, looking again, it is probably not worth it to go that unified plot route.

Member

dmcdougall commented Dec 18, 2012

@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.

Member

dmcdougall commented Jan 4, 2013

Can someone push the green button, please? :)

@mdboom mdboom added a commit that referenced this pull request Jan 4, 2013

@mdboom mdboom Merge pull request #1139 from dmcdougall/stemarg
Make Axes.stem take at least one argument.
3b649cb

@mdboom mdboom merged commit 3b649cb into matplotlib:master Jan 4, 2013

1 check passed

default The Travis build passed
Details

@dmcdougall dmcdougall deleted the dmcdougall:stemarg branch Jan 4, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment