Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename signature fix #17966

Merged
merged 13 commits into from
Oct 27, 2017
30 changes: 19 additions & 11 deletions pandas/core/frame.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@
from pandas import compat
from pandas.compat import PY36
from pandas.compat.numpy import function as nv
from pandas.util._decorators import Appender, Substitution
from pandas.util._decorators import (Appender, Substitution,
rewrite_axis_style_signature)
from pandas.util._validators import validate_bool_kwarg

from pandas.core.indexes.period import PeriodIndex
Expand Down Expand Up @@ -2917,12 +2918,18 @@ def align(self, other, join='outer', axis=None, level=None, copy=True,
broadcast_axis=broadcast_axis)

@Appender(_shared_docs['reindex'] % _shared_doc_kwargs)
def reindex(self, labels=None, index=None, columns=None, axis=None,
**kwargs):
axes = self._validate_axis_style_args(labels, 'labels',
axes=[index, columns],
axis=axis, method_name='reindex')
@rewrite_axis_style_signature('labels', [('method', None),
('copy', True),
('level', None),
('fill_value', np.nan),
('limit', None),
('tolerance', None)])
def reindex(self, *args, **kwargs):
axes = self._validate_axis_style_args(args, kwargs, 'labels',
'reindex')
kwargs.update(axes)
kwargs.pop('axis', None)
kwargs.pop('labels', None)
return super(DataFrame, self).reindex(**kwargs)

@Appender(_shared_docs['reindex_axis'] % _shared_doc_kwargs)
Expand All @@ -2933,8 +2940,10 @@ def reindex_axis(self, labels, axis=0, method=None, level=None, copy=True,
method=method, level=level, copy=copy,
limit=limit, fill_value=fill_value)

def rename(self, mapper=None, index=None, columns=None, axis=None,
**kwargs):
@rewrite_axis_style_signature('mapper', [('copy', True),
('inplace', False),
('level', None)])
def rename(self, *args, **kwargs):
"""Alter axes labels.

Function / dict values must be unique (1-to-1). Labels not contained in
Expand Down Expand Up @@ -3008,10 +3017,9 @@ def rename(self, mapper=None, index=None, columns=None, axis=None,
2 2 5
4 3 6
"""
axes = self._validate_axis_style_args(mapper, 'mapper',
axes=[index, columns],
axis=axis, method_name='rename')
axes = self._validate_axis_style_args(args, kwargs, 'mapper', 'rename')
kwargs.update(axes)
kwargs.pop('axis', None)
return super(DataFrame, self).rename(**kwargs)

