Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Adding density hist2d#12563
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
Adding density hist2d#12563
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
Note also that you do not have to open a new PR if you are not satisfied with your commits so far. You can always force-push to modify existing commits. This keeps the tracker a bit cleaner.
lib/matplotlib/axes/_axes.py Outdated
| "simultaneously. Please only use 'density', " | ||
| "since 'normed' is deprecated.") | ||
| if normed is not None: | ||
| cbook.warn_deprecated("2.1", name="'normed'", obj_type="kwarg", |
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.
You cannot deprecate for "2.1". The deprecation has to be always for the next point release, i.e. currently "3.1".removal can be omitted. That's two minor point releases by default.
lib/matplotlib/axes/_axes.py Outdated
| normed : bool, optional, default: False | ||
| Normalize histogram. | ||
| density : boolean, optional | ||
| If False, the default, returns the number of samples in each bin. |
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.
"returns" is a bit misleading. I would use "plots and returns".
@timhoffm Thanks for the comments. I was getting hard time rebasing. Last time I rebased my branch, I got other people commits on my branch and I just rebase this branch now and it looks fine after followingthis. So |
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.
Thanks for the PR!
Please be sure to add tests that test the new kwarg, and that test the Error.
| If False, the default, plots and returns the number of samples | ||
| in each bin. If True, returns the probability *density* | ||
| function at the bin, ``bin_count / sample_count / bin_area``. | ||
| Default is ``None`` for both *normed* and *density*. If either is |
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.
Can we keep the discussion of deprecatednormed kwarg in the description of that kwarg?
| cbook.warn_deprecated("3.1", name="'normed'", obj_type="kwarg", | ||
| alternative="'density") | ||
| normed = bool(density) or bool(normed) | ||
| h, xedges, yedges = np.histogram2d(x, y, bins=bins, range=range, |
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.
I don't think we should call the argumentnormed if we are going to change todensity later.
This could also use a comment in here about how we will need to switch todensity when np1.15 becomes our minimal version. Otherwise if I was looking at this code I'd have no idea why you are deprecating the namenormed when that's what numpy is using. Indeed reviewing this I had to see@anntzer 's comment:#11070 (comment) to understand why you were doing this. Should probably be in the PR description as well to save folks back referencing.
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.
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.
I'm fine with the change proposed here , but only because numpy is moving to usingdensity as well (in np1.15). So the PR should be clear why we are doing this.
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.
So you still havenormed = bool(density) or bool(normed), I think you should be populating and usingdensitynotnormed.
I think there should be a comment in thecode here for future reference to explain this.
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.
I misunderstood about your comment ondensity. Let me fix it right away.
653be4e to14a74acComparelib/matplotlib/axes/_axes.py Outdated
| @@ -6869,8 +6878,20 @@ def hist2d(self, x, y, bins=10, range=None, normed=False, weights=None, | |||
| keyword argument. Likewise, power-law normalization (similar | |||
| in effect to gamma correction) can be accomplished with | |||
| `.colors.PowerNorm`. | |||
| - Numpy 1.15 introduced a 'density' kwarg to ``hist2d``. Even though | |||
| Numpy 1.15 isn't required, 'density' kwarg is introduced | |||
| and passed as 'normed' to ``hist2d``. | |||
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.
@jklymak Do you want me to place this explanation somewhere else? Thanks.
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.
This probably doesn’t hurt I might have put it in the normed kwarg description
@thoo please ping me when this is ready and ask for a re-review! |
@jklymak Can you re-review this PR? |
This was covered by#13128. |
Close#11070