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

DEPR: Add deprecation warning for factorize() order keyword #19751

Merged
merged 10 commits into from
Feb 21, 2018
1 change: 1 addition & 0 deletions doc/source/whatsnew/v0.23.0.txt
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,7 @@ Deprecations

- The ``broadcast`` parameter of ``.apply()`` is deprecated in favor of ``result_type='broadcast'`` (:issue:`18577`)
- The ``reduce`` parameter of ``.apply()`` is deprecated in favor of ``result_type='reduce'`` (:issue:`18577`)
- The ``order`` parameter of :func:`factorize` is deprecated and will be removed in a future release (:issue:`19727`)

.. _whatsnew_0230.prior_deprecations:

Expand Down
2 changes: 2 additions & 0 deletions pandas/core/algorithms.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
from pandas.core import common as com
from pandas._libs import algos, lib, hashtable as htable
from pandas._libs.tslib import iNaT
from pandas.util._decorators import deprecate_kwarg


# --------------- #
Expand Down Expand Up @@ -436,6 +437,7 @@ def isin(comps, values):
return f(comps, values)


@deprecate_kwarg(old_arg_name='order', new_arg_name=None)
Copy link
Contributor

Choose a reason for hiding this comment

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

ca you add a test which catches this, AND see if we have any existing cases of actually passing this arg

Copy link
Contributor Author

@EricChea EricChea Feb 20, 2018

Choose a reason for hiding this comment

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

Sure thing. I added two additional tests. Please refer to ab47916.

I did not find any cases in the Repo where factorize() is called with an order parameter.

def factorize(values, sort=False, order=None, na_sentinel=-1, size_hint=None):
"""
Encode input values as an enumerated type or categorical variable
Expand Down
9 changes: 9 additions & 0 deletions pandas/tests/test_algos.py
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,15 @@ def test_uint64_factorize(self):
tm.assert_numpy_array_equal(labels, exp_labels)
tm.assert_numpy_array_equal(uniques, exp_uniques)

def test_deprecate_order(self):
# gh 19727 - check warning is raised for deprecated keyword, order.
# Test not valid once order keyword is removed.
data = np.array([2**63, 1, 2**63], dtype=np.uint64)
with tm.assert_produces_warning(expected_warning=FutureWarning):
algos.factorize(data, order=True)
with tm.assert_produces_warning(False):
algos.factorize(data)


class TestUnique(object):

Expand Down
14 changes: 14 additions & 0 deletions pandas/tests/util/test_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,14 @@ def _f2(new=False):
def _f3(new=0):
return new

@deprecate_kwarg('old', None)
def _f4(old=True, unchanged=True):
return old

self.f1 = _f1
Copy link
Contributor

Choose a reason for hiding this comment

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

why are u adding all this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a nutshell, to test that warnings are only raised if _f4() is called with the old keyword (keyword to deprecate).

I gave _f4() two keywords:

  • old to represent the keyword that's not removed yet.
  • unchanged is meant to represent a keyword that shouldn't be impacted by the decorator.

With f4 I can test that:

  • A FutureWarning will get raised if a user calls f4 with the old (deprecated keyword). For example self.f4(old=9) will raise an alert that the old keyword is no longer supported.
  • No warning is raised if f4 is called without the old keyword. For example, self.f4(unchanged=x) will not raise an alert.

Hope that makes sense.

self.f2 = _f2
self.f3 = _f3
self.f4 = _f4

def test_deprecate_kwarg(self):
x = 78
Expand Down Expand Up @@ -72,6 +77,15 @@ def test_bad_deprecate_kwarg(self):
def f4(new=None):
pass

def test_deprecate_keyword(self):
x = 9
with tm.assert_produces_warning(FutureWarning):
result = self.f4(old=x)
assert result is x
with tm.assert_produces_warning(None):
result = self.f4(unchanged=x)
assert result is True


def test_rands():
r = tm.rands(10)
Expand Down
38 changes: 36 additions & 2 deletions pandas/util/_decorators.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,9 @@ def deprecate_kwarg(old_arg_name, new_arg_name, mapping=None, stacklevel=2):
----------
old_arg_name : str
Name of argument in function to deprecate
new_arg_name : str
Name of preferred argument in function
new_arg_name : str or None
Name of preferred argument in function. Use None to raise warning that
``old_arg_name`` keyword is deprecated.
mapping : dict or callable
If mapping is present, use it to translate old arguments to
new arguments. A callable must do its own value checking;
Expand All @@ -82,12 +83,15 @@ def deprecate_kwarg(old_arg_name, new_arg_name, mapping=None, stacklevel=2):
...
>>> f(columns='should work ok')
should work ok

>>> f(cols='should raise warning')
FutureWarning: cols is deprecated, use columns instead
warnings.warn(msg, FutureWarning)
should raise warning

>>> f(cols='should error', columns="can\'t pass do both")
TypeError: Can only specify 'cols' or 'columns', not both

>>> @deprecate_kwarg('old', 'new', {'yes': True, 'no': False})
... def f(new=False):
... print('yes!' if new else 'no!')
Expand All @@ -96,6 +100,25 @@ def deprecate_kwarg(old_arg_name, new_arg_name, mapping=None, stacklevel=2):
FutureWarning: old='yes' is deprecated, use new=True instead
warnings.warn(msg, FutureWarning)
yes!


Copy link
Contributor

Choose a reason for hiding this comment

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

Can you have this many blank lines in a docstring without flake8 complaining?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that is correct. I didn't get any warnings here.

To raise a warning that a keyword will be removed entirely in the future

>>> @deprecate_kwarg(old_arg_name='cols', new_arg_name=None)
... def f(cols='', another_param=''):
... print(cols)
...
>>> f(cols='should raise warning')
FutureWarning: the 'cols' keyword is deprecated and will be removed in a
future version please takes steps to stop use of 'cols'
should raise warning
>>> f(another_param='should not raise warning')
should not raise warning

>>> f(cols='should raise warning', another_param='')
FutureWarning: the 'cols' keyword is deprecated and will be removed in a
future version please takes steps to stop use of 'cols'
should raise warning
"""

if mapping is not None and not hasattr(mapping, 'get') and \
Expand All @@ -107,6 +130,17 @@ def _deprecate_kwarg(func):
@wraps(func)
def wrapper(*args, **kwargs):
old_arg_value = kwargs.pop(old_arg_name, None)

if new_arg_name is None and old_arg_value is not None:
msg = (
"the '{old_name}' keyword is deprecated and will be "
"removed in a future version "
"please takes steps to stop use of '{old_name}'"
).format(old_name=old_arg_name)
warnings.warn(msg, FutureWarning, stacklevel=stacklevel)
kwargs[old_arg_name] = old_arg_value
return func(*args, **kwargs)

if old_arg_value is not None:
if mapping is not None:
if hasattr(mapping, 'get'):
Expand Down