CLN: aggregation.transform #36478

rhshadrach · 2020-09-19T17:21:59Z

closes CLN: Followup to 35964 #36330
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Followup to #35964

Moved whatsnew to reshaping
Use is_list_like/is_dict_like in aggregate.transform
Broke out dict-like and str/callable computations to own functions to split up the transform function
Added/refined typing

pandas/core/aggregation.py

jreback · 2020-09-19T20:01:18Z

pandas/core/aggregation.py

+
+    # combine results
+    if len(results) == 0:
+        raise ValueError("Transform function failed")


is this tested? is this new?

i think you should just let concat work and bubble up if needed

Not new, this is hit in tests.frame.apply.test_frame_transform.test_transform_bad_dtype. Removing these two lines results in the ValueError "No objects to concatenate" rather than "Transform function failed". I'm okay with either, slight preference for the current behavior (which is the same as this PR). Let me know if you prefer "No objects to concatenate" and can change.

hmm yeah I think we should change this, maybe to something a bit more userfriendly like
the input transform did not contain any transform functions

Are you thinking of the case where an aggregator is used? If we detect the function not transforming, we'll raise here:

try: results[name] = transform(colg, how, 0, *args, **kwargs) except Exception as e: if str(e) == "Function did not transform": raise e

before we get to this point. The raising of the code highlighted above should only occur if all the key/value pairs of the dictionary entirely failed - e.g. trying to take a product of strings or lambda x: raise ValueError.

ok, no this is the case of an empty list or dict right? (of functions that are supplied for aggregation), IOW its user visible

Ah, I see! I did not realize this line would be the one that raised for an empty list or dict. I've added that check before getting to this section of the code with the message "no results" (this is the message from 1.1 - but can change), along with tests for this.

pandas/core/aggregation.py

rhshadrach · 2020-09-20T15:44:47Z

/azp run

azure-pipelines · 2020-09-20T15:44:56Z

Azure Pipelines successfully started running 1 pipeline(s).

rhshadrach · 2020-09-21T20:20:53Z

/azp run

azure-pipelines · 2020-09-21T20:21:02Z

Azure Pipelines successfully started running 1 pipeline(s).

rhshadrach · 2020-09-21T21:12:27Z

@jreback Tests added and pass; responses to your questions are above.

jreback

minor comments, ping on green.

jreback · 2020-09-21T23:45:31Z

pandas/core/aggregation.py

+
+    # combine results
+    if len(results) == 0:
+        raise ValueError("Transform function failed")


hmm yeah I think we should change this, maybe to something a bit more userfriendly like
the input transform did not contain any transform functions

jreback · 2020-09-21T23:46:05Z

pandas/tests/frame/apply/test_frame_transform.py

@@ -56,15 +62,16 @@ def test_transform_list(axis, float_frame, ops, names):
    tm.assert_frame_equal(result, expected)


-def test_transform_dict(axis, float_frame):
+@pytest.mark.parametrize("argtype", [dict, Series])


use box rather than argtype

jreback · 2020-09-21T23:46:20Z

pandas/tests/series/apply/test_series_transform.py

@@ -45,12 +51,13 @@ def test_transform_list(string_series, ops, names):
        tm.assert_frame_equal(result, expected)


-def test_transform_dict(string_series):
+@pytest.mark.parametrize("argtype", [dict, Series])


…ansform_cleanup � Conflicts: � pandas/core/aggregation.py

…ansform_cleanup � Conflicts: � doc/source/whatsnew/v1.2.0.rst

ivanovmg · 2020-09-24T19:27:07Z

pandas/core/aggregation.py

+        # Check for missing columns on a frame
+        cols = sorted(set(func.keys()) - set(obj.columns))
+        if len(cols) > 0:
+            raise SpecificationError(f"Column(s) {cols} do not exist")


Better rename cols to missing_cols. Even better if extract function _check_missing_columns(obj, func).

ivanovmg · 2020-09-24T19:28:45Z

pandas/core/aggregation.py

+    if len(func) == 0:
+        raise ValueError("no results")


Not that it matters much, but probably it will be better to move this check on top of the function, before any iteration over func.

Agreed, thanks.

ivanovmg · 2020-09-24T19:40:41Z

pandas/core/aggregation.py

+        except Exception as err:
+            if str(err) == "Function did not transform" or str(err) == "no results":
+                raise err


Would it be reasonable to create a named exception rather than testing the error message match?

ivanovmg · 2020-09-24T19:42:44Z

pandas/core/aggregation.py

