BUG: groupby.transform(name) validates name is an aggregation #27597

ghost · 2019-07-25T23:16:22Z

closes #14274
closes #19354
closes #22509

tests added / passed
whatsnew entry

This is a long-standing bug. Effectively, it makes transform('rank') synonymous with g.rank() and same for other transformations.

We could also resolve this by restricting the g.transform(name) whitelist to only callables and aggs-to-be-broadcasted, telling users to use the named method directly, i.e g.rank() instead. Not a problem keeping this path working though, it just needs to return correct results.

Also, continuing some cleanups after #27467

doc/source/whatsnew/v1.0.0.rst

pandas/core/groupby/generic.py

WillAyd · 2019-07-26T00:29:43Z

Might have gotten lost in a conversation but should these just raise? I think these muddle the API a bit for transform.

I thought you suggested raising for non-reducing items going through agg seems like that could apply here as well

ghost · 2019-07-26T00:57:32Z

I thought you suggested raising for non-reducing items going through agg seems like that could apply here as well

I suggested agg should raise for transformations (will submit it next). That's not the same as transform raising for transformations, and it's main utility is turning aggs into transformations by broadcasting. So not the same case. I thought this route would be less disruptive.

I'd be perfectly ok with restricting g.transform to callables and agg-to-be-broadcasted only, that's reasonable, Just as long as there's agreement on this and it doesn't trigger a tug-of-war that stalls this indefinitely. g.transform('rank') has been returning nonsense for years and I want to finally fix it right now.

WillAyd · 2019-07-26T01:03:01Z

Right I'd be +1 for that restriction. In my head I would imagine that we could use the agg whitelist to validate what gets sent to both .agg and .transform

ghost · 2019-07-26T01:03:13Z

Thank you for reviewing

ghost · 2019-07-26T01:04:31Z

I would imagine that we could use the agg whitelist to validate what gets sent to both .agg and
.transform

Did you see the lists added in #27467? I did that so we could do both.

WillAyd · 2019-07-26T01:06:07Z

Yea that's what I was loosely referring to. Let's see what @jreback thinks

jreback

I agree that a non-transformation function (either directly, e.g. rank or an aggregation which broadcasts) should raise.

doc/source/whatsnew/v1.0.0.rst

pep8speaks · 2019-07-26T16:06:51Z

Hello @pilkibun! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-07-26 20:09:14 UTC

ghost · 2019-07-26T17:28:55Z

Ok. Passing the name of anything not on the list of reductions now raises an exception. Further, if the method exists on Grouper, the user is also advised to call the method directly.

I also created a section under breaking changes with Previous/New behavior examples.

TomAugspurger

It's not a huge deal, but I think that this could reasonably be called a bug fix. Is there any case where .transform(name) was doing the "expected" thing that we'll now raise on?

doc/source/whatsnew/v1.0.0.rst

ghost · 2019-07-26T19:47:32Z

was doing the "expected" thing that we'll now raise on?

all the cum* functions, having been cythonized, took a direct path which was not affected by broadcasting. They are all transformations, not aggs, and will now raise.

TomAugspurger · 2019-07-26T20:31:33Z

all the cythonized cum* functions took a direct path which was not affected. They are all transformations, not aggs, and will now raise.

Can you give an example?

ghost · 2019-07-26T20:40:28Z

of what?

TomAugspurger · 2019-07-26T20:45:39Z

Does transform(‘cumsum’) raise? If so, why? That seems OK to me.

…

On Jul 26, 2019, at 15:40, pilkibun ***@***.***> wrote: of what? — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

ghost · 2019-07-26T20:51:51Z

all the cum* functions are transformations, not aggs. Those are now meant to be called only directly.

TomAugspurger · 2019-07-26T21:00:30Z

-1 on outright breaking that then.

And I would need to reread the issue to understand the motivation for changing it.

ghost · 2019-07-26T21:02:14Z

oh my god.

#27597 (comment)

jreback

can you add tests for the ffill issue that you are closing

jreback · 2019-07-26T21:00:40Z

doc/source/whatsnew/v1.0.0.rst

+
+In previous releases, :meth:`DataFrameGroupBy.transform` and
+:meth:`SeriesGroupBy.transform`  did not validate that the function name
+passed was actually the name of an aggregation. As a result, users might get a


remove the: As a result.... sentence.

rised -> raise

