Add stacked kwarg to hist and implement stacked hists for step histtype #847

Merged
merged 6 commits into from Sep 10, 2012

Projects

None yet

8 participants

Contributor
neggert commented Apr 21, 2012

This PR addresses matplotlib/matplotlib#831. The main goal was to implement stacked histograms when using the step or stepfilled histtypes. Rather than add new histtypes as with the barstacked histtype, a new stacked kwarg was added. This kwarg behaves as expected for all four histtypes.

I've added an example showing what happens when histtype="stepfilled", stacked=True. Here is the output of the new example:

stacked histogram example

Owner
mdboom commented Jun 1, 2012

Sorry this PR has languished for so long. @WeatherGod: Do you have any comments on this approach in relation to the more general plans for "stacked" plots.

And I will make my usual comment that it's great to see an example -- we should also have a unit test for this.

Member

I will likely be at the 2012 SciPy conference. I am sure that we can discuss issues such as this and develop some sort of over-arching design plan.

Contributor
neggert commented Jul 22, 2012

Sorry for the delay, I was taking my PhD candidacy exam, so I didn't have time to work on this.

I added a test for the new functionality. Incidentally, I don't think there are any other tests for for axes.hist. Maybe in the future, I'll write some.

Did any of the higher-ups get a chance to discuss the fate of stacked plots at Scipy2012?

Member

I totally understand the need to focus on the candidacy exam. At SciPy2012, many things got discussed, but not that, unfortunately. Maybe a discussion should be started on the mailing list?

Member

And it looks like you need to do a rebase. Make sure that your test images are based on the rebased version as there may have been changes to small details such as text rendering or line anti-aliasing.

Contributor
neggert commented Aug 13, 2012

I've rebased and squashed it down to 1 commit. I also sent around a message to matplotlib-devel trying to start a conversation about the general idea of stacked-ness. Please reply there if you interested in the more general issues of how to approach this problem.

Contributor
neggert commented Aug 19, 2012

So I haven't gotten any replies to the message I sent to matplotlib-devel. Any thoughts on how to proceed?

@efiring efiring and 1 other commented on an outdated diff Aug 20, 2012
lib/matplotlib/axes.py
for i in xrange(nx):
# this will automatically overwrite bins,
# so that each histogram uses the same bins
m, bins = np.histogram(x[i], bins, weights=w[i], **hist_kwargs)
+ if mlast == None :
efiring
efiring Aug 20, 2012 Owner

Tiny style point: please delete space between condition and colon, here and below.

pelson
pelson Aug 20, 2012 Member

Tiny style point #3: it is preferable to use obj is None (PEP8: "Comparisons to singletons like None should always be done with is or is not, never the equality operators.").

Owner
efiring commented Aug 20, 2012

I'm sorry you have been having a hard time getting responses.
I haven't looked very closely, and in particular I have not tried out this changeset on my machine, but offhand it looks sensible. Unfortunately, I have no idea what ideas have been floating around regarding general "stackedness", but it is hard to imagine what would conflict with your changeset. And also unfortunately, yet another rebase will be needed. Whenever it is decided that this will go in, some additions to the CHANGELOG and other documentation will be in order.

Contributor
neggert commented Aug 24, 2012

Just cleaned up a few style issues and rebased.

