ENH: option for groupby.hist to match bins #22228

javadnoorb · 2018-08-07T03:37:54Z

closes ENH: groupby.hist bins don't match #22222
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Example code and result:

%matplotlib inline
import numpy as np
import pandas as pd

def test_hist_bins_match():
    # GH-22222
    N = 100
    bins = 5

    np.random.seed(0)
    df = pd.DataFrame(np.append(np.random.randn(N), np.random.randn(N) / 10),
                   columns=['rand'])
    df['group'] = [0] * N + [1] * N
    g = df.groupby('group')['rand']
    bin_range = np.linspace(df['rand'].min(), df['rand'].max(), bins + 1)

    ax = g.hist(bins=bins, alpha=0.7, equal_bins=True)
    both_hists_max = g.apply(lambda x: max(
        np.histogram(x, bins=bin_range)[0])).max()
    assert ax.iloc[0].get_ylim()[1] >= both_hists_max

test_hist_bins_match()

WillAyd

Thanks for the PR!

WillAyd · 2018-08-07T15:24:56Z

pandas/core/groupby/groupby.py

@@ -582,6 +582,15 @@ def wrapper(*args, **kwargs):
               kwargs_with_axis['axis'] is None:
                kwargs_with_axis['axis'] = self.axis

+            if (name == 'hist' and
+                    kwargs.pop('equal_bins', False)):


Can you get the same behavior without mutating the kwargs dict?

Yes, I can pass equal_bins to hist_series. Do you also mean this for line 588? Just asking, because that might be a bit tricky to pipe through.

Less concerned about 588 since you end up replacing that anyway

Thanks. I ended up copying kwargs and mutating that.

WillAyd · 2018-08-07T15:26:22Z

pandas/core/groupby/groupby.py

+                    kwargs.pop('equal_bins', False)):
+                # GH-22222
+                bins = kwargs.pop('bins', None)
+                if type(bins) == int:


Would be preferable I think to use is_integer from pandas.core.dtypes.inference

WillAyd · 2018-08-07T15:30:05Z

pandas/tests/plotting/test_groupby.py

+    ax = g.hist(bins=bins, alpha=0.7, equal_bins=True)
+    both_hists_max = g.apply(lambda x: max(
+        np.histogram(x, bins=bin_range)[0])).max()
+    assert ax.iloc[0].get_ylim()[1] >= both_hists_max


Is it not possible to make a stronger assertion here about the actual binned values?

Good suggestion! I'm comparing bar widths in the new test.

codecov · 2018-08-09T03:06:47Z

Codecov Report

Merging #22228 into master will decrease coverage by <.01%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master   #22228      +/-   ##
==========================================
- Coverage   92.08%   92.08%   -0.01%     
==========================================
  Files         169      169              
  Lines       50691    50698       +7     
==========================================
+ Hits        46681    46687       +6     
- Misses       4010     4011       +1

