BUG: pivot_table strings as aggfunc #18810

bobhaffner · 2017-12-17T01:16:48Z

closes BUG: pivot_table aggfunc should accept string function-likes #18713
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pep8speaks · 2017-12-17T01:16:51Z

Hello @bobhaffner! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on December 23, 2017 at 20:43 Hours UTC

codecov · 2017-12-17T02:05:16Z

Codecov Report

❗ No coverage uploaded for pull request base (master@507157d). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #18810   +/-   ##
=========================================
  Coverage          ?   91.65%           
=========================================
  Files             ?      154           
  Lines             ?    51368           
  Branches          ?        0           
=========================================
  Hits              ?    47080           
  Misses            ?     4288           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`89.52% <100%> (?)`
#single	`40.84% <0%> (?)`

Impacted Files	Coverage Δ
pandas/core/reshape/pivot.py	`96.35% <100%> (ø)`

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 507157d...e24d232. Read the comment docs.

gfyoung · 2017-12-17T21:28:29Z

doc/source/whatsnew/v0.22.0.txt

@@ -321,7 +321,8 @@ Reshaping
 - Bug in :func:`DataFrame.stack` which fails trying to sort mixed type levels under Python 3 (:issue:`18310`)
 - Fixed construction of a :class:`Series` from a ``dict`` containing ``NaN`` as key (:issue:`18480`)
 - Bug in :func:`Series.rank` where ``Series`` containing ``NaT`` modifies the ``Series`` inplace (:issue:`18521`)
-
+- Bug in :func:`Dataframe.pivot_table` which fails when you pass a string for aggfunc arg (:issue:`18713`)


We generally don't use the second-person (or any such person-pronouns in general).

bobhaffner · 2017-12-18T02:47:51Z

Thanks for the feedback @gfyoung.

gfyoung · 2017-12-18T02:58:26Z

pandas/tests/reshape/test_pivot.py

+        stds = f(np.std)
+        expected = concat([means, stds], keys=['mean', 'std'], axis=1)
+        tm.assert_frame_equal(result, expected)
+


I like what you're testing here, but I would modify a couple of things:

Add a newline after the first tm.assert_frame_equal. It makes it easier to read.

Construct the output explicitly using the DataFrame constructor (and check against that too). At this point, I know for example that f('sum') and f(np.sum) should be the same, but I don't know what the actual result is. Here is how I might go about writing the first half of this test.

... result = f('sum') func_result = f(np.sum) expected = DataFrame(...) tm.assert_frame_equal(result, func_result) # consistency tm.assert_frame_equal(result, expected) # correctness

you can parametrize this test by passing these functions in as well

something like

