BUG/CLN: Decouple Series/DataFrame.transform #35964

rhshadrach · 2020-08-28T20:33:13Z

closes BUG/QST: Series.transform with a dictionary #35811
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

First step toward #35725. Currently transform just calls aggregate, and so if we are to forbid aggregate from transforming, these need to be decoupled. Other than the bugfix (#35811), the only other behavioral change is in the error messages.

Assuming the bugfix #35811 is the correct behavior, docs/whatsnew also needs to be updated.

I wasn't sure if tests should be marked with #35725 or perhaps this PR #. Any guidance here?

rhshadrach · 2020-08-28T20:36:14Z

pandas/tests/series/apply/test_series_apply.py

@@ -222,7 +223,7 @@ def test_transform(self, string_series):
            expected.columns = ["sqrt"]
            tm.assert_frame_equal(result, expected)

-            result = string_series.transform([np.sqrt])
+            result = string_series.apply([np.sqrt])


The line transform([np.sqrt]) occurs 5 lines up. I believe the original intention was to test apply here.

jreback · 2020-09-02T17:43:12Z

I wasn't sure if tests should be marked with #35725 or perhaps this PR #. Any guidance here?
either is fine

jreback

some comments

pandas/core/generic.py

pandas/core/frame.py

pandas/core/series.py

pandas/tests/frame/apply/test_frame_apply.py

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

rhshadrach · 2020-09-05T12:57:48Z

@jreback Thanks for the review. Code has been reorganized and ready for another. For the comment about not adding things to the core files - I think the idea behind this that we should be distributing the implementation of methods in e.g. NDFrame into various submodules like aggregate instead of having one monolithic generic.py. Is that correct?

jreback

looking really good, a few comments

jreback · 2020-09-05T15:41:24Z

pandas/core/aggregation.py

