Re-write stacked step histogram #1724

Merged
merged 5 commits into from Mar 1, 2013

Conversation

Projects
None yet
5 participants
@neggert
Contributor

neggert commented Jan 30, 2013

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

This comment has been minimized.

Show comment Hide comment
@piti118

piti118 Jan 31, 2013

Contributor

+1 for this.

Contributor

piti118 commented Jan 31, 2013

+1 for this.

@mdboom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Feb 4, 2013

Owner

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:

barstacked-1 1

This branch:

barstacked_neggert

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?

Owner

mdboom commented Feb 4, 2013

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:

barstacked-1 1

This branch:

barstacked_neggert

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

This comment has been minimized.

Show comment Hide comment
@neggert

neggert Feb 23, 2013

Contributor

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?

Contributor

neggert commented Feb 23, 2013

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

This comment has been minimized.

Show comment Hide comment
@efiring

efiring Feb 23, 2013

Owner

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

Owner

efiring commented Feb 23, 2013

@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 comment has been minimized.

Show comment Hide comment
@tacaswell

tacaswell Feb 23, 2013

Owner

This overlaps with PR #1775 and issue #1763

Owner

tacaswell commented Feb 23, 2013

This overlaps with PR #1775 and issue #1763

@neggert

This comment has been minimized.

Show comment Hide comment
@neggert

neggert Feb 23, 2013

Contributor

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

Contributor

neggert commented Feb 23, 2013

@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

This comment has been minimized.

Show comment Hide comment
@tacaswell

tacaswell Feb 23, 2013

Owner

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

Owner

tacaswell commented Feb 23, 2013

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

@neggert

This comment has been minimized.

Show comment Hide comment
@neggert

neggert Feb 23, 2013

Contributor

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 #1775 (fixes #1763 and #1744)
  • Fix #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:
hist_steplog-expected

Observed:
hist_steplog

Difference:
hist_steplog-failed-diff

Should I just update the expected image?

Contributor

neggert commented Feb 23, 2013

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 #1775 (fixes #1763 and #1744)
  • Fix #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:
hist_steplog-expected

Observed:
hist_steplog

Difference:
hist_steplog-failed-diff

Should I just update the expected image?

@tacaswell

This comment has been minimized.

Show comment Hide comment
@tacaswell

tacaswell Feb 23, 2013

Owner

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

Owner

tacaswell commented Feb 23, 2013

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

This comment has been minimized.

Show comment Hide comment
@neggert

neggert Feb 24, 2013

Contributor

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/matplotlib/matplotlib/pull/1724#issuecomment-14000637.

Sent from Gmail Mobile

Contributor

neggert commented Feb 24, 2013

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/matplotlib/matplotlib/pull/1724#issuecomment-14000637.

Sent from Gmail Mobile

@mdboom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Feb 25, 2013

Owner

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!

Owner

mdboom commented Feb 25, 2013

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

View changes

lib/matplotlib/axes.py
@@ -4792,11 +4792,10 @@ def make_iterable(x):
if orientation == 'vertical':
self._process_unit_info(xdata=left, ydata=height, kwargs=kwargs)
if log:
- self.set_yscale('log')
+ self.set_yscale('log', nonposy = 'clip')

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Feb 25, 2013

Owner

PEP8: No spaces around = for kwargs.

@mdboom

mdboom Feb 25, 2013

Owner

PEP8: No spaces around = for kwargs.

@mdboom

View changes

lib/matplotlib/axes.py
@@ -8120,7 +8116,7 @@ def hist(self, x, bins=10, range=None, normed=False, weights=None,
if normed:
db = np.diff(bins)
m = (m.astype(float) / db) / m.sum()
- if stacked:
+ if stacked :

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Feb 25, 2013

Owner

PEP8: No spaces before colon.

@mdboom

mdboom Feb 25, 2013

Owner

PEP8: No spaces before colon.

@mdboom

View changes

lib/matplotlib/axes.py
+ if bottom is None:
+ bottom = np.zeros(len(m), np.float)
+ if stacked:
+ height = m-bottom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Feb 25, 2013

Owner

PEP8: spaces around operator

@mdboom

mdboom Feb 25, 2013

Owner

PEP8: spaces around operator

@mdboom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Feb 25, 2013

Owner

I'm getting this for hist_stacked_weights

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.

Owner

mdboom commented Feb 25, 2013

I'm getting this for hist_stacked_weights

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

This comment has been minimized.

Show comment Hide comment
@neggert

neggert Feb 28, 2013

Contributor

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

Contributor

neggert commented Feb 28, 2013

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

mdboom added a commit that referenced this pull request Mar 1, 2013

@mdboom mdboom merged commit c7da07d into matplotlib:v1.2.x Mar 1, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment