Rename signature fix #17966

TomAugspurger · 2017-10-24T18:58:09Z

closes API break with DataFrame.rename #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.

TomAugspurger · 2017-10-24T19:26:25Z

Added signature re-writing for python3 only. AFAICT, there's not a simple way to do that for both python 2 and 3.

codecov · 2017-10-24T20:53:02Z

Codecov Report

Merging #17966 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

@@            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%)`	⬇️
#single	`40.33% <75.29%> (-0.04%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/panel.py	`97% <100%> (+0.04%)`	⬆️
pandas/core/panel4d.py	`100% <100%> (ø)`	⬆️
pandas/core/generic.py	`92.44% <100%> (-0.09%)`	⬇️
pandas/core/frame.py	`97.75% <100%> (-0.1%)`	⬇️
pandas/util/_decorators.py	`80.7% <100%> (+2.7%)`	⬆️
pandas/util/_validators.py	`96.34% <100%> (+2.59%)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/plotting/_converter.py	`63.38% <0%> (-1.82%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7aeccd3...edcbb9a. Read the comment docs.

jorisvandenbossche · 2017-10-24T22:23:08Z

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?

TomAugspurger · 2017-10-24T22:40:45Z

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<#17966 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABQHIhWt100aiLR515jXWmupANBLJcERks5svmNSgaJpZM4QE7Qm>.

jorisvandenbossche · 2017-10-24T22:44:17Z

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

jreback · 2017-10-24T23:09:42Z

pandas/util/_decorators.py

+
+            sig = inspect.Signature(params)
+
+            func.__signature__ = sig


cool that u can do this

jreback · 2017-10-25T10:25:10Z

pandas/core/panel.py

-        axes = self._validate_axis_style_args(
-            labels, 'labels', axes=[items, major_axis, minor_axis],
-            axis=axis, method_name='reindex')
+    def reindex(self, *args, **kwargs):


it might actually make this more complicated, but would be ok with leaving panel/panel4d as is.

I briefly tried that, but had to add several if obj.ndim >= 3 inside internals. Got a bit messy.

TomAugspurger · 2017-10-25T11:12:49Z

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):

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.

TomAugspurger · 2017-10-25T11:15:06Z

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:

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.

jorisvandenbossche · 2017-10-25T11:38:19Z

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.

TomAugspurger · 2017-10-25T11:45:48Z

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...

jorisvandenbossche · 2017-10-25T11:48:10Z

@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.

TomAugspurger · 2017-10-25T11:51:28Z

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.

jorisvandenbossche · 2017-10-25T11:53:32Z

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)

TomAugspurger · 2017-10-25T11:58:17Z

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.

TomAugspurger · 2017-10-25T22:13:10Z

OK, this should be all good.

jreback · 2017-10-25T22:33:29Z

pandas/core/generic.py

+            warnings.warn(msg.format(method_name=method_name,), FutureWarning,
+                          stacklevel=4)
+            out[self._AXIS_NAMES[0]] = args[0]
+            out[self._AXIS_NAMES[1]] = args[1]


would really love to move this out of generic (and just pass self into a static function, maybe into pandas.compat.validations or something).

This is only used by reindex/rename, and now is sits next to those functions. IMO it is easier to keep it that way.

jorisvandenbossche

Now also looked at the rewritten validate_axis_args method. Looks good!

jorisvandenbossche · 2017-10-25T22:49:16Z

pandas/core/generic.py

+        elif len(args) == 2:
+            if 'axis' in kwargs:
+                # Unambiguously wrong
+                msg = "Cannot specify both {} and any of 'index' or 'columns'"


Shouldn't this be "Cannot specify both axis and index / columns" ?(as you check that axis is specified)

(and maybe ' ' around {})

jorisvandenbossche · 2017-10-25T22:49:18Z

pandas/core/generic.py

+            warnings.warn(msg.format(method_name=method_name,), FutureWarning,
+                          stacklevel=4)
+            out[self._AXIS_NAMES[0]] = args[0]
+            out[self._AXIS_NAMES[1]] = args[1]


This is only used by reindex/rename, and now is sits next to those functions. IMO it is easier to keep it that way.

TomAugspurger · 2017-10-26T15:08:33Z

@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 · 2017-10-26T23:58:02Z

@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)

TomAugspurger · 2017-10-27T01:29:29Z

Moved to utils/_validators.py.

jreback

lgtm

* Signature rewriting

TomAugspurger added 5 commits October 24, 2017 12:59

BUG/REF: Handle ambiguous rename

05efc51

Do for panel too

7c419e0

For panel4d

fbf246b

Pep8