+
+def transform(
+    obj: FrameOrSeries,
+    func: Union[str, List, Dict, Callable],


do we have an alias for this anywhere?

pandas/core/aggregation.py

jreback · 2020-09-05T15:42:21Z

pandas/core/aggregation.py

+        # combine results
+        if len(results) == 0:
+            raise ValueError("Transform function failed")
+        from pandas.core.reshape.concat import concat


put this at the top of the function

pandas/core/aggregation.py

jreback · 2020-09-05T15:43:55Z

pandas/core/generic.py

-            raise ValueError("transforms cannot produce aggregated results")
-
-        return result
+        raise NotImplementedError


can we remove this? or is the doc-string used?

maybe just move the doc-string to shared_docs?

jreback · 2020-09-05T15:45:25Z

pandas/tests/frame/apply/test_frame_transform.py

+        expected = f_sqrt
+        tm.assert_frame_equal(result, expected)
+
+        # list-like


i would actually split these to separate tests easier to grok

jreback · 2020-09-05T15:46:02Z

pandas/tests/frame/apply/test_frame_transform.py

+
+    # Determine which columns op will work on
+    columns = []
+    for column in df:


hmm can we explicit list the exluded ones rather than this?

jreback · 2020-09-05T15:46:43Z

pandas/tests/frame/common.py

@@ -29,3 +32,22 @@ def _check_mixed_int(df, dtype=None):
        assert df.dtypes["C"] == dtypes["C"]
    if dtypes.get("D"):
        assert df.dtypes["D"] == dtypes["D"]
+
+
+def zip_frames(frames, axis=1):


can you type args and output

jreback · 2020-09-05T15:46:58Z

pandas/tests/series/apply/test_series_transform.py

+        expected = f_sqrt.copy()
+        tm.assert_series_equal(result, expected)
+
+        # list-like


can you split this one as well

…ansform_refactor � Conflicts: � pandas/core/frame.py � pandas/tests/frame/apply/test_frame_transform.py � pandas/tests/frame/common.py � pandas/tests/series/apply/test_series_apply.py � pandas/tests/series/apply/test_series_transform.py

…ansform_refactor

rhshadrach · 2020-09-12T16:52:27Z

@jreback changes made and green. In fixing the one test I found cumcount doesn't exist for DataFrames/Series (which, after thinking a bit, makes sense). This prompted me to add in tests for (almost) all the transformation kernels.

jreback

some minor comments, but can address in a followon. thanks!

jreback · 2020-09-12T21:33:14Z

doc/source/whatsnew/v1.2.0.rst

@@ -342,6 +342,7 @@ Other
 ^^^^^
 - Bug in :meth:`DataFrame.replace` and :meth:`Series.replace` incorrectly raising ``AssertionError`` instead of ``ValueError`` when invalid parameter combinations are passed (:issue:`36045`)
 - Bug in :meth:`DataFrame.replace` and :meth:`Series.replace` with numeric values and string ``to_replace`` (:issue:`34789`)
+- Bug in :meth:`Series.transform` would give incorrect results or raise when the argument ``func`` was dictionary (:issue:`35811`)


move to reshaping section (generally we want almost nothing in Other)

jreback · 2020-09-12T21:34:02Z

pandas/core/aggregation.py

+        assert not is_series
+        return transform(obj.T, func, 0, *args, **kwargs).T
+
+    if isinstance(func, list):


we should probably use is_list_like here

jreback · 2020-09-12T21:34:12Z

pandas/core/aggregation.py

+        else:
+            func = {col: func for col in obj}
+
+    if isinstance(func, dict):


should probably use is_dict_like here

jreback · 2020-09-12T21:34:26Z

pandas/core/aggregation.py

+            # GH 15931 - deprecation of renaming keys
+            raise SpecificationError("nested renamer is not supported")
+
+        results = {}


can you type results

jreback · 2020-09-12T21:36:56Z

thanks @rhshadrach

jbrockmendel · 2020-12-03T21:40:23Z

pandas/tests/series/apply/test_series_transform.py

+    # GH 35964
+    s = Series(3 * [object])  # Series that will fail on most transforms
+    if op in ("backfill", "shift", "pad", "bfill", "ffill"):
+        pytest.xfail("Transform function works on any datatype")


is the issue here that the behavior is wrong or that the test isn't applicable to these cases?

Behavior is correct, the test is not applicable. This test is about behavior when transform fails and these ops are expected to never fail.

CLN: Decouple Series/DataFrame.transform

7b6ab94

rhshadrach added Apply Apply, Aggregate, Transform, Map Bug Clean labels Aug 28, 2020

rhshadrach commented Aug 28, 2020

View reviewed changes

jreback added this to the 1.2 milestone Sep 2, 2020

jreback requested changes Sep 2, 2020

View reviewed changes

rhshadrach added 4 commits September 4, 2020 22:17

Broke out tests to separate modules; moved code to aggregation

04c1238

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

052df6e

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

Fixed type-hints

7b13811

Fixed tests - added context manager for np errors

133bfaa

Added PR # to tests

a5d4a19

jreback requested changes Sep 5, 2020

View reviewed changes

rhshadrach mentioned this pull request Sep 5, 2020

CLN: Separate transform tests #36146

Merged

5 tasks

rhshadrach added 7 commits September 9, 2020 20:39

Adjustments and cleanups

8454d91

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

c37ef68

…ansform_refactor

Docs and linting fixes

9eee0cb

Added and improved some tests

cf4f80b

whatsnew

69e6807

whatsnew

f66a806

jreback approved these changes Sep 12, 2020

View reviewed changes

jreback merged commit ab5b38d into pandas-dev:master Sep 12, 2020

rhshadrach deleted the transform_refactor branch September 13, 2020 11:58

rhshadrach mentioned this pull request Sep 19, 2020

CLN: aggregation.transform #36478

Merged

5 tasks

topper-123 mentioned this pull request Oct 25, 2020

TST: Different tests were collected between gw0 and gw1 #37382

Merged

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

BUG/CLN: Decouple Series/DataFrame.transform (pandas-dev#35964)

621ca9f

jbrockmendel reviewed Dec 3, 2020

View reviewed changes

jorisvandenbossche mentioned this pull request Feb 7, 2021

BUG: ValueError on ".transform" method applied to an empty DataFrame #39636

Closed

3 tasks

simonjayhawkins mentioned this pull request Feb 27, 2021

BUG: DataFrame/Series.transform with list and non-list dict values #40018

Closed

rhshadrach mentioned this pull request Mar 5, 2021

DEPR: Partial failure in Series.transform and DataFrame.transform #40238

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG/CLN: Decouple Series/DataFrame.transform #35964

BUG/CLN: Decouple Series/DataFrame.transform #35964

rhshadrach commented Aug 28, 2020 •

edited

Loading

rhshadrach Aug 28, 2020

jreback commented Sep 2, 2020

jreback left a comment

rhshadrach commented Sep 5, 2020

jreback left a comment

jreback Sep 5, 2020

jreback Sep 5, 2020

jreback Sep 5, 2020

jreback Sep 5, 2020

jreback Sep 5, 2020

jreback Sep 5, 2020

jreback Sep 5, 2020

jreback Sep 5, 2020

rhshadrach commented Sep 12, 2020

jreback left a comment

jreback Sep 12, 2020

jreback Sep 12, 2020

jreback Sep 12, 2020

jreback Sep 12, 2020

jreback commented Sep 12, 2020

jbrockmendel Dec 3, 2020

rhshadrach Dec 3, 2020

BUG/CLN: Decouple Series/DataFrame.transform #35964

BUG/CLN: Decouple Series/DataFrame.transform #35964

Conversation

rhshadrach commented Aug 28, 2020 • edited Loading

Choose a reason for hiding this comment

jreback commented Sep 2, 2020

jreback left a comment

Choose a reason for hiding this comment

rhshadrach commented Sep 5, 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 12, 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

jreback commented Sep 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhshadrach commented Aug 28, 2020 •

edited

Loading