Re-write stacked step histogram

[neggert]

This is a second try at #1706. I think it resolves all of the issues with that PR.

This is re-write of how stacked histograms are handled. It should fix #1679 and #1631, which both came about because of the way that stacked histograms were handled, starting in #847.

I've also added tests that should catch both bugs if they pop up again.

[piti118]

+1 for this.

[mdboom]

Thanks for working on this -- it's a hairy bit of code. I am seeing a change in behavior vs. the 1.1.x baseline mentioned in #1679 -- not sure whether it's a correct change or a regression... it seems some bars that were once very close to zero in height are larger now.

v1.1.x:

This branch:

It might be a good idea to add this as another image test.

Also, could you rebase on master to make sure it's mergeable again and being tested by Travis?

[neggert]

Finally have time to work on this again.

I'm a little bit confused about your request to rebase on master. Do you want this PR to target master instead of v1.2.x? If I rebase onto master and still target v1.2.x, won't this PR unintentionally try to pull in all of the changes in the master branch?

[efiring]

@neggert, I'm not positive, but I suspect @mdboom was not intending that you change the target. In case he is unable to answer right away, I suggest that you leave the PR targetting 1.2.x, and rebase on that; I think the main point is to get it into testable and mergeable condition again. Assuming it really is a bugfix, and is not changing behavior that user code is depending on, then 1.2.x is the right target. (And you are correct, it should be rebased only against the intended up-to-date target.)

[tacaswell]

This overlaps with PR #1775 and issue #1763

[neggert]

@tacaswell This will include your fix. I would propose that I include your additional test into this PR, then merge this one. Does that seem reasonable?

[tacaswell]

@neggert That seems fine. While fixing that I started planning to do a bunch of the stuff that is in this PR.

[neggert]

Just pushed a bunch of changes.

• Rebase onto v1.2.x
• Fix the bug @mdboom mentioned above
• Added a test for that bug
• Incorporated @tacaswell 's test and fix from Issue #1763 #1775 (fixes Matplotlib 1.2.0 no longer respects the "bottom" argument for horizontal histograms #1763 and bottom keyword doesn't work for non-stacked histograms #1744)
• Fix 'stepfilled' histogram is not filled properly when setting yscale('log') #1727: stackedfilled hists don't play nice with log scales. This cropped up because one of my above fixes broke a hack that prevented this from happening in bar plots. I removed that hack and now both histtypes handle this the same way: setting nonpos(x/y) = 'clip' when calling set_(x/y)scale. Note that it's possible that this could break something if someone was relying on that hack.

There is one test that fails on this PR and I'd like some advice on what to do about it. hist_steplog fails, but the image difference is very slight.

Expected:

Observed:

Difference:

Should I just update the expected image?

[tacaswell]

I would have preferred if you cherry picked/merged my commits rather than squashing them down into a single commit.

Thinking about this more, the issue with bottom is a major regression, and should be fixed sooner rather than later (possibly separate from this PR which seems to have a whole bunch of things going on).

[neggert]

I'll leave that up to @mdboom et al. If they want to merge your PR now and
study mine a bit more, I'm happy to rebase on top of yours.

On Saturday, February 23, 2013, Thomas A Caswell wrote:

I would have preferred if you cherry picked/merged my commits rather than
squashing them down into a single commit.

Thinking about this more, the issue with bottom is a major regression,
and should be fixed sooner rather than later (possibly separate from this
PR which seems to have a whole bunch of things going on).

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1724#issuecomment-14000637.

Sent from Gmail Mobile

[mdboom]

Thanks for all this. Indeed, I meant to rebase on 1.2.x, as you have done.

I think updating the baseline image is sufficient, as you say.

I plan to test and merge #1763 relatively soon, and then look at this, which I think would also be very nice to have in a 1.2.1. I'll let you know if this needs a rebase -- if it's straightforward I can just do that myself and push from there.

Thanks for all of this work -- it's nice to have all these details get ironed out!

[mdboom]

PEP8: No spaces around = for kwargs.

[mdboom]

I'm getting this for hist_stacked_weights

I'm also seeing the same hist_steplog failure as you, though it seems updating the baseline image is the thing to do there.

Other than the PEP8 fixes, and getting this test to pass, I think this is an important thing to have in v1.2.1, so thanks so much for working on this.

[neggert]

Just rebased, fixed the tests, and did the PEP8 fixes.