@pytest.mark.parametrize("f, f_numpy", [('sum', np.sum), ('mean', np.mean'), ('std', np.std]) def test_pivot_func_string(self, f, f_numpy):

Thank you, both, I'll incorporate these changes

jreback · 2017-12-18T12:27:35Z

pandas/tests/reshape/test_pivot.py

+        stds = f(np.std)
+        expected = concat([means, stds], keys=['mean', 'std'], axis=1)
+        tm.assert_frame_equal(result, expected)
+


you can parametrize this test by passing these functions in as well

something like

@pytest.mark.parametrize("f, f_numpy", [('sum', np.sum), ('mean', np.mean'), ('std', np.std]) def test_pivot_func_string(self, f, f_numpy):

bobhaffner · 2017-12-18T23:52:23Z

test_pivot_string_as_func is an attempt at @gfyoung's suggestion regarding comparing to a frame created from DataFrame()

test_pivot_string_func_vs_func is an attempt at @jreback's suggestion of using @pytest.mark.parametrize to compare string_funcs to numpy funcs

Separating the cases into two functions seemed like the right thing to do. As opposed to one rather long one with multiple DataFrame statements

gfyoung · 2017-12-18T23:57:41Z

pandas/tests/reshape/test_pivot.py

+             (['sum', 'mean'], [np.sum, np.mean]),
+             (['sum', 'std'], [np.sum, np.std]),
+             (['std', 'mean'], [np.std, np.mean])]
+


Remove newline.

This is a weird one, flake8 likes that newline

instead of using funcs, simply put this in the parametrize

gfyoung · 2017-12-18T23:57:52Z

pandas/tests/reshape/test_pivot.py

+
+    @pytest.mark.parametrize("f, f_numpy", funcs)
+    def test_pivot_string_func_vs_func(self, f, f_numpy):
+        # GH #18713


Add that this test is for consistency purposes.

gfyoung · 2017-12-18T23:58:02Z

pandas/tests/reshape/test_pivot.py

@@ -1109,6 +1109,48 @@ def test_pivot_margins_name_unicode(self):
        expected = pd.DataFrame(index=index)
        tm.assert_frame_equal(table, expected)

+    def test_pivot_string_as_func(self):
+        # GH #18713


Add that this test is for correctness.

gfyoung · 2017-12-18T23:58:26Z

Separating the cases into two functions seemed like the right thing to do. As opposed to one rather long one with multiple DataFrame statements.

Seems reasonable to me.

bobhaffner · 2017-12-20T14:03:10Z

Just following up on this to make sure no other changes are required. No worries if folks are busy

jreback · 2017-12-21T15:17:39Z

pandas/tests/reshape/test_pivot.py

+             (['sum', 'mean'], [np.sum, np.mean]),
+             (['sum', 'std'], [np.sum, np.std]),
+             (['std', 'mean'], [np.std, np.mean])]
+


instead of using funcs, simply put this in the parametrize

jreback · 2017-12-21T17:26:15Z

doc/source/whatsnew/v0.22.0.txt

@@ -341,7 +341,8 @@ Reshaping
 - Bug in :func:`DataFrame.stack` which fails trying to sort mixed type levels under Python 3 (:issue:`18310`)
 - Fixed construction of a :class:`Series` from a ``dict`` containing ``NaN`` as key (:issue:`18480`)
 - Bug in :func:`Series.rank` where ``Series`` containing ``NaT`` modifies the ``Series`` inplace (:issue:`18521`)
-
+- Bug in :func:`Dataframe.pivot_table` which fails when the ``aggfunc`` arg is of type string.  The behavior is now consistent with other methods like ``agg`` and ``apply`` (:issue:`18713`)


rebase on master. can you move to 0.23 (docs were renamed), prob easiest to just check this file from master and past in new one

bobhaffner · 2017-12-21T18:00:38Z

Very interesting. Looks like the rebase handled the whatsnew change quite nicely

(pandas-dev) Bobs-MacBook-Pro:pandas-dev bob$ git rebase upstream/master
First, rewinding head to replay your work on top of it...
Applying: pivot_table strings as aggfunc
Using index info to reconstruct a base tree...
A	doc/source/whatsnew/v0.22.0.txt
.git/rebase-apply/patch:11: trailing whitespace.
- 
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging doc/source/whatsnew/v0.23.0
Applying: fixed pep8 issues
Applying: fixed whatsnew
Using index info to reconstruct a base tree...
A	doc/source/whatsnew/v0.22.0.txt
Falling back to patching base and 3-way merge...
Auto-merging doc/source/whatsnew/v0.23.0
Applying: updated tests
Applying: added comments to test funcs
Applying: removed list usage in test parametrize

gfyoung · 2017-12-21T18:03:25Z

@bobhaffner : This is git - you should expect great things from it 😄 (unless you're pushing large files OR operating at big-tech-company-scale... :0 )

bobhaffner · 2017-12-21T18:08:41Z

@gfyoung Haha, gotcha :-)

jreback · 2017-12-21T18:37:12Z

lgtm. ping on green.

bobhaffner · 2017-12-22T19:24:40Z

lgtm. ping on green.

@jreback is this intended for me or some kind of shorthand for a notification process? At any rate, ping :-)

Looks like there's a conflict with the whatsnew piece being changed back to the old name. I can follow suit or wait till this is sorted out.

jreback · 2017-12-23T20:43:38Z

thanks @bobhaffner

yes 'ping on green' is a notification of you to ping me when this has passed and all buttons are green.

bobhaffner · 2017-12-24T00:16:07Z

Thanks @jreback and @gfyoung!!!

gfyoung reviewed Dec 17, 2017

View reviewed changes

gfyoung added Compat pandas objects compatability with Numpy or Python functions Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels Dec 17, 2017

gfyoung reviewed Dec 18, 2017

View reviewed changes

jreback requested changes Dec 18, 2017

View reviewed changes

bobhaffner force-pushed the pv_allow_str_aggfuncs branch from 6a1f4f6 to fe36303 Compare December 18, 2017 23:44

gfyoung reviewed Dec 18, 2017

View reviewed changes

jreback requested changes Dec 21, 2017

View reviewed changes

bobhaffner force-pushed the pv_allow_str_aggfuncs branch from c4921c1 to 1a9cfeb Compare December 21, 2017 16:04

jreback requested changes Dec 21, 2017

View reviewed changes

bobhaffner added 6 commits December 21, 2017 11:48

pivot_table strings as aggfunc

70a45d7

fixed pep8 issues

5b68ae1

fixed whatsnew

c4d4731

updated tests

54b80af

added comments to test funcs

00f2b44

removed list usage in test parametrize

0decc05

bobhaffner force-pushed the pv_allow_str_aggfuncs branch from 1a9cfeb to 0decc05 Compare December 21, 2017 17:57

jreback added this to the 0.23.0 milestone Dec 21, 2017

jreback approved these changes Dec 21, 2017

View reviewed changes

jreback added 2 commits December 23, 2017 15:41

Merge branch 'master' into PR_TOOL_MERGE_PR_18810

54644f3

doc edit

e24d232

jreback merged commit 81adbe6 into pandas-dev:master Dec 23, 2017

bobhaffner deleted the pv_allow_str_aggfuncs branch December 24, 2017 00:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: pivot_table strings as aggfunc #18810

BUG: pivot_table strings as aggfunc #18810

bobhaffner commented Dec 17, 2017

pep8speaks commented Dec 17, 2017 •

edited

Loading

codecov bot commented Dec 17, 2017 •

edited

Loading

gfyoung Dec 17, 2017

bobhaffner commented Dec 18, 2017

gfyoung Dec 18, 2017 •

edited

Loading

jreback Dec 18, 2017 •

edited

Loading

bobhaffner Dec 18, 2017

jreback Dec 18, 2017 •

edited

Loading

bobhaffner commented Dec 18, 2017

gfyoung Dec 18, 2017

bobhaffner Dec 19, 2017

jreback Dec 21, 2017

gfyoung Dec 18, 2017

gfyoung Dec 18, 2017

gfyoung commented Dec 18, 2017

bobhaffner commented Dec 20, 2017

jreback Dec 21, 2017

jreback Dec 21, 2017

bobhaffner commented Dec 21, 2017

gfyoung commented Dec 21, 2017

bobhaffner commented Dec 21, 2017

jreback commented Dec 21, 2017

bobhaffner commented Dec 22, 2017

jreback commented Dec 23, 2017

bobhaffner commented Dec 24, 2017

BUG: pivot_table strings as aggfunc #18810

BUG: pivot_table strings as aggfunc #18810

Conversation

bobhaffner commented Dec 17, 2017

pep8speaks commented Dec 17, 2017 • edited Loading

Comment last updated on December 23, 2017 at 20:43 Hours UTC

codecov bot commented Dec 17, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

bobhaffner commented Dec 18, 2017

gfyoung Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

jreback Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

bobhaffner commented Dec 18, 2017

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

gfyoung commented Dec 18, 2017

bobhaffner commented Dec 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobhaffner commented Dec 21, 2017

gfyoung commented Dec 21, 2017

bobhaffner commented Dec 21, 2017

jreback commented Dec 21, 2017

bobhaffner commented Dec 22, 2017

jreback commented Dec 23, 2017

bobhaffner commented Dec 24, 2017

pep8speaks commented Dec 17, 2017 •

edited

Loading

codecov bot commented Dec 17, 2017 •

edited

Loading

gfyoung Dec 18, 2017 •

edited

Loading

jreback Dec 18, 2017 •

edited

Loading

jreback Dec 18, 2017 •

edited

Loading