Rename signature fix
State: closed

- [ ] closes #17963
- [ ] tests added / passed
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

Sample usage:

```
In [3]: df = pd.DataFrame(index=['a', 'b'], columns=['A', 'B'])

In [4]: df.reindex(['a'], ['A'])
/Users/taugspurger/.virtualenvs/pandas-dev/bin/ipython:1: UserWarning: Intepreting call
        '.reindex(a, b)' as
        '.reindex(index=a, columns=b)'.
Use named arguments to remove any ambiguity.
  #!/Users/taugspurger/Envs/pandas-dev/bin/python3.6
Out[4]:
     A
a  NaN

In [5]: df.reindex(None, ['A'])
/Users/taugspurger/.virtualenvs/pandas-dev/bin/ipython:1: UserWarning: Intepreting call
        '.reindex(a, b)' as
        '.reindex(index=a, columns=b)'.
Use named arguments to remove any ambiguity.
  #!/Users/taugspurger/Envs/pandas-dev/bin/python3.6
Out[5]:
     A
a  NaN
b  NaN
```

I'd like to simplify this further if possible, things go messy :/ 
I'd also like to fix the function signature, which is doable on python3 at least.

Labels:
name: API Design

Comments:
Added signature re-writing for python3 only. AFAICT, there's not a simple way to do that for both python 2 and 3.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17966?src=pr&el=h1) Report
> Merging [#17966](https://codecov.io/gh/pandas-dev/pandas/pull/17966?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/7aeccd392f0c07ecd1c8d929a0d0312fd8151971?src=pr&el=desc) will **decrease** coverage by `0.03%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/17966/graphs/tree.svg?src=pr&width=650&token=eZ4WkYLtcO&height=150)](https://codecov.io/gh/pandas-dev/pandas/pull/17966?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #17966      +/-   ##
==========================================
- Coverage   91.26%   91.23%   -0.04%     
==========================================
  Files         163      163              
  Lines       50117    50165      +48     
==========================================
+ Hits        45739    45766      +27     
- Misses       4378     4399      +21
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.04% <100%> (-0.02%)` | :arrow_down: |
| #single | `40.33% <75.29%> (-0.04%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17966?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/panel.py](https://codecov.io/gh/pandas-dev/pandas/pull/17966?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcGFuZWwucHk=) | `97% <100%> (+0.04%)` | :arrow_up: |
| [pandas/core/panel4d.py](https://codecov.io/gh/pandas-dev/pandas/pull/17966?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcGFuZWw0ZC5weQ==) | `100% <100%> (ø)` | :arrow_up: |
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/17966?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `92.44% <100%> (-0.09%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17966?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.75% <100%> (-0.1%)` | :arrow_down: |
| [pandas/util/\_decorators.py](https://codecov.io/gh/pandas-dev/pandas/pull/17966?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvX2RlY29yYXRvcnMucHk=) | `80.7% <100%> (+2.7%)` | :arrow_up: |
| [pandas/util/\_validators.py](https://codecov.io/gh/pandas-dev/pandas/pull/17966?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvX3ZhbGlkYXRvcnMucHk=) | `96.34% <100%> (+2.59%)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/17966?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/plotting/\_converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/17966?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19jb252ZXJ0ZXIucHk=) | `63.38% <0%> (-1.82%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17966?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17966?src=pr&el=footer). Last update [7aeccd3...edcbb9a](https://codecov.io/gh/pandas-dev/pandas/pull/17966?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

In principle this is fine for me (anyhow, the solution to interpret it as positional index=, columns= but warning is good), but have the feeling it is more complex as needed? 
In the case the user passes positional arguments `index, columns`, this is captured as `labels=index, index=columns`, but specifying both `labels` as `index` is not valid? So normally we should raise for that case, but can't we catch this situation, reinterprete and do the warning as in this PR? Or am I missing something?
I’ll see if that works, it would be more maintainable. One drawback of that is having to treat a user explicitly passing labels=foo, index=bar the same as just passing two positional args. Currently we would raise on that, instead we would issue a warning.

________________________________
From: Joris Van den Bossche <notifications@github.com>
Sent: Tuesday, October 24, 2017 5:23:14 PM
To: pandas-dev/pandas
Cc: Tom Augspurger; Author
Subject: Re: [pandas-dev/pandas] [WIP]Rename signature fix (#17966)


In principle this is fine for me (anyhow, the solution to interpret it as positional index=, columns= but warning is good), but have the feeling it is more complex as needed?
In the case the user passes positional arguments index, columns, this is captured as labels=index, index=columns, but specifying both labels as index is not valid? So normally we should raise for that case, but can't we catch this situation, reinterprete and do the warning as in this PR? Or am I missing something?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<https://github.com/pandas-dev/pandas/pull/17966#issuecomment-339153618>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABQHIhWt100aiLR515jXWmupANBLJcERks5svmNSgaJpZM4QE7Qm>.

Ah, yes, that's a case I didn't think of. Personally, I would go for accepting that minor disadvantage to have the simpler code. The question then maybe is if we would make the warning stronger in mentioning that this will be changed in the future (actually deprecate instead of a UserWarning), so that in the future we can actually raise for the labels=foo, index=bar case
> Ah, yes, that's a case I didn't think of. Personally, I would go for accepting that minor disadvantage to have the simpler code

Hmm I'm having trouble making the simple way (no `*args, **kwargs)` work. IIUC, then all three of these have to be handled the same way (this is master):

```python
In [15]: df.rename(None, str.lower)  # <----- This is the problem one
Out[15]:
     A    B
a  NaN  NaN
b  NaN  NaN

In [16]: df.rename(None, index=str.lower)
Out[16]:
     A    B
a  NaN  NaN
b  NaN  NaN

In [17]: df.rename(mapper=None, index=str.lower)
Out[17]:
     A    B
a  NaN  NaN
b  NaN  NaN
```

We want to treat `[15]` differently, since that is (presumably) using the old index/columns style.
> The question then maybe is if we would make the warning stronger in mentioning that this will be changed in the future

Once we're python3 only, we could using keyword-only arguments I think:

```python
def rename(mapper=None, *, index=None, columns=None, axis=None, ...):
```

I should be able to emit a warning whenever `len(args) > 1`, and in the future we can have a regular signature and remove all the hacks in this PR. 
> IUC, then all three of these have to be handled the same way (this is master):

I personally don't mind doing that (just all treating them as positional to deal with [15]). There will be a warning in any case, so people doing one of those three cases will typically change their code to not have the warning anymore. 
In the case of `df.rename(mapper=None, index=str.lower)` the warning will not be fully accurate, but then, this is really a very strange thing to do.
I'd be fine with raising on `In[17]`, since if they passed anything other than `None` we would already raise. But I think `In[16]` is perfectly fine. We would be reinterpreting that as `df.rename(index=None, columns=str.lower)`, or just `.rename(columns=str.lower)`.

Let me try using different defaults other than None to see if that helps at all...
@TomAugspurger if it is too complicated, I am also fine with this though! It's working, and we can later on remove the hacks again.
FWIW, I find this version of `_validate_axis_style_kwargs` more understandable, but presumably because it's more recent :) My last commit add a docstring / examples, so hopefully that helps.
To be clear, I mainly meant the signature hacking, I didn't look at the changes in `_validate_axis_style_kwargs` (since it moved it is a bit difficult to see what changed)
Ah, yeah. FWIW, I'm 51/49 on whether overwriting the signature is a good thing or not. It's going to confuse static type checkers anyway. I'm OK with it for now since it'll be removed once we're Python3 only.
OK, this should be all good.
@jreback are you OK with it in generic? I use `self` to figure out whether we're a Panel or a DataFrame, and it's only used on those two methods, so I think it's OK where it is.
> @jreback are you OK with it in generic? I use self to figure out whether we're a Panel or a DataFrame, and it's only used on those two methods, so I think it's OK where it is.

I don't really like it, because this is really validation code.

If you could move it would be much better (and forr sure pass in ``self`` as the arg)
Moved to `utils/_validators.py`.