Histogram compatibility with numpy 7364 by chelseatroy · Pull Request #7856 · matplotlib/matplotlib

chelseatroy · 2017-01-17T04:13:22Z

The hist function should now work with either 'normed' or 'density' defined.

If neither are defined, default is 'False.'
If both are defined as different things, the function raises an error.

Some tests added, and comment documentation updated, to reflect the changes.

QuLogic · 2017-01-17T04:27:32Z

determinism_tex.svg

@@ -0,0 +1,29 @@
+<?xml version="1.0" encoding="utf-8" standalone="no"?>


Please rebase and remove this file entirely.

Hi @QuLogic!

Thanks for the quick response. I rebased, but my changes are up to date with the current remote. The contents of determinism_tex.svg appear to be tested in test_backend_svg.py (line 204). Would you still like me to remove it?

This was added in your first commit. You'll need to rebase and edit the first commit to remove the file.

QuLogic · 2017-01-17T04:28:37Z

lib/matplotlib/axes/_axes.py


    @_preprocess_data(replace_names=["x", 'weights'], label_namer="x")
-    def hist(self, x, bins=None, range=None, normed=False, weights=None,
+    def hist(self, x, bins=None, range=None, density=None, weights=None,


Changing the order is probably not a good idea, for backwards compatibility's sake, even though people really shouldn't be expecting order-only arguments to work this far in.

Hi @QuLogic!

My understanding is that if a user is expecting order-only arguments to work this far in, then their argument will just take the name density instead of normed now. And since the name density is meant, in this change, to actually replace normed, the code will still be backward compatible. If the developer specifically named it normed, then the fact that it's named allows it to work even though normed is at the end now.

Please let me know if this is inaccurate.

I think this is safe to do. If people are using positional it just works, if they are using keyword it still just works. By putting this here we make it clearer which the preferred name is.

QuLogic · 2017-01-17T04:31:00Z

lib/matplotlib/axes/_axes.py

            Default is ``None``

-        normed : boolean, optional
+        normed or density : boolean, optional


normed, density

suggest putting 'density' first as that will (eventually) be the preferred name.

QuLogic · 2017-01-17T04:31:42Z

lib/matplotlib/axes/_axes.py

            An array of weights, of the same shape as `x`.  Each value in `x`
            only contributes its associated weight towards the bin count
-            (instead of 1).  If `normed` is True, the weights are normalized,
+            (instead of 1).  If `normed` and/or 'density' is True, the weights are normalized,


Probably too long; wrap at 79 characters.

QuLogic · 2017-01-17T04:31:46Z

lib/matplotlib/axes/_axes.py

            If `True`, then a histogram is computed where each bin gives the
            counts in that bin plus all bins for smaller values. The last bin
-            gives the total number of datapoints.  If `normed` is also `True`
+            gives the total number of datapoints.  If `normed` and/or 'density' is also `True`


Probably too long; wrap at 79 characters.

QuLogic · 2017-01-17T04:32:20Z

lib/matplotlib/axes/_axes.py

        """
+
+        # This sets the density variable, if necessary, to its predecessor, 'normed.'
+        if density != None and normed != None and density != normed:


Use is not None.

QuLogic · 2017-01-17T04:32:31Z

lib/matplotlib/axes/_axes.py

+        # This sets the density variable, if necessary, to its predecessor, 'normed.'
+        if density != None and normed != None and density != normed:
+            raise ValueError('The density and normed arguments represent the same concept. Please set only one of them, or set them to the same value.')
+        elif normed != None and density == None:


Use is None.

QuLogic · 2017-01-17T04:34:46Z

lib/matplotlib/tests/test_axes.py

    ax = fig.add_subplot(111)
    ax.hist((d1, d2), stacked=True, normed=True)

+@image_comparison(baseline_images=['hist_stacked_normed'])


Two blank lines above.

QuLogic · 2017-01-17T04:35:17Z

lib/matplotlib/tests/test_axes.py

                      noverlap=noverlap, pad_to=pad_to, sides='onesided',
                      mode='phase', scale='dB')

-    with pytest.raises(ValueError):


Why is this removed?

@QuLogic it shouldn't have been. My mistake—good catch!

QuLogic · 2017-01-17T04:36:15Z

lib/matplotlib/tests/test_axes.py

+    d1 = np.linspace(1, 3, 20)
+    d2 = np.linspace(0, 10, 50)
+    fig = plt.figure()
+    ax = fig.add_subplot(111)


fig, ax = plt.subplots()

QuLogic · 2017-01-17T04:37:20Z

Mostly minor formatting things; it seems to make sense otherwise.

NelleV · 2017-01-17T18:06:50Z

If that makes your life easier, we are using the tool pep8 to check for coherent styles in matplotlib. You can install it and run it locally on your computer, and maybe even configure your text editor to display the pep8 violations.

tacaswell · 2017-01-17T22:56:11Z

lib/matplotlib/axes/_axes.py

+            set, then that value will be used. If neither are set, then the args
+            will be treated as 'False.'
+
+            If both are set to different things, the hist function raises an


I suggest 'if both are set, raise' even if they are the same.

Change made and pushed.

The docs are out of sync with the code.

chelseatroy · 2017-01-17T23:45:10Z

Pushed several changes as per @QuLogic's observations and suggestions!

chelseatroy · 2017-01-18T18:30:44Z

Force pushed to remove determinism_tex.svg from 3 commits back.

dstansby · 2017-02-01T17:56:33Z

There are still a few PEP 8 violations that are causing the tests to fail (https://travis-ci.org/matplotlib/matplotlib/jobs/193148586#L1777), would it be okay to fix them?

They look like they are all lines that are longer than 79 characters, which is the maximum allowed line length.

tacaswell · 2017-02-03T02:59:17Z

lib/matplotlib/axes/_axes.py

            only contributes its associated weight towards the bin count
-            (instead of 1).  If `normed` is True, the weights are normalized,
-            so that the integral of the density over the range remains 1.
+            (instead of 1).  If `normed` and/or 'density' is True,


should be just 'or'

tacaswell · 2017-02-03T02:59:44Z

lib/matplotlib/axes/_axes.py

-            If `cumulative` evaluates to less than 0 (e.g., -1), the direction
-            of accumulation is reversed.  In this case, if `normed` is also
-            `True`, then the histogram is normalized such that the first bin
+            gives the total number of datapoints. If `normed` and/or 'density'


tacaswell · 2017-02-03T03:00:04Z

lib/matplotlib/axes/_axes.py

        -------
        n : array or list of arrays
-            The values of the histogram bins. See **normed** and **weights**
+            The values of the histogram bins. See **normed or density** and **weights**


This line is likely too long and needs to be wrapped.

tacaswell · 2017-02-03T03:02:08Z

lib/matplotlib/tests/test_axes.py

    ax.hist((d1, d2), stacked=True, normed=True)


+@image_comparison(baseline_images=['hist_stacked_normed'])


If you are going to add an image test you also need to commit the baseline images.

If this adds coverage we don't have other wise, the images (or at least the png) should be added; if not, just drop the decorator.

tacaswell · 2017-03-31T13:09:12Z

@chelseatroy ping on this.

chelseatroy · 2017-06-27T19:49:42Z

Hi @tacaswell. Just saw this. I guess for some reason I didn't receive a notification about your ping on this PR. If this still hasn't been done, I can make the changes this week...though I suspect someone has beat me to it.

chelseatroy · 2017-06-27T19:59:06Z

@tacaswell I guess I'm pretty lost as to what to do from here. All the build checks are failing, I don't really understand why, I basically just changed the character width of some comments.

efiring · 2017-06-27T20:24:25Z

Presumably because of other changes since the last time you added a commit, there are conflicts so that your changes no longer merge cleanly. The "Resolve conflicts" button shows them nicely, and it looks like they will be easy to resolve.

tacaswell · 2017-06-28T01:55:34Z

=================================== FAILURES ===================================
 PEP8-check(ignoring E111 E114 E115 E116 E121 E122 E123 E124 E125 E126 E127 E128 E129 E131 E226 E240 E241 E242 E243 E244 E245 E246 E247 E248 E249 E265 E266 E704 W503) 
[gw0] linux -- Python 3.6.1 /home/travis/build/matplotlib/matplotlib/venv/bin/python
/home/travis/build/matplotlib/matplotlib/lib/matplotlib/axes/_axes.py:5874:80: E501 line too long (80 > 79 characters)
            set, then that value will be used. If neither are set, then the args
                                                                               ^
/home/travis/build/matplotlib/matplotlib/lib/matplotlib/axes/_axes.py:6013:1: W293 blank line contains whitespace
^
/home/travis/build/matplotlib/matplotlib/lib/matplotlib/axes/_axes.py:6014:80: E501 line too long (80 > 79 characters)
        # Sets the density variable, if necessary, to its predecessor, 'normed.'
                                                                               ^
!!!!!!!!!!!! xdist.dsession.Interrupted: stopping after 1 failures !!!!!!!!!!!!!

Looks like there are 2 formatting issues.

tacaswell · 2017-06-28T02:00:51Z

lib/matplotlib/axes/_axes.py

+        elif normed is None and density is None:
+            density = False
+
+        def _normalize_input(inp, ename='input'):


I am confused by this? Did this come from the merge?

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

Hi @chelseatroy, 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.

tacaswell added this to the 2.1 (next point release) milestone Jan 17, 2017

QuLogic requested changes Jan 17, 2017

View reviewed changes

tacaswell reviewed Jan 17, 2017

View reviewed changes

chelseatroy added 3 commits January 18, 2017 12:27

Replace hist normed arg with density arg in a backward-compatible way.

ca99315

Update documentation to reflect addition of density arg to hist

9034fc7

Adjust documentation and test formatting

932e4c8

chelseatroy force-pushed the histogram_compatibility_with_numpy_7364 branch from bdcaab5 to 932e4c8 Compare January 18, 2017 18:29

Raise if density and normed are both set in hist

a09dd87

tacaswell reviewed Feb 3, 2017

View reviewed changes

tacaswell mentioned this pull request Mar 18, 2017

Rely on numpy to properly normalize histograms with unequal bin widths #8333

Closed

phobson mentioned this pull request Mar 30, 2017

FIX: Introduced new keyword 'density' in the hist function #8408

Closed

Update documentation to match pep8 guidelines

a49cd96

Merge branch 'master' into histogram_compatibility_with_numpy_7364

408f236

tacaswell reviewed Jun 28, 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

anntzer mentioned this pull request Mar 15, 2018

Change manual kwargs popping to kwonly arguments. #10545

Merged

6 tasks

		@@ -0,0 +1,29 @@
		<?xml version="1.0" encoding="utf-8" standalone="no"?>

		ax.hist((d1, d2), stacked=True, normed=True)


		@image_comparison(baseline_images=['hist_stacked_normed'])

Uh oh!

Conversation

chelseatroy commented Jan 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuLogic commented Jan 17, 2017

Uh oh!

NelleV commented Jan 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chelseatroy commented Jan 17, 2017

Uh oh!

chelseatroy commented Jan 18, 2017

Uh oh!

dstansby commented Feb 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell Feb 3, 2017 • edited by QuLogic Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Mar 31, 2017

Uh oh!

chelseatroy commented Jun 27, 2017

Uh oh!

chelseatroy commented Jun 27, 2017

Uh oh!

efiring commented Jun 27, 2017

Uh oh!

tacaswell commented Jun 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dstansby commented Aug 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

tacaswell Feb 3, 2017 •

edited by QuLogic

Loading