+    """
+    from pandas.core.reshape.concat import concat
+
+    if obj.ndim != 1:


Maybe extract method _is_series(obj). I noticed that this dimensions check is run in another function, but there is a local variable is_series.

rhshadrach · 2020-09-24T19:56:10Z

Thanks for reviewing @ivanovmg. I don't necessarily disagree with your suggestions, but I think they are outside the scope of this PR.

rhshadrach · 2020-09-25T01:58:56Z

@jreback The diff on this is getting to be too complex (and honestly started that way). I've opened #36618 as a precursor.

…ansform_cleanup � Conflicts: � pandas/core/aggregation.py

rhshadrach · 2020-10-01T21:08:11Z

@jreback - This is ready for another review. Failure is unrelated.

jreback

also pls rebase

jreback · 2020-10-02T22:21:57Z

pandas/core/aggregation.py

    """
    Compute transform in the case of a dict-like func
    """
    from pandas.core.reshape.concat import concat

+    if len(func) == 0:
+        raise ValueError("no results")


where is this raised to? is it user facing?

Yes - this is raised directly to user. The error type and message agrees with 1.1.x, however it wasn't tested until this PR. Can change/improve if desired.

ok i think this needs to be more explict on what is happening, e.g. an empty function specification was provided. otherwise lgtm.

…ansform_cleanup

jreback · 2020-10-06T22:47:14Z

thanks @rhshadrach very nice

CLN: Refactor and typing in aggregate.transform

72512b7

rhshadrach added Apply Apply, Aggregate, Transform, Map Clean labels Sep 19, 2020

jreback requested changes Sep 19, 2020

View reviewed changes

Added tests for list-like(ndarray) and dict-like (Series)

061f9c6

jreback requested changes Sep 21, 2020

View reviewed changes

rhshadrach added 2 commits September 22, 2020 17:13

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

409fda2

…ansform_cleanup � Conflicts: � pandas/core/aggregation.py

argtype -> box

2b2547f

jreback added this to the 1.2 milestone Sep 22, 2020

rhshadrach added 4 commits September 23, 2020 16:52

Added case and tests for empty list-like or dict-like

431488c

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

9e77105

…ansform_cleanup � Conflicts: � doc/source/whatsnew/v1.2.0.rst

Merge cleanup

3e72b80

Merge cleanup

506e804

ivanovmg reviewed Sep 24, 2020

View reviewed changes

Moved length check

d0193c9

rhshadrach mentioned this pull request Sep 25, 2020

CLN: Break up aggregate.transform #36618

Merged

5 tasks

rhshadrach added 2 commits September 28, 2020 17:31

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

e53b389

…ansform_cleanup � Conflicts: � pandas/core/aggregation.py

cast

ea2ad3d

jreback requested changes Oct 2, 2020

View reviewed changes

rhshadrach added 3 commits October 4, 2020 10:42

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

e5a7f92

…ansform_cleanup

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

744f1fb

…ansform_cleanup

Changed error message

7066f80

jreback approved these changes Oct 6, 2020

View reviewed changes

jreback merged commit ebd9906 into pandas-dev:master Oct 6, 2020

rhshadrach deleted the transform_cleanup branch October 11, 2020 13:22

kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020

CLN: aggregation.transform (pandas-dev#36478)

d6de4e8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLN: aggregation.transform #36478

CLN: aggregation.transform #36478

rhshadrach commented Sep 19, 2020

jreback Sep 19, 2020

jreback Sep 19, 2020

rhshadrach Sep 20, 2020 •

edited

Loading

jreback Sep 21, 2020

rhshadrach Sep 22, 2020

jreback Sep 22, 2020

rhshadrach Sep 23, 2020

rhshadrach commented Sep 20, 2020

azure-pipelines bot commented Sep 20, 2020

rhshadrach commented Sep 21, 2020

azure-pipelines bot commented Sep 21, 2020

rhshadrach commented Sep 21, 2020

jreback left a comment

jreback Sep 21, 2020

jreback Sep 21, 2020

jreback Sep 21, 2020

ivanovmg Sep 24, 2020

ivanovmg Sep 24, 2020

rhshadrach Sep 24, 2020

ivanovmg Sep 24, 2020

ivanovmg Sep 24, 2020

rhshadrach commented Sep 24, 2020

rhshadrach commented Sep 25, 2020

rhshadrach commented Oct 1, 2020

jreback left a comment

jreback Oct 2, 2020

rhshadrach Oct 4, 2020 •

edited

Loading

jreback Oct 6, 2020

jreback commented Oct 6, 2020

CLN: aggregation.transform #36478

CLN: aggregation.transform #36478

Conversation

rhshadrach commented Sep 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhshadrach Sep 20, 2020 • 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

rhshadrach commented Sep 20, 2020

azure-pipelines bot commented Sep 20, 2020

rhshadrach commented Sep 21, 2020

azure-pipelines bot commented Sep 21, 2020

rhshadrach commented Sep 21, 2020

jreback 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

rhshadrach commented Sep 24, 2020

rhshadrach commented Sep 25, 2020

rhshadrach commented Oct 1, 2020

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhshadrach Oct 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Oct 6, 2020

rhshadrach Sep 20, 2020 •

edited

Loading

rhshadrach Oct 4, 2020 •

edited

Loading