jreback · 2019-07-26T21:01:20Z

doc/source/whatsnew/v1.0.0.rst

+
+.. code-block:: ipython
+
+    In [1]: g.transform('ers >= Decepticons')


just pass it a name like 'foo'

jreback · 2019-07-26T21:02:16Z

doc/source/whatsnew/v1.0.0.rst

+.. ipython:: python
+   :okexcept:
+
+   g.transform('ers >= Decepticons')


use foo, & make this a code-block (so we don't have the long traceback)

put the 'rank' in its own ipython block; I would also show .rank() or at least indicate that they are now the same.

jreback · 2019-07-26T21:03:34Z

pandas/core/groupby/groupby.py

@@ -241,8 +241,9 @@ class providing the base-class of operations.

 Parameters
 ----------
-f : function
-    Function to apply to each group
+func : callable or str


leave this as f, otherwise this is an api change

jreback · 2019-07-26T21:03:59Z

pandas/tests/groupby/test_transform.py

@@ -1052,6 +1051,20 @@ def test_transform_agg_by_name(reduction_func, obj):
    assert len(set(DataFrame(result).iloc[-3:, -1])) == 1


+def test_transform_transformation_by_name(transformation_func):
+    """Make sure g.transform('name') raises a helpful error for non-agg


can you add the issue refences numbers as a comment

TomAugspurger · 2019-07-26T22:12:31Z

Is #27597 (comment) answering why “cumsum” should raise?

There’s value in accepting non-agg transform names here. We have an open issue about accepting lists of functions in transform. That change would prohibit .transform([“cummin”, “cummax”]). And using .transform to signal to your reader that the operation is indeed a transform is valuable as well.

ghost · 2019-07-29T18:12:27Z

Is #27597 (comment) answering why “cumsum” should raise?

Tom, I asked you about this exact issue in #27389 two weeks ago. In response you said you found the bug useful and then then proceeded to ignore my followup. Then you showed up here, after I've written the patch, and the tests, and the docs, skipped over the preceding discussion with your peers, ignored the fact that two of them asked me to make the change you're objecting to, and casually expect me to rewrite this a third time.

I think that's awful behavior, and it's very effectively undermining my desire to help improve pandas.

If this is what I have to put up with in order to volunteer my time to fix serious, long-standing bugs, it's a bad deal and my answer is no.

TomAugspurger · 2019-07-29T18:36:19Z

In response you said you found the bug useful

What comment is that? In #27389 (comment) I said that I find "broadcasting a transformation result useful in some other cases?". I'm not sure anymore what that's referring to since the post may have been edited.

and then then proceeded to ignore my followup.

Don't attribute to malice what can be attributed to busyness.

Then you showed up here, after I've written the patch, and the tests, and the docs, skipped over the preceding discussion with your peers, ignored the fact that two of them asked me to make the change you're objecting to, and casually expect me to rewrite this a third time.

If I find a change that I think is detrimental to pandas then yes, I'm going to speak up about it. I do apologize if that means lost effort, but I care more about the project as a whole.

pilkibun added 4 commits July 25, 2019 18:21

BUG: do not broadcast result of transformations in transform

784e621

comment

4ad587e

doc

b6216af

doc

15597f5

jbrockmendel reviewed Jul 26, 2019

View reviewed changes

doc/source/whatsnew/v1.0.0.rst Outdated Show resolved Hide resolved

jbrockmendel reviewed Jul 26, 2019

View reviewed changes

pandas/core/groupby/generic.py Outdated Show resolved Hide resolved

pilkibun added 2 commits July 25, 2019 20:00

doc

0f77cbf

use _values

880b2d8

WillAyd added the Groupby label Jul 26, 2019

jreback requested changes Jul 26, 2019

View reviewed changes

doc/source/whatsnew/v1.0.0.rst Outdated Show resolved Hide resolved

pilkibun added 4 commits July 26, 2019 10:57

CLN: reserve g.transform(name) exclusively for broadcasting aggregations

36a15f2

rank is not an agg

0c77a37

fix test

09703f8

fix tests

8eeb01a

pilkibun added 2 commits July 26, 2019 11:35

checks

c0a71ce

whatsnew

2ce2bb7

checks

d4bafef

ghost changed the title ~~BUG: do not broadcast result of transformations in groupby.transform~~ CLN: groupby.transform(name) validates name is an aggregation Jul 26, 2019

ghost changed the title ~~CLN: groupby.transform(name) validates name is an aggregation~~ BUG: groupby.transform(name) validates name is an aggregation Jul 26, 2019

TomAugspurger reviewed Jul 26, 2019

View reviewed changes

doc/source/whatsnew/v1.0.0.rst Outdated Show resolved Hide resolved

doc/source/whatsnew/v1.0.0.rst Outdated Show resolved Hide resolved

whatsnew

57e2122

Update transform docstring

f942e55

jreback requested changes Jul 26, 2019

View reviewed changes

ghost closed this Jul 29, 2019

ghost deleted the groupby_transform_cleanups2 branch July 29, 2019 18:12

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: groupby.transform(name) validates name is an aggregation #27597

BUG: groupby.transform(name) validates name is an aggregation #27597

ghost commented Jul 25, 2019 •

edited by jreback

Loading

WillAyd commented Jul 26, 2019

ghost commented Jul 26, 2019 •

edited by ghost

Loading

WillAyd commented Jul 26, 2019

ghost commented Jul 26, 2019

ghost commented Jul 26, 2019 •

edited by ghost

Loading

WillAyd commented Jul 26, 2019

jreback left a comment

pep8speaks commented Jul 26, 2019 •

edited

Loading

ghost commented Jul 26, 2019 •

edited by ghost

Loading

TomAugspurger left a comment

ghost commented Jul 26, 2019 •

edited by ghost

Loading

TomAugspurger commented Jul 26, 2019

ghost commented Jul 26, 2019

TomAugspurger commented Jul 26, 2019 via email

ghost commented Jul 26, 2019 •

edited by ghost

Loading

TomAugspurger commented Jul 26, 2019

ghost commented Jul 26, 2019

jreback left a comment

jreback Jul 26, 2019

jreback Jul 26, 2019

jreback Jul 26, 2019

jreback Jul 26, 2019

jreback Jul 26, 2019

TomAugspurger commented Jul 26, 2019

ghost commented Jul 29, 2019

TomAugspurger commented Jul 29, 2019


		.. code-block:: ipython

		In [1]: g.transform('ers >= Decepticons')

BUG: groupby.transform(name) validates name is an aggregation #27597

BUG: groupby.transform(name) validates name is an aggregation #27597

Conversation

ghost commented Jul 25, 2019 • edited by jreback Loading

WillAyd commented Jul 26, 2019

ghost commented Jul 26, 2019 • edited by ghost Loading

WillAyd commented Jul 26, 2019

ghost commented Jul 26, 2019

ghost commented Jul 26, 2019 • edited by ghost Loading

WillAyd commented Jul 26, 2019

jreback left a comment

Choose a reason for hiding this comment

pep8speaks commented Jul 26, 2019 • edited Loading

Comment last updated at 2019-07-26 20:09:14 UTC

ghost commented Jul 26, 2019 • edited by ghost Loading

TomAugspurger left a comment

Choose a reason for hiding this comment

ghost commented Jul 26, 2019 • edited by ghost Loading

TomAugspurger commented Jul 26, 2019

ghost commented Jul 26, 2019

TomAugspurger commented Jul 26, 2019 via email

ghost commented Jul 26, 2019 • edited by ghost Loading

TomAugspurger commented Jul 26, 2019

ghost commented Jul 26, 2019

jreback left a comment

Choose a reason for hiding this comment

jreback Jul 26, 2019

Choose a reason for hiding this comment

jreback Jul 26, 2019

Choose a reason for hiding this comment

jreback Jul 26, 2019

Choose a reason for hiding this comment

jreback Jul 26, 2019

Choose a reason for hiding this comment

jreback Jul 26, 2019

Choose a reason for hiding this comment

TomAugspurger commented Jul 26, 2019

ghost commented Jul 29, 2019

TomAugspurger commented Jul 29, 2019

ghost commented Jul 25, 2019 •

edited by jreback

Loading

ghost commented Jul 26, 2019 •

edited by ghost

Loading

ghost commented Jul 26, 2019 •

edited by ghost

Loading

pep8speaks commented Jul 26, 2019 •

edited

Loading

ghost commented Jul 26, 2019 •

edited by ghost

Loading

ghost commented Jul 26, 2019 •

edited by ghost

Loading

ghost commented Jul 26, 2019 •

edited by ghost

Loading