Convert DataFrame.rename to keyword only; simplify axis validation #29140

WillAyd · 2019-10-21T21:11:20Z

closes DataFrame.rename only validates column arguments #29136 and ref API break with DataFrame.rename #17963
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

I think we can greatly simplify core if we update our assumptions around the axes of dimensions we deal with. I came to this change trying to get rid of NDFrame._construct_axes_from_arguments which appears to do some argument mutation that isn't very clear. This was one of the few methods that called that, while also calling validate_axis_style_args which is guilty of the same kind of mutation

By being more explicit about the signature and converting this to keyword only arguments (which a FutureWarning detailed anyway) we can simplify the code and I think make more readable

cc @TomAugspurger

pep8speaks · 2019-10-21T21:11:33Z

Hello @WillAyd! 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 2020-01-09 17:06:47 UTC

WillAyd · 2019-10-21T21:11:44Z

pandas/core/frame.py

 [("copy", True), ("inplace", False), ("level", None), ("errors", "ignore")],
 )
- def rename(self, *args, **kwargs):
+ def rename(


Change here was to be explicit about what is accepted

+1 to being explicit. The only reason we had *args, **kwargs in the first place was to support the ambiguous case.

WillAyd · 2019-10-21T21:14:08Z

pandas/core/generic.py

+ raise TypeError("Cannot specify both 'mapper' and any of 'index' or 'columns'")
+ else:
+ # use the mapper argument
+ if axis in {1, "columns"}:


We accept 3 arguments, mapper, index and columns. mapper can conflict with index or columns if either is provided, which the checks before this take care of. At this point if there aren't any conflicts, I think easiest to just combine the object

Note that this is arguably a little too strict, as this is no longer valid:

df.rename({"key": "val"}, axis=1, index={"key2": "val2"})

But I don't think there is a lot to gain in supporting that given the variety of other options available here

Note that this is arguably a little too strict, as this is no longer valid:

I don't recall if that case was originally discussed, but I think it's OK to break it.

WillAyd · 2019-10-21T21:15:14Z

pandas/core/generic.py

- for axis in range(self._AXIS_LEN):
- v = axes.get(self._AXIS_NAMES[axis])
- if v is None:
+ for axis_no, replacements in enumerate((index, columns)):


Ultimately I am trying to get rid of all of the internal _AXIS_* functions and mappings in core.generic, which seem like relics of the panel days

If we buy into just having index and columns being the potential axes in that order things can be greatly simplified

WillAyd · 2019-10-21T21:15:55Z

pandas/core/series.py

 )

- def rename(self, index=None, **kwargs):
+ def rename(


The Series method doesn't completely align with the DataFrame method, but I think this is an easy way to go about it while maintain backwards compat

What's the value in accepting axis here, if we don't also have mapper and columns?

I don't see an easy way to accept mapper. That's essentially fulfilled by index.

So maybe my question is: should we also accept columns=? (Not arguing for this btw. I don't know what's best here).

Do we validate axis at all, or just ignore?

Here axis gets ignored. Looks like this was implemented as part of #18589

So maybe my question is: should we also accept columns=? (Not arguing for this btw. I don't know what's best here).

Yea I'm not sure myself. I was thinking ideally it would have been great to align the signatures better but for where we are that would cause some API churn. So I guess best to leave unless there's a ringing need to do something here (?)

WillAyd · 2019-10-21T21:16:28Z

pandas/tests/frame/test_alter_axes.py

 result = result.reindex(columns=df.columns)
 tm.assert_frame_equal(result, df)

- def test_ambiguous_warns(self):


These use cases are now covered by the test_rename_positional_raises test case above, so deleted as duplicative

WillAyd · 2019-10-21T21:18:00Z

pandas/tests/series/test_alter_axes.py

 s.rename({}, axis="index")
- with pytest.raises(ValueError, match="No axis named 5"):
- s.rename({}, axis=5)
+ # TODO: clean up shared index validation


Commented this out for now as I didn't think it was hugely critical and seemed somewhat orthoganal to the test, but can add back in if there is a huge objection.

Ideally though we would have a centralized place for index validation that doesn't mutate the arguments of the caller.

Fine to move this elsewhere. I do have a slight preference for not completely ignoring arguments that are there just for compatibility, but I don't know what our general rule is.

Yea let me take another look I think should be able to keep this. Both self._get_axis_number and self._get_axis_name have some kind of validation in generic.

I don't know if there is one consistent way we do this right now, but want to march towards that

I think the preference should be that the axis parameter should be validated if specified on a Series for consistency with most Series methods that accept axis (e.g. fillna) and all DataFrame methods that accept axis.

Both self._get_axis_number and self._get_axis_name have some kind of validation in generic.

normally you can just pass axis to super and it'll get validated? in this case there is also the call to self._set_name (cause of #28920). so probably will need the call to self._get_axis_number.

WillAyd · 2019-10-21T21:20:11Z

pandas/core/frame.py

 2 2 5
 4 3 6
 """
- axes = validate_axis_style_args(self, args, kwargs, "mapper", "rename")


Side note - there is only one other use now of this validate_axis_style_args function (see DataFrame.reindex). The intent here is good, but I think the fact that it mutates the args and kwargs being passed it makes it very difficult to reason about. As a follow up to this would like to replace entirely with something simple that just operates on the axis argument

Is there anything blocking a similar PR for .reindex?

As a follow up to this would like to replace entirely with something simple that just operates on the axis argument

Is this referring to DataFrame.reindex, or validate_axis_style_args? Ideally we can completely remove validate_axis_style_args.

I didn't look before because I was trying to keep this focused, but it might not be too much extra effort. I'll take a look and if it gets out of hand let you know

jbrockmendel · 2019-10-21T21:32:23Z

pandas/core/frame.py

+ Union[Mapping[Hashable, Hashable], Callable[[Hashable], Hashable]]
+ ] = None,
+ columns: Optional[
+ Union[Mapping[Hashable, Hashable], Callable[[Hashable], Hashable]]


is it worth making a name for this in _typing?

Was thinking the same thing. Any suggestions? LabelReplacement?

TomAugspurger · 2019-10-21T22:02:20Z

pandas/core/frame.py

 [("copy", True), ("inplace", False), ("level", None), ("errors", "ignore")],
 )
- def rename(self, *args, **kwargs):
+ def rename(


+1 to being explicit. The only reason we had *args, **kwargs in the first place was to support the ambiguous case.

TomAugspurger · 2019-10-21T22:04:10Z

pandas/core/frame.py

 2 2 5
 4 3 6
 """
- axes = validate_axis_style_args(self, args, kwargs, "mapper", "rename")


Is there anything blocking a similar PR for .reindex?

As a follow up to this would like to replace entirely with something simple that just operates on the axis argument

Is this referring to DataFrame.reindex, or validate_axis_style_args? Ideally we can completely remove validate_axis_style_args.

TomAugspurger · 2019-10-21T22:06:46Z

pandas/core/generic.py

+ raise TypeError("Cannot specify both 'mapper' and any of 'index' or 'columns'")
+ else:
+ # use the mapper argument
+ if axis in {1, "columns"}:


Note that this is arguably a little too strict, as this is no longer valid:

I don't recall if that case was originally discussed, but I think it's OK to break it.

TomAugspurger · 2019-10-21T22:10:05Z

pandas/core/series.py

 )