@Appender(_shared_docs['fillna'] % _shared_doc_kwargs)
Expand Down
98 changes: 52 additions & 46 deletions pandas/core/generic.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
from pandas.core.dtypes.cast import maybe_promote, maybe_upcast_putmask
from pandas.core.dtypes.missing import isna, notna
from pandas.core.dtypes.generic import ABCSeries, ABCPanel, ABCDataFrame
from pandas.core.common import (_all_not_none, _count_not_none,
from pandas.core.common import (_count_not_none,
_maybe_box_datetimelike, _values_from_object,
AbstractMethodError, SettingWithCopyError,
SettingWithCopyWarning)
Expand Down Expand Up @@ -728,51 +728,6 @@ def swaplevel(self, i=-2, j=-1, axis=0):
result._data.set_axis(axis, labels.swaplevel(i, j))
return result

def _validate_axis_style_args(self, arg, arg_name, axes,
axis, method_name):
out = {}
for i, value in enumerate(axes):
if value is not None:
out[self._AXIS_NAMES[i]] = value

aliases = ', '.join(self._AXIS_NAMES.values())
if axis is not None:
# Using "axis" style, along with a positional arg
# Both index and columns should be None then
axis = self._get_axis_name(axis)
if any(x is not None for x in axes):
msg = (
"Can't specify both 'axis' and {aliases}. "
"Specify either\n"
"\t.{method_name}({arg_name}, axis=axis), or\n"
"\t.{method_name}(index=index, columns=columns)"
).format(arg_name=arg_name, method_name=method_name,
aliases=aliases)
raise TypeError(msg)
out[axis] = arg

elif _all_not_none(arg, *axes):
msg = (
"Cannot specify all of '{arg_name}', {aliases}. "
"Specify either {arg_name} and 'axis', or {aliases}."
).format(arg_name=arg_name, aliases=aliases)
raise TypeError(msg)

elif _all_not_none(arg, axes[0]):
# This is the "ambiguous" case, so emit a warning
msg = (
"Interpreting call to '.{method_name}(a, b)' as "
"'.{method_name}(index=a, columns=b)'. " # TODO
"Use keyword arguments to remove any ambiguity."
).format(method_name=method_name)
warnings.warn(msg, stacklevel=3)
out[self._AXIS_ORDERS[0]] = arg
out[self._AXIS_ORDERS[1]] = axes[0]
elif axes[0] is None:
# This is for the default axis, like reindex([0, 1])
out[self._AXIS_ORDERS[0]] = arg
return out

# ----------------------------------------------------------------------
# Rename

Expand Down Expand Up @@ -2822,6 +2777,57 @@ def sort_index(self, axis=0, level=None, ascending=True, inplace=False,
new_axis = labels.take(sort_index)
return self.reindex(**{axis_name: new_axis})

def _validate_axis_style_args(self, args, kwargs, arg_name, method_name):
out = {}
# Goal: fill out with index/columns-style arguments
# like out = {'index': foo, 'columns': bar}
# Start by validaing for consistency
if 'axis' in kwargs and any(x in kwargs for x in self._AXIS_NUMBERS):
msg = "Cannot specify both 'axis' and any of 'columns' or 'index'."
raise TypeError(msg)

# Start with explicit values provided by the user...
if arg_name in kwargs:
if args:
msg = "{} got multiple values for argument '{}'".format(
method_name, arg_name)
raise TypeError(msg)
axis = self._get_axis_name(kwargs.get('axis', 0))
out[axis] = kwargs[arg_name]

# fill in axes
for k, v in kwargs.items():
try:
ax = self._get_axis_name(k)
except ValueError:
pass
else:
out[ax] = v

# All user-provided kwargs have been handled now.
# Now we supplement with positional arguments, raising when there's
# ambiguity

if len(args) == 0:
pass # validate later
elif len(args) == 1:
axis = self._get_axis_name(kwargs.get('axis', 0))
out[axis] = args[0]
elif len(args) == 2:
if 'axis' in kwargs:
msg = "Cannot specify both {} and any of 'index' or 'columns'"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

(and maybe ' ' around {})

raise TypeError(msg.format(arg_name))
msg = ("Intepreting call\n\t'.{method_name}(a, b)' as "
"\n\t'.{method_name}(index=a, columns=b)'.\nUse named "
"arguments to remove any ambiguity.")
warnings.warn(msg.format(method_name=method_name,), stacklevel=3)
out[self._AXIS_NAMES[0]] = args[0]
out[self._AXIS_NAMES[1]] = args[1]
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

else:
msg = "Cannot specify all of '{}', 'index', 'columns'."
raise TypeError(msg.format(arg_name))
return out

_shared_docs['reindex'] = """
Conform %(klass)s to new index with optional filling logic, placing
NA/NaN in locations having no value in the previous index. A new object
Expand Down
31 changes: 21 additions & 10 deletions pandas/core/panel.py
Original file line number Diff line number Diff line change
Expand Up @@ -1199,20 +1199,31 @@ def _wrap_result(self, result, axis):
return self._construct_return_type(result, axes)

@Appender(_shared_docs['reindex'] % _shared_doc_kwargs)
def reindex(self, labels=None,
items=None, major_axis=None, minor_axis=None,
axis=None, **kwargs):
major_axis = (major_axis if major_axis is not None else
kwargs.pop('major', None))
minor_axis = (minor_axis if minor_axis is not None else
kwargs.pop('minor', None))
axes = self._validate_axis_style_args(
labels, 'labels', axes=[items, major_axis, minor_axis],
axis=axis, method_name='reindex')
def reindex(self, *args, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

major = kwargs.pop("major", None)
minor = kwargs.pop('minor', None)

if major is not None:
if kwargs.get("major_axis"):
raise TypeError("Cannot specify both 'major' and 'major_axis'")
kwargs['major_axis'] = major
if minor is not None:
if kwargs.get("minor_axis"):
raise TypeError("Cannot specify both 'minor' and 'minor_axis'")

kwargs['minor_axis'] = minor
# major_axis = (major_axis if major_axis is not None else
# kwargs.pop('major', None))
# minor_axis = (minor_axis if minor_axis is not None else
# kwargs.pop('minor', None))
axes = self._validate_axis_style_args(args, kwargs, 'labels',
'reindex')
if self.ndim >= 4:
# Hack for PanelND
axes = {}
kwargs.update(axes)
kwargs.pop('axis', None)
kwargs.pop('labels', None)
return super(Panel, self).reindex(**kwargs)

@Appender(_shared_docs['rename'] % _shared_doc_kwargs)
Expand Down
33 changes: 28 additions & 5 deletions pandas/core/panel4d.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
""" Panel4D: a 4-d dict like collection of panels """

import warnings
from pandas.core.generic import NDFrame
from pandas.core.panelnd import create_nd_panel_factory
from pandas.core.panel import Panel

Expand Down Expand Up @@ -62,12 +63,34 @@ def panel4d_reindex(self, labs=None, labels=None, items=None, major_axis=None,
# Hack for reindex_axis deprecation
# Ha, we used labels for two different things
# I think this will work still.
axes = self._validate_axis_style_args(
labs, 'labels',
axes=[labels, items, major_axis, minor_axis],
axis=axis, method_name='reindex')
if labs is None:
args = ()
else:
args = (labs,)
kwargs_ = dict(labels=labels,
items=items,
major_axis=major_axis,
minor_axis=minor_axis,
axis=axis)
kwargs_ = {k: v for k, v in kwargs_.items() if v is not None}
# major = kwargs.pop("major", None)
# minor = kwargs.pop('minor', None)

# if major is not None:
# if kwargs.get("major_axis"):
# raise TypeError("Cannot specify both 'major' and 'major_axis'")
# kwargs_['major_axis'] = major
# if minor is not None:
# if kwargs.get("minor_axis"):
# raise TypeError("Cannot specify both 'minor' and 'minor_axis'")
# kwargs_['minor_axis'] = minor

if axis is not None:
kwargs_['axis'] = axis

axes = self._validate_axis_style_args(args, kwargs_, 'labs', 'reindex')
kwargs.update(axes)
return super(Panel, self).reindex(**kwargs)
return NDFrame.reindex(self, **kwargs)


Panel4D.__init__ = panel4d_init
Expand Down
29 changes: 28 additions & 1 deletion pandas/tests/frame/test_alter_axes.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,14 @@

from __future__ import print_function

import inspect
import pytest

from datetime import datetime, timedelta

import numpy as np

from pandas.compat import lrange
from pandas.compat import lrange, PY2
from pandas import (DataFrame, Series, Index, MultiIndex,
RangeIndex, date_range, IntervalIndex,
to_datetime)
Expand Down Expand Up @@ -956,6 +957,17 @@ def test_reindex_api_equivalence(self):
for res in [res2, res3]:
tm.assert_frame_equal(res1, res)

def test_rename_positional(self):
df = pd.DataFrame(columns=['A', 'B'])
with tm.assert_produces_warning(UserWarning) as rec:
result = df.rename(None, str.lower)
expected = pd.DataFrame(columns=['a', 'b'])
assert_frame_equal(result, expected)
assert len(rec) == 1
message = str(rec[0].message)
assert 'rename' in message
assert 'Use named arguments' in message

def test_assign_columns(self):
self.frame['hi'] = 'there'

Expand Down Expand Up @@ -987,6 +999,21 @@ def test_ambiguous_warns(self):
with tm.assert_produces_warning(UserWarning):
df.rename({0: 10}, {"A": "B"})

@pytest.mark.skipif(PY2, reason="inspect.signature")
def test_rename_signature(self):
sig = inspect.signature(pd.DataFrame.rename)
parameters = set(sig.parameters)
assert parameters == {"self", "mapper", "index", "columns", "axis",
"inplace", "copy", "level"}

@pytest.mark.skipif(PY2, reason="inspect.signature")
def test_reindex_signature(self):
sig = inspect.signature(pd.DataFrame.reindex)
parameters = set(sig.parameters)
assert parameters == {"self", "labels", "index", "columns", "axis",
"limit", "copy", "level", "method",
"fill_value", "tolerance"}


class TestIntervalIndex(object):

Expand Down
16 changes: 8 additions & 8 deletions pandas/tests/frame/test_axis_select_reindex.py
Original file line number Diff line number Diff line change
Expand Up @@ -476,32 +476,32 @@ def test_reindex_positional_warns(self):
def test_reindex_axis_style_raises(self):
# https://github.com/pandas-dev/pandas/issues/12392
df = pd.DataFrame({"A": [1, 2, 3], 'B': [4, 5, 6]})
with tm.assert_raises_regex(TypeError, 'reindex'):
with tm.assert_raises_regex(TypeError, 'Cannot specify both labels'):
df.reindex([0, 1], ['A'], axis=1)

with tm.assert_raises_regex(TypeError, 'reindex'):
with tm.assert_raises_regex(TypeError, 'Cannot specify both labels'):
df.reindex([0, 1], ['A'], axis='index')

with tm.assert_raises_regex(TypeError, 'reindex'):
with tm.assert_raises_regex(TypeError, "Cannot specify both 'axis'"):
df.reindex(index=[0, 1], axis='index')

with tm.assert_raises_regex(TypeError, 'reindex'):
with tm.assert_raises_regex(TypeError, "Cannot specify both 'axis'"):
df.reindex(index=[0, 1], axis='columns')

with tm.assert_raises_regex(TypeError, 'reindex'):
with tm.assert_raises_regex(TypeError, "Cannot specify both 'axis'"):
df.reindex(columns=[0, 1], axis='columns')

with tm.assert_raises_regex(TypeError, 'reindex'):
with tm.assert_raises_regex(TypeError, "Cannot specify both 'axis'"):
df.reindex(index=[0, 1], columns=[0, 1], axis='columns')

with tm.assert_raises_regex(TypeError, 'Cannot specify all'):
df.reindex([0, 1], [0], ['A'])

# Mixing styles
with tm.assert_raises_regex(TypeError, 'reindex'):
with tm.assert_raises_regex(TypeError, "Cannot specify both 'axis'"):
df.reindex(index=[0, 1], axis='index')

with tm.assert_raises_regex(TypeError, 'reindex'):
with tm.assert_raises_regex(TypeError, "Cannot specify both 'axis'"):
df.reindex(index=[0, 1], axis='columns')

def test_reindex_single_named_indexer(self):
Expand Down
Loading