Flag	Coverage Δ
#multiple	`90.49% <85.71%> (-0.01%)`	⬇️
#single	`42.33% <0%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/plotting/_core.py	`83.48% <ø> (ø)`	⬆️
pandas/core/groupby/groupby.py	`96.04% <85.71%> (-0.12%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab274c2...adccd1a. Read the comment docs.

javadnoorb · 2018-08-09T03:23:55Z

pandas/core/groupby/groupby.py

            def curried_with_axis(x):
                return f(x, *args, **kwargs_with_axis)

            def curried(x):
-                return f(x, *args, **kwargs)
+                return f(x, *args, **kwargs_wo_axis)


line 611 will be run b/c 'hist' is in base.plotting_methods, the result is returned, so this change should only have a local effect.

javadnoorb · 2018-08-09T03:25:18Z

pandas/plotting/_core.py

@@ -2470,8 +2470,11 @@ def hist_series(self, by=None, ax=None, grid=True, xlabelsize=None,
        bin edges are calculated and returned. If bins is a sequence, gives
        bin edges, including left edge of first bin and right edge of last
        bin. In this case, bins is returned unmodified.
-    bins: integer, default 10
-        Number of histogram bins to be used


This is repeated and seems redundant.

Repeated where? This should still stay, no?

bins is defined on line 2468 and 2473. It looks like a mistake when writing the docstring. What do you think?

Ah OK didn't see that. This is fine then

WillAyd · 2018-08-11T17:02:17Z

pandas/plotting/_core.py

@@ -2470,8 +2470,11 @@ def hist_series(self, by=None, ax=None, grid=True, xlabelsize=None,
        bin edges are calculated and returned. If bins is a sequence, gives
        bin edges, including left edge of first bin and right edge of last
        bin. In this case, bins is returned unmodified.
-    bins: integer, default 10
-        Number of histogram bins to be used


Repeated where? This should still stay, no?

WillAyd · 2018-08-11T17:03:44Z

pandas/plotting/_core.py

@@ -2480,6 +2483,7 @@ def hist_series(self, by=None, ax=None, grid=True, xlabelsize=None,
    matplotlib.axes.Axes.hist : Plot a histogram using matplotlib.

    """
+    # TODO: separate docstrings of series and groupby hist functions (GH-22241)


Don't need this comment here - the open issue should suffice

WillAyd · 2018-08-11T17:04:53Z

pandas/tests/plotting/test_groupby.py

+    g = df.groupby('group')['rand']
+    ax = g.hist(bins=bins, alpha=0.7, equal_bins=True)[0]
+    bin_width_group0 = ax.patches[0].get_width()
+    bin_width_group1 = ax.patches[bins].get_width()


Instead of using np.random can you not define a set of data whereby you can also easily assert that both groups have an equal number of bins across the same X-scale?

I can replace np.random with some predefined values. But I think that comparing the number of bins wouldn't do the job. That's the current state of the code. We're here just changing the range without changing the number of bins.

I've written a new test, that compares the x-axis values of the bins, that should be more rigorous. For some reason the functions see kwargs differently in python 2 and 3, so some CI tests fail in the former. I'll make a commit once I figure out what's going on.

WillAyd · 2018-08-11T17:05:34Z

pandas/tests/plotting/test_groupby.py

@@ -12,6 +12,23 @@
 from pandas.tests.plotting.common import TestPlotBase


+@td.skip_if_no_mpl
+def test_hist_bins_match():


Move this function into the class that already exists in the module

WillAyd · 2018-08-11T17:06:50Z

pandas/core/groupby/groupby.py

            if 'axis' not in kwargs_with_axis or \
               kwargs_with_axis['axis'] is None:
                kwargs_with_axis['axis'] = self.axis

+            if name == 'hist' and kwargs_wo_axis.pop('equal_bins', False):


I don't think another copy of kwargs is the cleanest solution here - can you not just get from the kwargs dict instead of popping?

In that case matplotltib.pyplot.hist throws an error that equal_bins is not recognized. An alternative solution that I can think of is to pass equal_bins as a named argument to hist_series in pandas/plotting/_core.py. But then it will be a dummy variable that's not used within the function. Any suggestions?

To clarify a bit more ~~~, in python3 (but for some reason not python2),~~~ any extra argument passed to kwargswill be read in this line (as kwds):

pandas/pandas/plotting/_core.py

Line 2501 in 0370740

ax.hist(values, bins=bins, **kwds)

javadnoorb · 2018-08-11T19:52:51Z

There is a python2 versus python3 discrepancy that's causing some CI tests to fail. I opened #22285 to deal with it.

Merge branch 'master' of https://github.com/pandas-dev/pandas into hist_match_bins_tmp1_rebased

javadnoorb · 2018-08-13T17:27:32Z

One job failed for no reason. Otherwise it's green.

javadnoorb · 2018-08-13T17:29:34Z

pandas/plotting/_core.py

@@ -2443,7 +2443,7 @@ def hist_frame(data, column=None, by=None, grid=True, xlabelsize=None,

 def hist_series(self, by=None, ax=None, grid=True, xlabelsize=None,
                xrot=None, ylabelsize=None, yrot=None, figsize=None,
-                bins=10, **kwds):
+                bins=10, equal_bins=False, **kwds):


I'm trying the alternative approach here. equal_bins will be unused within the function. This is just to avoid popping kwds.

This also takes care of py2/3 compat issue.

Might have missed the compat issue but if you are using .get it shouldn't matter whether this exists as a keyword or not

kwds can only contain matploltib.pyplot.hist input args (b/c of line 2504) and throws an error if equal_bins is a keyword.

javadnoorb · 2018-08-13T17:30:53Z

pandas/tests/plotting/test_groupby.py

+    @pytest.mark.parametrize(
+        'bins, equal_bins',
+        zip([5, None, np.linspace(-3, 3, 10)], [True, False])
+    )


Trying all "possible" forms of bins with and without equal_bins

Would still prefer if you could be explicit about what is expected here, i.e. add expected as a third parameter here and assert that we get the correct number of bins across the two groups

WillAyd · 2018-08-13T18:09:18Z

pandas/core/groupby/groupby.py

@@ -581,6 +582,17 @@ def wrapper(*args, **kwargs):
            if 'axis' not in kwargs_with_axis or \
               kwargs_with_axis['axis'] is None:
                kwargs_with_axis['axis'] = self.axis
+            if (name == 'hist' and
+                    kwargs.get('equal_bins', False) is True):


Do we need is True here or can we use the implicit truthiness of objects?

It's just a bit safer this way, as it guards against some non-boolean statements by the user, such as equal_bins=None. But your suggestion should be fine if we ignore bad inputs from user.

WillAyd · 2018-08-13T18:11:15Z

pandas/core/groupby/groupby.py

+                bins = kwargs.get('bins')
+                if bins is None:
+                    bins = 10  # use default value used in `hist_series`
+                if is_integer(bins):


Could just use is_scalar here since we only need to guard against scalar vs sequence

WillAyd · 2018-08-13T18:12:03Z

pandas/core/groupby/groupby.py

+                    kwargs.get('equal_bins', False) is True):
+                # GH-22222
+                bins = kwargs.get('bins')
+                if bins is None:


What is the purpose of this? Doesn't the keyword already default to 10?

The keyword defaults to 10 after the function is called (curried_axis), but we need its value before that (on line 593)

WillAyd · 2018-08-13T18:12:49Z

pandas/tests/plotting/test_groupby.py



 @td.skip_if_no_mpl
 class TestDataFrameGroupByPlots(TestPlotBase):
+    @pytest.mark.parametrize(


Readability nit but let's add an empty line here

WillAyd · 2018-08-13T18:13:40Z

pandas/tests/plotting/test_groupby.py

+                index=[0, 1], columns=['min', 'max'])
+            group_ranges = g.agg([min, max])
+            assert np.isclose(group_ranges, hist_ranges).all()
+        tm.close()


Not sure what the purpose of this call is?

tm.close() closes any open figure. Otherwise because of pytest.parameterize we will keep plotting on the same figure and accumulate counts.

WillAyd · 2018-08-13T18:15:54Z

pandas/tests/plotting/test_groupby.py

+        axes = g.hist(bins=bins, alpha=0.7, equal_bins=equal_bins)
+        ax = axes[0]
+
+        if bins is None:


Just generally avoid this type of code in tests - you aren't asserting anything about the functionality of the code being tested but rather just adding separate logic here in the test case

The test should be slightly different, depending on the type provided for bins. Do you recommend dropping parametrize decorator and writing separate tests for these instead?

WillAyd · 2018-08-13T18:16:48Z

pandas/tests/plotting/test_groupby.py

+    @pytest.mark.parametrize(
+        'bins, equal_bins',
+        zip([5, None, np.linspace(-3, 3, 10)], [True, False])
+    )


Would still prefer if you could be explicit about what is expected here, i.e. add expected as a third parameter here and assert that we get the correct number of bins across the two groups

javadnoorb · 2018-08-14T02:12:02Z

doc/source/whatsnew/v0.24.0.txt

@@ -640,6 +640,7 @@ Plotting

 - Bug in :func:`DataFrame.plot.scatter` and :func:`DataFrame.plot.hexbin` caused x-axis label and ticklabels to disappear when colorbar was on in IPython inline backend (:issue:`10611`, :issue:`10678`, and :issue:`20455`)
 - Bug in plotting a Series with datetimes using :func:`matplotlib.axes.Axes.scatter` (:issue:`22039`)
+- The new argument ``equal_bins`` in :func:`SeriesGroupBy.hist` sets histogram bins to equal values (:issue:`22222`)


This is actually hist method of a SeriesGroupBy object, although that't not available in the docs:

https://pandas.pydata.org/pandas-docs/stable/api.html

Should it be added?

This is available in api.rst if that suffices:

The following methods are available in both ``SeriesGroupBy`` and ``DataFrameGroupBy`` objects, but may differ slightly, usually in that the ``DataFrameGroupBy`` version usually permits the specification of an axis argument, and often an argument indicating whether to restrict application to columns of a specific data type. .. autosummary:: :toctree: generated/ ... DataFrameGroupBy.hist

Just change the reference to DataFrameGroupBy.hist in the whatsnew - should suffice

javadnoorb · 2018-08-14T02:52:39Z

pandas/tests/plotting/test_groupby.py



 @td.skip_if_no_mpl
 class TestDataFrameGroupByPlots(TestPlotBase):

+    @pytest.mark.parametrize('equal_bins, bins, expected1, expected2', (


Need two expected variables for the two groups. They correspond to the leftmost part of each histogram bar on the x-axis.

javadnoorb · 2018-08-14T02:54:08Z

pandas/tests/plotting/test_groupby.py

+
+        tm.assert_almost_equal(points[:num_bins], np.array(expected1))
+        tm.assert_almost_equal(points[num_bins:], np.array(expected2))
+        tm.close()


this is to close the figure (to avoid overwriting it during parametrize)

jreback · 2018-08-14T03:07:39Z

pandas/core/groupby/groupby.py

@@ -581,6 +581,16 @@ def wrapper(*args, **kwargs):
            if 'axis' not in kwargs_with_axis or \
               kwargs_with_axis['axis'] is None:
                kwargs_with_axis['axis'] = self.axis
+            if name == 'hist' and kwargs.get('equal_bins', False):
+                # GH-22222
+                bins = kwargs.get('bins')


do not add anything here

this is obscuring already extremely fragile code

Do you mean not in the wrapper? Or also beyond that? Is there any other place you'd suggest?

WillAyd · 2018-08-14T03:01:21Z

doc/source/whatsnew/v0.24.0.txt

@@ -640,6 +640,7 @@ Plotting

 - Bug in :func:`DataFrame.plot.scatter` and :func:`DataFrame.plot.hexbin` caused x-axis label and ticklabels to disappear when colorbar was on in IPython inline backend (:issue:`10611`, :issue:`10678`, and :issue:`20455`)
 - Bug in plotting a Series with datetimes using :func:`matplotlib.axes.Axes.scatter` (:issue:`22039`)
+- The new argument ``equal_bins`` in :func:`SeriesGroupBy.hist` sets histogram bins to equal values (:issue:`22222`)


Just change the reference to DataFrameGroupBy.hist in the whatsnew - should suffice

WillAyd · 2018-08-14T03:02:23Z

pandas/core/groupby/groupby.py

@@ -581,6 +581,16 @@ def wrapper(*args, **kwargs):
            if 'axis' not in kwargs_with_axis or \
               kwargs_with_axis['axis'] is None:
                kwargs_with_axis['axis'] = self.axis
+            if name == 'hist' and kwargs.get('equal_bins', False):
+                # GH-22222
+                bins = kwargs.get('bins')


Hmm wondering if we even need to use .get here - can we just access by index? I'm assuming that 'equal_bins' should always be there so a KeyError when it's not would be fine

same py2/3 issue. In py3 equal_bins, bins, etc do not take their default values unless the code inside hist_series function is being run.

WillAyd · 2018-08-14T03:04:27Z

pandas/plotting/_core.py

@@ -2443,7 +2443,7 @@ def hist_frame(data, column=None, by=None, grid=True, xlabelsize=None,

 def hist_series(self, by=None, ax=None, grid=True, xlabelsize=None,
                xrot=None, ylabelsize=None, yrot=None, figsize=None,
-                bins=10, **kwds):
+                bins=10, equal_bins=False, **kwds):


Might have missed the compat issue but if you are using .get it shouldn't matter whether this exists as a keyword or not

WillAyd · 2018-08-14T03:04:48Z

pandas/core/groupby/groupby.py

@@ -581,6 +581,16 @@ def wrapper(*args, **kwargs):
            if 'axis' not in kwargs_with_axis or \
               kwargs_with_axis['axis'] is None:
                kwargs_with_axis['axis'] = self.axis
+            if name == 'hist' and kwargs.get('equal_bins', False):


Can just do kwargs.get('equal_bins') - no need for the False argument

For some reason py3 and py2 kwargs behavior are different. In py3 kwargs defaults are not set here if the parameter is not provided by the user. I opened an issue on this (#22285). I'm not sure where to move the code (per @jreback's comment) to avoid this.

WillAyd · 2018-08-14T03:09:28Z

pandas/core/groupby/groupby.py

+            if name == 'hist' and kwargs.get('equal_bins', False):
+                # GH-22222
+                bins = kwargs.get('bins')
+                if bins is None:


Can you point me to where bins doesn't default to 10?

WillAyd · 2018-08-14T03:10:08Z

pandas/tests/plotting/test_groupby.py

+        (True, 10,
+         [-3., -2.4, -1.8, -1.2, -0.6, 0., 0.6, 1.2, 1.8, 2.4],
+         [-3., -2.4, -1.8, -1.2, -0.6, 0., 0.6, 1.2, 1.8, 2.4]),
+        (True, None,


Assuming the default is always 10 for bins then None is not a valid argument here and should in fact raise an error instead of giving valid results

WillAyd · 2018-08-14T03:12:28Z

pandas/tests/plotting/test_groupby.py

+        points = np.array([patch.get_bbox().get_points()
+                           for patch in ax.patches])[:, 0, 0]
+
+        tm.assert_almost_equal(points[:num_bins], np.array(expected1))


Would prefer to use tm.assert_numpy_array_equal instead here and on line below

javadnoorb · 2018-08-15T03:48:16Z

Making a new function that does this is pretty straightforward:

diff --git a/pandas/core/groupby/groupby.py b/pandas/core/groupby/groupby.py
index 3f84fa0..2d324f4 100644
--- a/pandas/core/groupby/groupby.py
+++ b/pandas/core/groupby/groupby.py
@@ -1001,6 +1001,13 @@ class GroupBy(_GroupBy):
     len(grouped) : int
         Number of groups
     """
+    def new_hist(self, bins=10, equal_bins=False, **kwargs):
+        if equal_bins and is_scalar(bins):
+            # share the same numpy array for all group bins
+            bins = np.linspace(self.obj.min(),
+                               self.obj.max(), bins + 1)
+        return self.hist(bins=bins, **kwargs)
+
     def _bool_agg(self, val_test, skipna):
         """Shared func to call any / all Cython GroupBy implementations"""

but I don't know how to overload SeriesGroupBy.hist without touching the wrappers, or some other sensitive part of the code. I defer this PR to anyone who knows how to do this.

WillAyd · 2018-11-23T03:32:55Z

Closing as stale. Ping if you'd like to pick this back up

javadnoorb added 4 commits August 6, 2018 21:21

ENH: option to equalize histogram bin widths

8a9ddfb

ENH: option to equalize histogram bin widths

2d003b4

ENH: option to equalize histogram bin widths

277035a

ENH: fix docstring

8e3feb7

javadnoorb changed the title ~~ENH: option to for goupby.hist to match bins~~ ENH: option for goupby.hist to match bins Aug 7, 2018

WillAyd requested changes Aug 7, 2018

View reviewed changes

WillAyd added Visualization plotting Groupby labels Aug 7, 2018

javadnoorb changed the title ~~ENH: option for goupby.hist to match bins~~ ENH: option for groupby.hist to match bins Aug 9, 2018

ENH/TST/DOC: no mut kwargs - use is_integer/test bar widths/move doc

006aea7

javadnoorb commented Aug 9, 2018

View reviewed changes

TST: more rigorous test

f488e88

WillAyd requested changes Aug 11, 2018

View reviewed changes

javadnoorb added 6 commits August 11, 2018 15:57

debugging py2/3 discrepancy

5a14dec

remove kwargs copy mutation/make test more rigorous

650405d

sync

a5a287f

Merge branch 'master' of https://github.com/pandas-dev/pandas into hist_match_bins_tmp1_rebased

syncing the fork

82c1cc9

more rigorous test / remove kwargs copy

18c2564

whitespace fix

d52ec4b

javadnoorb commented Aug 13, 2018

View reviewed changes

WillAyd requested changes Aug 13, 2018

View reviewed changes

javadnoorb added 2 commits August 13, 2018 22:03

CLN: use expected in test / other minor changes

4a42feb

whatsnew update

adccd1a

javadnoorb commented Aug 14, 2018

View reviewed changes

jreback requested changes Aug 14, 2018

View reviewed changes

WillAyd requested changes Aug 14, 2018

View reviewed changes

WillAyd closed this Nov 23, 2018

ENH: option for groupby.hist to match bins #22228

ENH: option for groupby.hist to match bins #22228

Conversation

javadnoorb commented Aug 7, 2018 • edited Loading

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 9, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javadnoorb Aug 11, 2018 • edited Loading

Choose a reason for hiding this comment

javadnoorb commented Aug 11, 2018 • edited Loading

javadnoorb commented Aug 13, 2018

javadnoorb Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javadnoorb Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javadnoorb commented Aug 15, 2018

WillAyd commented Nov 23, 2018

javadnoorb commented Aug 7, 2018 •

edited

Loading

codecov bot commented Aug 9, 2018 •

edited

Loading

javadnoorb Aug 11, 2018 •

edited

Loading

javadnoorb commented Aug 11, 2018 •

edited

Loading

javadnoorb Aug 13, 2018 •

edited

Loading

javadnoorb Aug 13, 2018 •

edited

Loading