FIX: Introduced new keyword 'density' in the hist function by moboehle · Pull Request #8408 · matplotlib/matplotlib

moboehle · 2017-03-30T14:18:35Z

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

…istent with numpy

QuLogic · 2017-03-30T19:46:10Z

@tacaswell I thought we had a PR for this; did it not get merged?

phobson · 2017-03-30T20:47:48Z

@QuLogic see #7856

moboehle · 2017-03-31T08:01:20Z

Sorry, I didn't see that there was a PR already, since it did not reference the issue.
The solutions to introducing 'density' as a new keyword are almost identical. However, as @tacaswell pointed out, we can pass the keyword density to np.histogram and delete the additional normalization in the hist function, in order to simplify the code. This is the main difference between the two fixes. Furthermore, in the current version of this PR, no error is raised if 'normed' and 'density' are both used but not contradicting. I think as long as there is no ambiguity in the input, no error should be raised, as it would cause an unnecessary inconvenience to the user.

phobson · 2017-03-31T12:01:46Z

lib/matplotlib/axes/_axes.py

                slc = slice(None, None, -1)

-            if normed:
+            if density:


Looks like this path is never tested. Could you add a test that uses the density option?

phobson · 2017-03-31T12:02:27Z

lib/matplotlib/axes/_axes.py

        if histtype == 'barstacked' and not stacked:
            stacked = True

+        if density is not None and normed is not None and normed != density:


Add a test to check this

phobson · 2017-03-31T12:03:22Z

lib/matplotlib/axes/_axes.py

            stacked = True

+        if density is not None and normed is not None and normed != density:
+            raise ValueError("kwargs density and normed cannot have different\


Minor style comment. Can you do:

raise ValueError("kwargs density and normed cannot have different " "values. Use density only, since normed will be deprecated.")

tacaswell · 2017-03-31T13:39:17Z

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.

moboehle · 2017-04-02T13:57:47Z

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

dstansby

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?

dstansby · 2017-04-05T08:15:53Z

lib/matplotlib/pyplot.py

-         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,


lib/matplotlib/pyplot.py is an autogenerated file, so these changes will need to be reverted.

tacaswell · 2017-04-06T02:36:44Z

#7856 has a test (that needs an image committed).

@moboehle I suggest you do the following:

git checkout bccn_hist # just to be safe
# add a remote
git remote add chelseatroy https://github.com/chelseatroy/matplotlib.git
# update your remotes
git remote update
git merge chelseatroy/histogram_compatibility_with_numpy_7364

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 into https://gitter.im/matplotlib/matplotlib

QuLogic · 2017-05-28T22:21:30Z

Ping @moboehle.

dstansby · 2017-08-05T11:45:50Z

Hi @moboehle , thanks for the PR! Since there were two PRs I have merged them together with a few fixes and put them in #8993 - you will still be attributed to the work when it gets merged though.

FIX: Introduced new keyword 'density' in the hist function to be cons…

5a81ed1

…istent with numpy

Reformatted according to pep8

52e9cf8

phobson reviewed Mar 31, 2017

View reviewed changes

phobson added this to the 2.1 (next point release) milestone Mar 31, 2017

moboehle added 2 commits April 2, 2017 15:50

Added tests for density. Style fixes in hist function. Density and no…

781eecd

…rmed cannot be set to not None at the same time anymore.

Density and normed cannot be set to not None at the same time anymore.

eba7169

dstansby requested changes Apr 5, 2017

View reviewed changes

dstansby mentioned this pull request Aug 5, 2017

Add 'density' kwarg to histogram #8993

Merged

dstansby added the status: duplicate label Aug 7, 2017

dstansby closed this Aug 7, 2017

Uh oh!

Conversation

moboehle commented Mar 30, 2017

Uh oh!

QuLogic commented Mar 30, 2017

Uh oh!

phobson commented Mar 30, 2017

Uh oh!

moboehle commented Mar 31, 2017

Uh oh!

phobson Mar 31, 2017

Choose a reason for hiding this comment

Uh oh!

phobson Mar 31, 2017

Choose a reason for hiding this comment

Uh oh!

phobson Mar 31, 2017

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Mar 31, 2017

Uh oh!

moboehle commented Apr 2, 2017

Uh oh!

dstansby left a comment

Choose a reason for hiding this comment

Uh oh!

dstansby Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Apr 6, 2017

Uh oh!

QuLogic commented May 28, 2017

Uh oh!

dstansby commented Aug 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants