BUG: Fixed NDFrame.transform('abs') #20800
Conversation
TomAugspurger
added
Regression
Codecov Report
@@ Coverage Diff @@
## master #20800 +/- ##
==========================================
- Coverage 91.85% 91.78% -0.07%
==========================================
Files 153 153
Lines 49310 49273 -37
==========================================
- Hits 45292 45224 -68
- Misses 4018 4049 +31
Continue to review full report at Codecov.
|
|
I'd also add fill and rank to your list. FWIW there's probably a lot of overlap with the groupby module and what it considers to be a transformation vs an aggregation. Wonder if we should keep all of these functions and their "application type" in a shared module for both DataFrame and GroupBy ops |
pandas/core/apply.py
Outdated
| @@ -111,7 +111,9 @@ def get_result(self): | |||
|
|
|||
| # string dispatch | |||
| if isinstance(self.f, compat.string_types): | |||
| self.kwds['axis'] = self.axis | |||
| if self.f not in {'abs'}: | |||
There was a problem hiding this comment.
NO, anytime 'hacky' work-around you need to re-think the patch.
There was a problem hiding this comment.
either you need signature introspection (in python 3, may not be possible in python 2), alternatively can catch this type of error, but should be much higher level (in .aggregate), and not here.
There was a problem hiding this comment.
Ugh, signature introspection seems hackier than hard-coding, no?
There was a problem hiding this comment.
I'll see how hard it is to get Py2 & 3 working by inspecting the signature.
|
This should be for 0.23. It's a regression and was included in the docs. |
pandas/core/apply.py
Outdated
| @@ -111,8 +111,11 @@ def get_result(self): | |||
|
|
|||
| # string dispatch | |||
| if isinstance(self.f, compat.string_types): | |||
| self.kwds['axis'] = self.axis | |||
| return getattr(self.obj, self.f)(*self.args, **self.kwds) | |||
| func = getattr(self.obj, self.f) | |||
There was a problem hiding this comment.
much better, and now IIRC we actually do something like this in groupby apply (for the same reason). can you add some comments here on what is going on
pandas/tests/frame/test_apply.py
Outdated
| @@ -880,6 +880,13 @@ def f(): | |||
| with np.errstate(all='ignore'): | |||
| df.agg({'A': ['abs', 'sum'], 'B': ['mean', 'max']}) | |||
|
|
|||
| def test_transform_abs_name(self): | |||
There was a problem hiding this comment.
do we need more tests? (for other funcs)
pandas/tests/frame/test_apply.py
Outdated
| result = df.transform('abs') | ||
| expected = pd.DataFrame({"A": [1, 2]}) | ||
| result = df.transform(method) | ||
| expected = operator.methodcaller(method)(df) |
There was a problem hiding this comment.
Learned something new today. Just out of curiosity, is this any different from doing getattr(df, method)()?
There was a problem hiding this comment.
in this case, no. methodcaller is nicer though when you need to bind parameters, but don't have the object yet (common in pytest fixtures).
pandas/tests/frame/test_apply.py
Outdated
| @@ -880,11 +881,14 @@ def f(): | |||
| with np.errstate(all='ignore'): | |||
| df.agg({'A': ['abs', 'sum'], 'B': ['mean', 'max']}) | |||
|
|
|||
| def test_transform_abs_name(self): | |||
| @pytest.mark.parametrize('method', [ | |||
| 'abs', 'shift', 'pct_change', 'cumsum', 'rank', | |||
There was a problem hiding this comment.
Splitting to #20821
Closes #19760
So this is a hacky workaround. I think we would ideally whitelist a set of valid NDFrame.transform methods, but I don't want to break things. What would that list include?