- def rename(self, index=None, **kwargs):
+ def rename(


What's the value in accepting axis here, if we don't also have mapper and columns?

I don't see an easy way to accept mapper. That's essentially fulfilled by index.

So maybe my question is: should we also accept columns=? (Not arguing for this btw. I don't know what's best here).

TomAugspurger · 2019-10-21T22:10:19Z

pandas/core/series.py

 )

- def rename(self, index=None, **kwargs):
+ def rename(


Do we validate axis at all, or just ignore?

TomAugspurger · 2019-10-21T22:12:32Z

pandas/tests/frame/test_alter_axes.py

 # https://github.com/pandas-dev/pandas/issues/12392
 df = DataFrame({"a": [1, 2], "b": [1, 2]}, index=["X", "Y"])
- result = df.rename(str.lower, columns=str.upper)
+ result = df.rename(index=str.lower, columns=str.upper)


So did this break? If so, is it solely because of the keyword only change to index and columns?

If so, I might prefer a proper deprecation warning for this change... But could be convinced otherwise, since I'd consider the old code bad style.

This would be related to https://github.com/pandas-dev/pandas/pull/29140/files#r337242915

This is a little tough to support without keeping the ambiguity, and it doesn't really work outside of this one niche application.

For instance, if you tried to apply str.lower to the columns and str.upper to the index you would get the following error:

>>> df.rename(str.lower, index=str.upper, axis=1) TypeError: Cannot specify both 'axis' and any of 'index' or 'columns'.

Even explicitly stating the axis in this case would raise an error:

>>> df.rename(str.lower, columns=str.upper, axis=0) TypeError: Cannot specify both 'axis' and any of 'index' or 'columns'.

So we could still support this but it would take some gymnastics and not sure it makes things clearer, though open to the thoughts of others.

Maybe can be clarified further in the whatsnew to mitigate confusion?

TomAugspurger · 2019-10-21T22:13:27Z

pandas/tests/series/test_alter_axes.py

 s.rename({}, axis="index")
- with pytest.raises(ValueError, match="No axis named 5"):
- s.rename({}, axis=5)
+ # TODO: clean up shared index validation


Fine to move this elsewhere. I do have a slight preference for not completely ignoring arguments that are there just for compatibility, but I don't know what our general rule is.

WillAyd · 2019-11-07T01:40:11Z

Closing to clear queue; will pick back up when I get a little more free time

jbrockmendel · 2019-12-26T19:21:17Z

Re-open? This looks like a nice bugfix

WillAyd · 2019-12-27T14:54:07Z

Yea sure I'll try to take another look

WillAyd · 2020-01-09T00:21:02Z

pandas/core/generic.py


 mapper = {self._info_axis_name: f}
- return self.rename(**mapper)
+ return self.rename(**mapper) # type: ignore


Couldn't seem to get dict unpacking to place nicely with mypy. @simonjayhawkins

jreback

lgtm. @TomAugspurger

TomAugspurger

Glancing through, https://github.com/pandas-dev/pandas/pull/29140/files#r337263499 is I think my main outstanding concern.

df.reanme(str.lower, columns=str.upper)

will I think be broken, which is unfortunate. I think I'm OK with it if others are though, since we'd recommend .rename(index=str.lower, columns=str.upper) anyway.

doc/source/whatsnew/v1.0.0.rst

pandas/_typing.py

jreback · 2020-01-09T13:17:05Z

@WillAyd conflict and some comments

pandas/core/frame.py

pandas/core/generic.py

pandas/core/frame.py

pandas/core/generic.py

pandas/core/series.py

WillAyd · 2020-01-09T16:20:05Z

@TomAugspurger yea that is the one case where this would break things, but the flip side is that also invites ambiguous uses like df.rename(str.lower, index=str.upper) (on master silently discards index) so not sure there's a perfect solution

TomAugspurger · 2020-01-09T16:22:23Z

Mmm I'd forgotten that case. So +1 to whatever the group thinks here :)

pandas/_typing.py

TomAugspurger · 2020-01-09T19:24:08Z

Planning to merge this in an hour, just before tagging the RC.

TomAugspurger · 2020-01-09T21:04:31Z

Thanks @WillAyd!

WillAyd added 5 commits October 21, 2019 12:56

Worked around frame edges

f09c6df

Fixed frame tests

66acc90

Fixed Series tests

177a440

whatsnew

b66e9e2

Cleaned up test cases

2415e46

WillAyd added the Clean label Oct 21, 2019

WillAyd commented Oct 21, 2019

View reviewed changes

style updates

296a9df

WillAyd mentioned this pull request Oct 21, 2019

Reduce Circular Imports with pandas.core.reshape.concat #29133

Closed

jbrockmendel reviewed Oct 21, 2019

View reviewed changes

TomAugspurger reviewed Oct 21, 2019

View reviewed changes

WillAyd closed this Nov 7, 2019

WillAyd reopened this Dec 27, 2019

WillAyd added 4 commits January 7, 2020 16:07

Merge remote-tracking branch 'upstream/master' into fix-rename

6ba8518

post merge fixes

4128780

style fixups

2776c7d

fixups

8e20fa4

WillAyd force-pushed the fix-rename branch from a6e5b91 to 8e20fa4 Compare January 8, 2020 00:26

WillAyd added 3 commits January 8, 2020 16:12

Added okexcept

cca8eed

Black fixup

1f8e1cf

mypy fixups

4dc8bbe

WillAyd commented Jan 9, 2020

View reviewed changes

flake8

1b261f8

jreback approved these changes Jan 9, 2020

View reviewed changes

jreback added this to the 1.0 milestone Jan 9, 2020

TomAugspurger reviewed Jan 9, 2020

View reviewed changes

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

simonjayhawkins reviewed Jan 9, 2020

View reviewed changes

pandas/_typing.py Outdated Show resolved Hide resolved

simonjayhawkins reviewed Jan 9, 2020

View reviewed changes

pandas/core/frame.py Outdated Show resolved Hide resolved

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

pandas/core/frame.py Outdated Show resolved Hide resolved

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

simonjayhawkins reviewed Jan 9, 2020

View reviewed changes

pandas/core/series.py Outdated Show resolved Hide resolved

WillAyd added 3 commits January 9, 2020 07:34

Merge remote-tracking branch 'upstream/master' into fix-rename

b5e54bd

Comments addressed

ab1ee2e

minor fixups

af0b7c5

simonjayhawkins reviewed Jan 9, 2020

View reviewed changes

pandas/_typing.py Outdated Show resolved Hide resolved

bound typevar to self

d5d812c

TomAugspurger mentioned this pull request Jan 9, 2020

RLS: 1.0.0 #27492

Closed

TomAugspurger merged commit df75ea6 into pandas-dev:master Jan 9, 2020

simonjayhawkins mentioned this pull request Jan 11, 2020

TYP: type up parts of series.py #30761

Merged

WillAyd deleted the fix-rename branch February 13, 2020 20:00

Uh oh!

Convert DataFrame.rename to keyword only; simplify axis validation #29140

Convert DataFrame.rename to keyword only; simplify axis validation #29140

Uh oh!

Conversation

WillAyd commented Oct 21, 2019

pep8speaks commented Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2020-01-09 17:06:47 UTC

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

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

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

Choose a reason for hiding this comment

WillAyd commented Nov 7, 2019

jbrockmendel commented Dec 26, 2019

WillAyd commented Dec 27, 2019

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

TomAugspurger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jreback commented Jan 9, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WillAyd commented Jan 9, 2020

TomAugspurger commented Jan 9, 2020

Uh oh!

TomAugspurger commented Jan 9, 2020

TomAugspurger commented Jan 9, 2020

Labels

6 participants

pep8speaks commented Oct 21, 2019 •

edited

Loading