ef77ca1

Signature rewriting

44e01da

TomAugspurger added this to the 0.21.0 milestone Oct 24, 2017

TomAugspurger added the API Design label Oct 24, 2017

TomAugspurger mentioned this pull request Oct 24, 2017

test against pandas rc dask/dask#2814

Merged

jreback reviewed Oct 24, 2017

View reviewed changes

pandas/util/_decorators.py

sig = inspect.Signature(params)

func.__signature__ = sig

Copy link

Contributor

jreback Oct 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool that u can do this

jreback reviewed Oct 25, 2017

View reviewed changes

Added docs, changed to futurewarning

396a454

Move comment

3cb3fac

TomAugspurger changed the title ~~[WIP]Rename signature fix~~ Rename signature fix Oct 25, 2017

TomAugspurger added 2 commits October 25, 2017 17:08

Catch bug in rename

4766c24

Fill in coverage

ccbd75e

jreback reviewed Oct 25, 2017

View reviewed changes

jorisvandenbossche reviewed Oct 25, 2017

View reviewed changes

TomAugspurger added 2 commits October 25, 2017 20:52

pep8

d01aa20

Fixed error message

1c1517a

TomAugspurger mentioned this pull request Oct 26, 2017

RLS: 0.21.0 #17285

Closed

64 tasks

TomAugspurger added 2 commits October 26, 2017 20:22

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

06144af

Move to validators

edcbb9a

jreback approved these changes Oct 27, 2017

View reviewed changes

jorisvandenbossche merged commit 3863063 into pandas-dev:master Oct 27, 2017

TomAugspurger deleted the rename-sig-fix branch October 27, 2017 12:04

peterpanmj pushed a commit to peterpanmj/pandas that referenced this pull request Oct 31, 2017

BUG/REF: Handle ambiguous rename case (pandas-dev#17966)

be1af11

* Signature rewriting

No-Stream pushed a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017

BUG/REF: Handle ambiguous rename case (pandas-dev#17966)

5572934

* Signature rewriting

mroeschke mentioned this pull request Jan 7, 2023

DEPR: Enforce disallowing 2 positional args in DataFrame.reindex #50612

Merged

mroeschke mentioned this pull request Jan 19, 2023

REF: Use * syntax to make reindex kwargs keyword only #50853

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename signature fix #17966

Rename signature fix #17966

TomAugspurger commented Oct 24, 2017

TomAugspurger commented Oct 24, 2017

codecov bot commented Oct 24, 2017 •

edited

Loading

jorisvandenbossche commented Oct 24, 2017

TomAugspurger commented Oct 24, 2017 via email

jorisvandenbossche commented Oct 24, 2017

jreback Oct 24, 2017

jreback Oct 25, 2017

TomAugspurger Oct 25, 2017

TomAugspurger commented Oct 25, 2017

TomAugspurger commented Oct 25, 2017

jorisvandenbossche commented Oct 25, 2017

TomAugspurger commented Oct 25, 2017

jorisvandenbossche commented Oct 25, 2017

TomAugspurger commented Oct 25, 2017

jorisvandenbossche commented Oct 25, 2017

TomAugspurger commented Oct 25, 2017

TomAugspurger commented Oct 25, 2017

jreback Oct 25, 2017

jorisvandenbossche Oct 25, 2017

jorisvandenbossche left a comment

jorisvandenbossche Oct 25, 2017

jorisvandenbossche Oct 25, 2017

jorisvandenbossche Oct 25, 2017

TomAugspurger commented Oct 26, 2017

jreback commented Oct 26, 2017

TomAugspurger commented Oct 27, 2017

jreback left a comment

Rename signature fix #17966

Rename signature fix #17966

Conversation

TomAugspurger commented Oct 24, 2017

TomAugspurger commented Oct 24, 2017

codecov bot commented Oct 24, 2017 • edited Loading

Codecov Report

jorisvandenbossche commented Oct 24, 2017

TomAugspurger commented Oct 24, 2017 via email

jorisvandenbossche commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Oct 25, 2017

TomAugspurger commented Oct 25, 2017

jorisvandenbossche commented Oct 25, 2017

TomAugspurger commented Oct 25, 2017

jorisvandenbossche commented Oct 25, 2017

TomAugspurger commented Oct 25, 2017

jorisvandenbossche commented Oct 25, 2017

TomAugspurger commented Oct 25, 2017

TomAugspurger commented Oct 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche 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

TomAugspurger commented Oct 26, 2017

jreback commented Oct 26, 2017

TomAugspurger commented Oct 27, 2017

jreback left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 24, 2017 •

edited

Loading