Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bugs in stacked histograms #1706

Closed
wants to merge 2 commits into from
Closed

Conversation

neggert
Copy link
Contributor

@neggert neggert commented Jan 26, 2013

Hi,

This is an attempt to fix both #1679 and #1631, which both came about because of the way that stacked histograms are handled, introduced in #847.

This isn't ready to go in yet, but I'm running into some problems getting it to work correctly, so I thought I'd try to get some more eyes on it.

There are two problems I'm running into:

  1. fill_between isn't actually filling
  2. fill_between seems to be screwing up autosetting the y axis limits.

Here is the image I get when I run test_hist_stacked. The two histograms are now in the proper order, fixing #1679, you can see the two problems I mentioned. The histograms should be filled and the minimum of the y-axis should be 0.

hist_stacked

Could someone a little more familiar with fill_between take a look at this?

p.set_snap(False)
patch.update(kwargs)
if lbl is not None:
patch.set_label(lbl)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the switch to fill_between, we're dealing with a PolyCollection rather than a list of patches. Thus, some changes here. I also put the patches from the bar histtype into a PatchCollection, so they can be treated using the same interface.

@dmcdougall
Copy link
Member

This currently targets master. Should it instead target v1.2.x if its intent is to fix a bug?

@neggert
Copy link
Contributor Author

neggert commented Jan 26, 2013

I suppose you're correct. Let's get it working first, then I'll switch it over.

@WeatherGod
Copy link
Member

If this changes the output type of the call to PolyCollection from a list
of patches, we can't make this go to v1.2.x branch since it would be a
change in API.

@neggert
Copy link
Contributor Author

neggert commented Jan 28, 2013

I think we can keep the API the same by just returning patches.get_paths(). I'm not particularly attached to using the collections, that's just what fill_between returns. If we can't get fill_between to behave, this doesn't matter anyway.

@dmcdougall
Copy link
Member

I think we can keep the API the same by just returning patches.get_paths().

Actually, not only the type of the returned value needs to be the same, but the order in which they appear in the returned iterable needs to be unchanged as well.

@neggert
Copy link
Contributor Author

neggert commented Jan 28, 2013

Let's cross that bridge when we come to it. Right now, this isn't even working properly.

patches.append(self.fill(x, y,
closed=False, facecolor=c))
patches.append(self.fill_between(x, y, y_bottom,
closed=False, facecolors=c))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PEP8 nitpick: this line is under-indented: it should be align with the opening ( of the previous line

@neggert
Copy link
Contributor Author

neggert commented Jan 30, 2013

Closed in favor of #1723.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants