Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
WeatherGod merged 2 commits intomatplotlib:masterfrombgamari:master
Sep 4, 2012
Merged

Conversation

@bgamari
Copy link
Contributor

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

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
ContributorAuthor

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 eithernp.log ornp.log1p. Given we are talking about histograms here, it seems thatlog1p 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 uselog1p (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 toNone the bin representing the value is white out which looks nice. From my little test, log1p dies onNone. You may want to take care ofNone 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
ContributorAuthor

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
ContributorAuthor

Is there any way to runboilerplate.py without installing the tree? It's currently giving meAttributeError: 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

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
ContributorAuthor

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 forpcolorfast by passing it aLogNorm as thenorm kwarg (meaningcolorbar does the right thing, showing ticks labelled as 10^n in the logarithmic case)
  • Instead of takinglog(x+1) I mask out the bins with zero counts asNone (or NaN, as this is apparently represented as)
  • I added a default rc value for thehist2d.log option
  • I added a brief snippet towhats_new.rst
  • I ranboilerplate.py

How does it look to others?

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
ContributorAuthor

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
ContributorAuthor

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 inhist2d_demo.py, perhaps by adding a second subplot? Unfortunately, it seems thathist2d_demo.py only uses pylab which apparently doesn't exposeLogNorm in any way. Would it be frowned upon to importLogNorm directly frommatplotlib.colors?

@bgamari
Copy link
ContributorAuthor

Any opinion on the above? It might be nice to see a patch along these lines go in to 1.2.

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
ContributorAuthor

Choose a reason for hiding this comment

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

Done.

@bgamari
Copy link
ContributorAuthor

Any thoughts concerning adding a logarithmic histogram tohist2d_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
ContributorAuthor

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
ContributorAuthor

How does this look as an example?

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
ContributorAuthor

I think this is ready for merging. Any other requests?

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
ContributorAuthor

Good catch. Fixed.

@travisbot
Copy link

This pull requestfails (mergedd33877c intocf7618c).

@bgamari
Copy link
ContributorAuthor

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 requestSep 4, 2012
@WeatherGodWeatherGod merged commitf3ccff3 intomatplotlib:masterSep 4, 2012
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bgamari@WeatherGod@piti118@travisbot

[8]ページ先頭

©2009-2025 Movatter.jp