@efiring efiring and 1 other commented on an outdated diff Aug 24, 2012
lib/matplotlib/axes.py
@@ -7919,6 +7927,8 @@ def hist(self, x, bins=10, range=None, normed=False, weights=None,
# multiple hist with data of different length
x = [np.asarray(xi) for xi in x]
+ x = x[::-1] # reverse datasets for caculating stacked hist
efiring
efiring Aug 24, 2012 Owner

Why do you reverse the order? This seems like an unnecessary change from the previous behavior.

neggert
neggert Aug 24, 2012 Contributor

In the loop starting on line 8002, I want to iterate over the datasets in reverse order so that the first input goes on top. Note that I put them back in the correct order at line 8027.

Now that you point this out, it might be better to reverse the loop than the vector. This would result in the first dataset being plotted at the bottom, which might be better.

neggert
neggert Aug 24, 2012 Contributor

I've put up the alternative. This is probably the minimally invasive way to do it.

P.S. I was mistaken about the first dataset going at the bottom. That would require more extensive changes.

ndawe commented Sep 3, 2012

Will this be merged in soon? Looking forward to this feature!

For now I am using many complicated fill_betweens... For me, this is one of the few remaining issues with matplotlib that prevent me from fully abandoning ROOT for plotting. I've been developing an interface between ROOT and matplotlib in rootpy:

https://github.com/rootpy/rootpy/blob/master/rootpy/plotting/root2matplotlib.py

but ROOT plots stacked histograms like this "stacked stepfilled" feature. In matplotlib, stacked bar plots can have very thin white lines between bars in vector-based formats, which I find unappealing.

Member

@ndawe If you don't mind installing from source, it is possible to use this changeset without it having been merged into the main codebase. Add a new git remote to point to the repo of @neggert. Then you can checkout his branch and install :)

If you need help with any of this, let me know.

At least now you don't have to wait for it to be merged.

Member

The only thing left to do is to update pyplot.py using the boilerplate.py script in the mpl root directory and to add a note to docs/users/whats_new.rst.

Note, I am not sure if the feature freeze has happened yet or not, so it is possible this may get deferred to the next release.

Owner
mdboom commented Sep 4, 2012

I think we can still get this in to 1.2 by the end of the week, if @WeatherGod's tiny fixes are addressed.

Contributor
neggert commented Sep 5, 2012

Done. The changes made by boilerplate.py included a bunch of whitespace changes. I left them out of the commit, but I can put them back if there was some good reason for them.

This pull request fails (merged 014fe4c1 into 94c53e1).

This pull request fails (merged 9f6c10da into 94c53e1).

Contributor
neggert commented Sep 5, 2012

Rebased as well.

This pull request fails (merged 77c5978 into 0f9f85f).

@dmcdougall dmcdougall commented on an outdated diff Sep 5, 2012
doc/users/whats_new.rst
@@ -127,6 +127,16 @@ local intensity of the vector field.
.. plot:: mpl_examples/pylab_examples/streamplot_demo.py
+
+New hist functionality
+----------------------
+
+Nic Eggert added a new `stack kwarg to :meth:`~matplotlib.pyplot.hist` that
dmcdougall
dmcdougall Sep 5, 2012 Member

Should stack have a closing backtick?

dmcdougall
dmcdougall Sep 5, 2012 Member

Also, isn't the kwarg called stacked, not stack?

This pull request fails (merged 242ecac into 0f9f85f).

@WeatherGod WeatherGod commented on the diff Sep 5, 2012
lib/matplotlib/axes.py
# this will automatically overwrite bins,
# so that each histogram uses the same bins
m, bins = np.histogram(x[i], bins, weights=w[i], **hist_kwargs)
+ if mlast is None:
+ mlast = np.zeros(len(bins)-1, np.int)
WeatherGod
WeatherGod Sep 5, 2012 Member

Just a thought... are we guaranteed that bins will have a non-zero length? If not, then it would be a good idea to catch the length-zero case and raise a descriptive exception.

neggert
neggert Sep 5, 2012 Contributor

The bins argument to np.histogram takes either the number of bins or an array of bin edges.

I just played around a little, and I don't think it's possible for np.histogram to return a zero-length array of bin edges. It raises an exception if one tries to set bins=0

In fact, the only way I can get it to return anything less than a length-2 array is by explicitly passing only one bin edge. This will obviously result in some strange behavior, but that's a pretty edge case. If np.histogram doesn't raise an exception for that, I don't think we should, either.

Contributor
neggert commented Sep 5, 2012

Anyone have any idea why the bot keeps telling me the PR fails? I've rebased onto the latest version of master.

Member

Don't worry too much about the Travisbot right now, we are still working out the kinks. As for np.histogram(), I have a vague recollection of it being possible for it to return empty arrays in bad situations, which was later fixed. But it was an extreme edge case that I am not sure is possible to trigger here anyway.

Member

However, the logs do speak of stacked plots failing (not yours, but some of the originals). Maybe worth investigating further.

Owner
mdboom commented Sep 10, 2012

I'm not seeing any additional test failures with this branch on my local machine. Are others (ignoring Travis, since it seems to throw a lot of false positive failures). What's left to be done on this? Anything?

Contributor
neggert commented Sep 10, 2012

I think mine looks fine. Here's the test log if someone more knowledgeable wants to look at it: https://gist.github.com/3691954. If those look good, IMO, this is good to go.

@mdboom mdboom merged commit 634f02d into matplotlib:master Sep 10, 2012

1 check failed

default The Travis build failed
Details
Owner
mdboom commented Jan 25, 2013

This may have introduced #1679: @neggert: Do you have any thoughts?

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