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

Add log option to Axes.hist2d #1061

Merged
merged 2 commits into from Sep 4, 2012
Merged

Add log option to Axes.hist2d #1061

merged 2 commits into from Sep 4, 2012

Conversation

bgamari
Copy link
Contributor

@bgamari bgamari commented Aug 8, 2012

We use np.log1p since one would usually expect to find bins with 0 counts.

@@ -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)
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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)

@WeatherGod
Copy link
Member

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.

  • Need to update pyplot.py using the boilerplate.py code (this should be done last)
  • Need to add a note to the docstring for which version this kwarg was added (use .. versionadded::)
  • Need to add an entry to doc/users/whats_new.rst for this feature (or, if more appropriate, doc/api/api_changes.rst)

Also desirable (let us know if you need help with either of these):

  • Add a test for this.
  • Add an example usage for the gallery.

@bgamari
Copy link
Contributor Author

bgamari commented Aug 9, 2012

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.

@bgamari
Copy link
Contributor Author

bgamari commented Aug 15, 2012

Is there any way to run boilerplate.py without installing the tree? It's currently giving me AttributeError: type object 'Axes' has no attribute 'hist2d', which suggests it's trying to run against my installed matplotlib-1.1.1.

@WeatherGod
Copy link
Member

No, it.assumes that whatever install of mpl is the one you are using to
rebuild pyplot.py.

@piti118
Copy link
Contributor

piti118 commented Aug 15, 2012

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?

@WeatherGod
Copy link
Member

Ooh, good point, I hadn't thought of that. It would show, by default the
log of the values. Presumedly, one would set a log norm object in case the
user does not provide one for the scalar mappable. This would override the
default of Normalize for the ScalarMappable.

@bgamari
Copy link
Contributor Author

bgamari commented Aug 21, 2012

I believe I touched on most of the changes mentioned above in this updated series. Things have changed quite a bit,

  • Instead of taking the log myself, I leave this for pcolorfast by passing it a LogNorm as the norm kwarg (meaning colorbar does the right thing, showing ticks labelled as 10^n in the logarithmic case)
  • Instead of taking log(x+1) I mask out the bins with zero counts as None (or NaN, as this is apparently represented as)
  • I added a default rc value for the hist2d.log option
  • I added a brief snippet to whats_new.rst
  • I ran boilerplate.py

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.
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Yes, of course.

@bgamari
Copy link
Contributor Author

bgamari commented Aug 22, 2012

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 hist2d_demo.py, perhaps by adding a second subplot? Unfortunately, it seems that hist2d_demo.py only uses pylab which apparently doesn't expose LogNorm in any way. Would it be frowned upon to import LogNorm directly from matplotlib.colors?

@bgamari
Copy link
Contributor Author

bgamari commented Aug 26, 2012

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
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@bgamari
Copy link
Contributor Author

bgamari commented Aug 28, 2012

Any thoughts concerning adding a logarithmic histogram to hist2d_demo.py?

@WeatherGod
Copy link
Member

That would be a great idea.

@WeatherGod
Copy link
Member

Note, that I think it would be best to have it as a separate example

@bgamari
Copy link
Contributor Author

bgamari commented Aug 28, 2012

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?

@bgamari
Copy link
Contributor Author

bgamari commented Aug 28, 2012

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
Copy link
Member

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

@bgamari
Copy link
Contributor Author

bgamari commented Aug 30, 2012

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.
Copy link
Member

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

@bgamari
Copy link
Contributor Author

bgamari commented Aug 30, 2012

Good catch. Fixed.

@travisbot
Copy link

This pull request fails (merged d33877c into cf7618c).

@bgamari
Copy link
Contributor Author

bgamari commented Aug 30, 2012

Not entirely sure what @travisbot is upset about but I don't see anything that could be from my (quite trivial) work.

@WeatherGod
Copy link
Member

Don't fret. We are still working on configuring our automated testing
setup over at Travis. It has been complaining about everyone's commits the
past couple of days.

@WeatherGod
Copy link
Member

Good work @bgamari, I will merge.

WeatherGod added a commit that referenced this pull request Sep 4, 2012
Add log option to Axes.hist2d
@WeatherGod WeatherGod merged commit f3ccff3 into matplotlib:master Sep 4, 2012
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