You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Hist Plot Normalization should allow a 'Per Bin' Normalization #4038
The hist function as already absurdly complicated. I am not a fan of making it any more complicated that it it (and would actually prefer to remove many of it's bells and whistles or just get rid of it all together, but neither of those are going to happen anytime soon).
I think the accepted answer on SO (just using bar) is the right thing to do as in separates the computation logic from the plotting logic.
Yeah, I believe that separating logic from computation is a great argument, however, by that reasoning hist should not exist as a standalone function.
But hist exists, and it exists because it's a helper function that improves common uses. Other libraries like pandas depend on it. We should understand how that helper function currently fails to help, and either refactor it or improve it.
With respect to it's current implementation, I'd argue that it's counterintuitive to not do per bin normalization. A regular hist plot shows counts per bin, why wouldn't a normalized show normalized counts per bin. Instead it shows a fraction, but it's not clear what unit that corresponds to, unless you read the code. We can fix this.
As for #4037, I don't believe having a normalized_value other than 1 is particularly important, and would consider it a bells and whistle, even if it is a low hanging bell and whistle.
@tacaswell If there is other complicated elements of hist you believe should be removed, I'm open to that.
My issue with hist is that with the same top-level command you can:
plot the histogram of one array
plot the histograms of many arrays as
stacked
next to each other
the histogram can then be be either
vertical
horizontal
and either
bars (filled or not)
line (which to preserve return types is implemented as a Patch object, not a Line2D object!)
with normalization
none
integrate to 1
cumulative or not, plus a bunch of style controls. IIRC there are also some kwargs which only matter depending on the value of other kwargs (and adding a 'normalize_to' kwarg would definitely fall into this catagory) which is a very bad practice. The combinatorics of all this add up to a large number of code-paths (and a some-what convulated code base) which we do not have through testing on. I am not enthusiastic about making this worse by adding more complication.
Be careful about what is 'intuitive', the current norm behavior is to plot the density which matches the behavior of np.histogram and (from my background) is the natural and correct thing to plot. From looking at the docstring this seems to be pretty clearly documented (http://matplotlib.org/api/pyplot_api.html#matplotlib.pyplot.hist). I am not really sure what you mean by 'per-bin'.
Some what related, it looks like numpy has deprecated normed in favor of density in 1.6. That is our current minimum np so we should probably do the same.
The path I think we should go should be is to improve the documentation/examples/API for step/bar/barh to make it really easy for users to find how to take pre-binned/normed/cumsummed/what ever data plot them as histograms. This is tangentially related to #643,
You can also use the weights kwarg to get all of the bins to sum to 1 by passing ax.hist(x, ..., weights=np.oneslike(x) / x.shape[-1]) or something similar.
All of that said, if you want to take a crack at re-factoring hist go for it! Have a look at what we did with the boxplot refactor #2643 in term of the pattern to follow (one function which takes in raw data + processing parameters and one (or in this case maybe more) functions which take in the output from the processing function). One lesson learned from that work was that before we start pulling it apart, we should make sure we have 100% test coverage on hist (as the 1.4.1 release was basically driven by all fixing all of the boxplot code we broke).
Also remember I am not the final word on things in this project (which I feel compelled to remind people only when I disagree with them).
attn @neggert as the last person to do major work on hist (no good deed goes unpunished).
Are there any other views on this from the dev team? If there is generally negative feelings about this, I'd lean towards closing, especially given that there is no additional progress in the last couple years.
Since at this point, after 2 years, neither @tacaswell nor I understand what is meant by 'per-bin' normalization, I will close it. As @tacaswell noted, however, we would be happy to have a PR that refactors hist to make the code more understandable, readable, and testable (and tested), and if that also adds new functionality, so much the better.
Activity
tacaswell commentedon Jan 25, 2015
The
histfunction as already absurdly complicated. I am not a fan of making it any more complicated that it it (and would actually prefer to remove many of it's bells and whistles or just get rid of it all together, but neither of those are going to happen anytime soon).I think the accepted answer on SO (just using
bar) is the right thing to do as in separates the computation logic from the plotting logic.fridiculous commentedon Jan 25, 2015
Yeah, I believe that separating logic from computation is a great argument, however, by that reasoning
histshould not exist as a standalone function.But
histexists, and it exists because it's a helper function that improves common uses. Other libraries likepandasdepend on it. We should understand how that helper function currently fails to help, and either refactor it or improve it.With respect to it's current implementation, I'd argue that it's counterintuitive to not do per bin normalization. A regular hist plot shows counts per bin, why wouldn't a normalized show normalized counts per bin. Instead it shows a fraction, but it's not clear what unit that corresponds to, unless you read the code. We can fix this.
As for #4037, I don't believe having a normalized_value other than 1 is particularly important, and would consider it a bells and whistle, even if it is a low hanging bell and whistle.
@tacaswell If there is other complicated elements of
histyou believe should be removed, I'm open to that.tacaswell commentedon Jan 25, 2015
My issue with hist is that with the same top-level command you can:
the histogram can then be be either
and either
Patchobject, not aLine2Dobject!)with normalization
cumulative or not, plus a bunch of style controls. IIRC there are also some kwargs which only matter depending on the value of other kwargs (and adding a 'normalize_to' kwarg would definitely fall into this catagory) which is a very bad practice. The combinatorics of all this add up to a large number of code-paths (and a some-what convulated code base) which we do not have through testing on. I am not enthusiastic about making this worse by adding more complication.
Be careful about what is 'intuitive', the current norm behavior is to plot the density which matches the behavior of
np.histogramand (from my background) is the natural and correct thing to plot. From looking at the docstring this seems to be pretty clearly documented (http://matplotlib.org/api/pyplot_api.html#matplotlib.pyplot.hist). I am not really sure what you mean by 'per-bin'.Some what related, it looks like numpy has deprecated
normedin favor ofdensityin 1.6. That is our current minimum np so we should probably do the same.The path I think we should go should be is to improve the documentation/examples/API for
step/bar/barhto make it really easy for users to find how to take pre-binned/normed/cumsummed/what ever data plot them as histograms. This is tangentially related to #643,You can also use the
weightskwarg to get all of the bins to sum to 1 by passingax.hist(x, ..., weights=np.oneslike(x) / x.shape[-1])or something similar.All of that said, if you want to take a crack at re-factoring
histgo for it! Have a look at what we did with the boxplot refactor #2643 in term of the pattern to follow (one function which takes in raw data + processing parameters and one (or in this case maybe more) functions which take in the output from the processing function). One lesson learned from that work was that before we start pulling it apart, we should make sure we have 100% test coverage onhist(as the 1.4.1 release was basically driven by all fixing all of theboxplotcode we broke).Also remember I am not the final word on things in this project (which I feel compelled to remind people only when I disagree with them).
attn @neggert as the last person to do major work on
hist(no good deed goes unpunished).petehuang commentedon Jan 6, 2017
Are there any other views on this from the dev team? If there is generally negative feelings about this, I'd lean towards closing, especially given that there is no additional progress in the last couple years.
efiring commentedon Jan 7, 2017
Since at this point, after 2 years, neither @tacaswell nor I understand what is meant by 'per-bin' normalization, I will close it. As @tacaswell noted, however, we would be happy to have a PR that refactors hist to make the code more understandable, readable, and testable (and tested), and if that also adds new functionality, so much the better.