Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
FIX: Introduced new keyword 'density' in the hist function#8408
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
@tacaswell I thought we had a PR for this; did it not get merged? |
Sorry, I didn't see that there was a PR already, since it did not reference the issue. |
| @@ -6220,7 +6225,7 @@ def hist(self, x, bins=None, range=None, normed=False, weights=None, | |||
| if cbook.is_numlike(cumulative) and cumulative < 0: | |||
| slc = slice(None, None, -1) | |||
| ifnormed: | |||
| ifdensity: | |||
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.
Looks like this path is never tested. Could you add a test that uses thedensity option?
lib/matplotlib/axes/_axes.py Outdated
| @@ -6132,6 +6135,10 @@ def hist(self, x, bins=None, range=None, normed=False, weights=None, | |||
| if histtype == 'barstacked' and not stacked: | |||
| stacked = True | |||
| if density is not None and normed is not None and normed != density: | |||
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.
Add a test to check this
lib/matplotlib/axes/_axes.py Outdated
| @@ -6132,6 +6135,10 @@ def hist(self, x, bins=None, range=None, normed=False, weights=None, | |||
| if histtype == 'barstacked' and not stacked: | |||
| stacked = True | |||
| if density is not None and normed is not None and normed != density: | |||
| raise ValueError("kwargs density and normed cannot have different\ | |||
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.
Minor style comment. Can you do:
raiseValueError("kwargs density and normed cannot have different ""values. Use density only, since normed will be deprecated.")
Thanks for your work on this! I definitely think it should raise if both kwargs are passed. If the uses passes in both, then they must be passed as identical values (so we do not have to document and people do not have understand the precedence rules). This can lead to very brittle code, will just move the frustration / debugging to a later time (when they two values get out of sync), and does not actually help improve the clarity. @moboehle Are you comfortable enough with git to merge the two pull requests together? |
…rmed cannot be set to not None at the same time anymore.
@phobson Thanks for your feedback, I added some tests now. @tacaswell You are right, allowing to set both 'normed' and 'density' could lead to odd behavior, I updated this now. I would be happy to merge the pull requests together, but I am not quite sure how to go about 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.
This looks good - just need to get rid of the changes to the autogenerated file.
I also can't see any tests in the diff?
| bottom=None, histtype='bar', align='mid', orientation='vertical', | ||
| rwidth=None, log=False, color=None, label=None, stacked=False, | ||
| hold=None, data=None, **kwargs): | ||
| def hist(x, bins=None, range=None, density=None, normed=None, weights=None, |
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.
lib/matplotlib/pyplot.py is an autogenerated file, so these changes will need to be reverted.
#7856 has a test (that needs an image committed). @moboehle I suggest you do the following: You will probably have to clean up some conflicts, but then commit and push everything back to your branch. You will need commit the test image results as well. If you need help jump intohttps://gitter.im/matplotlib/matplotlib |
Ping@moboehle. |
Introduced new keyword 'density' in the hist function to be consistent with numpy.
This pull request closes issue#7364. Contrary to what was suggested in the comments, no error is raised in the case that both keywords density and normed are used. If neither are defined, default is 'False'.