-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add log option to Axes.hist2d #1061
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
Conversation
@@ -8010,6 +8013,7 @@ def hist2d(self, x, y, bins = 10, range=None, normed=False, weights=None, | |||
|
|||
if cmin is not None: h[h<cmin]=None | |||
if cmax is not None: h[h>cmax]=None | |||
if log: h = h.log1p(h) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you mean 'np.log1p(h)' here? Plus, let's have this kwarg better handled. First, let us stick with the convention of setting the default to None. This will allow for future revisions to treat that to mean "the default behavior", which usually means to grab from the rc-params. Second, let us take the opportunity now to choose either True/False or string values as valid inputs to the kwarg -- not both. This has caused so much grief elsewhere. If you can imagine that other log scales (or even more generally, other z scales) would be desired, then let's go with strings. Otherwise, stick with booleans.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bah, yes, you are right. This is why one shouldn't write code in Github's editor.
In fact, the patch isn't even consistent in that it documents three values for the log kwarg, yet only implements only one. I was originally thinking of allowing the user to choose to use either np.log
or np.log1p
. Given we are talking about histograms here, it seems that log1p
or similar is the only reasonable approach as we expect bins with zero counts. Consequently, I'd probably assume just make the option boolean and always use log1p
(this would need to be made clear in the documentation, of course). Anyone have any better ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hist2d
takes advantage of the fact that when an element in array is set to None
the bin representing the value is white out which looks nice. From my little test, log1p dies on None
. You may want to take care of None
properly. Try this:
x = randn(100)
y = randn(100)
hist2d(x,y,cmin=1)
Interesting feature, and I definitely support the idea because the user has no other way to scale the histogram before display. More work will be needed on this PR, however, to get it up to snuff.
Also desirable (let us know if you need help with either of these):
|
Sure, I completely understand that more work is necessary. I largely opened to the request to see if people thought this would be a reasonable idea. I'll try to put some time in tonight to bring this into a mergeable form. |
Is there any way to run |
No, it.assumes that whatever install of mpl is the one you are using to |
If log is done this way, will the colorbar label come out right?. I mean, will the label show the value or log of value? |
Ooh, good point, I hadn't thought of that. It would show, by default the |
I believe I touched on most of the changes mentioned above in this updated series. Things have changed quite a bit,
How does it look to others? |
@@ -8077,6 +8077,10 @@ def hist2d(self, x, y, bins = 10, range=None, normed=False, weights=None, | |||
All bins that has count more than cmax will not be displayed (set to none before passing to imshow) | |||
and these count values in the return value count histogram will also be set to nan upon return | |||
|
|||
*log* : [None|True|False] | |||
Plot log(counts+1). The default values is False. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this need to be fixed with the updated code? Since it is no longer counts+1, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. Yes, of course.
Here is a quick patch adding a note pointing out how to use a logarithmic color scale. It might also be nice to demonstrate how to do this in |
Any opinion on the above? It might be nice to see a patch along these lines go in to 1.2. |
Remaining keyword arguments are passed directly to :meth:pcolorfast. | ||
|
||
Rendering the histogram with a logarithmic color scale is | ||
accomplished by passing a .. class::colors.LogNorm instance to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do
:class:`colors.LogNorm`
instead. Note the backticks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Any thoughts concerning adding a logarithmic histogram to |
That would be a great idea. |
Note, that I think it would be best to have it as a separate example |
Alright, that is fine. I'll submit a patch with a new example shortly but do you suppose 3f2cdaf could be merged in the meantime? |
How does this look as an example? |
Remaining keyword arguments are passed directly to :meth:pcolorfast. | ||
|
||
Rendering the histogram with a logarithmic color scale is | ||
accomplished by passing a :class::`colors.LogNorm` instance to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
single colon, not double colon
I think this is ready for merging. Any other requests? |
@@ -8077,7 +8077,11 @@ def hist2d(self, x, y, bins = 10, range=None, normed=False, weights=None, | |||
All bins that has count more than cmax will not be displayed (set to none before passing to imshow) | |||
and these count values in the return value count histogram will also be set to nan upon return | |||
|
|||
Remaining keyword arguments are passed directly to :meth:pcolorfast | |||
Remaining keyword arguments are passed directly to :meth:pcolorfast. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I didn't notice this before, but we need backticks around "pcolorfast".
Good catch. Fixed. |
Not entirely sure what @travisbot is upset about but I don't see anything that could be from my (quite trivial) work. |
Don't fret. We are still working on configuring our automated testing |
Good work @bgamari, I will merge. |
Add log option to Axes.hist2d
We use np.log1p since one would usually expect to find bins with 0 counts.