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
2 changes: 2 additions & 0 deletions doc/source/whatsnew/v0.23.0.txt
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,7 @@ Other API Changes
- :func:`Series.to_csv` now accepts a ``compression`` argument that works in the same way as the ``compression`` argument in :func:`DataFrame.to_csv` (:issue:`18958`)
- Set operations (union, difference...) on :class:`IntervalIndex` with incompatible index types will now raise a ``TypeError`` rather than a ``ValueError`` (:issue:`19329`)
- :class:`DateOffset` objects render more simply, e.g. ``<DateOffset: days=1>`` instead of ``<DateOffset: kwds={'days': 1}>`` (:issue:`19403`)
- :func:`util._decorators.deprecate_kwarg` now has the ability to deprecate a keyword entirely. Previously, using ``@deprecate_kwarg`` required that an alternative keyword be provided in favor of the deprecated keyword.
Copy link
Member

Choose a reason for hiding this comment

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

It's not needed to add this to the whatsnew docs, since this is private functionality. So the deprecation warning about order itself is enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Please refer to b5daae0


.. _whatsnew_0230.deprecations:

Expand Down Expand Up @@ -643,6 +644,7 @@ Removal of prior version deprecations/changes
- The modules ``pandas.tools.hashing`` and ``pandas.util.hashing`` have been removed (:issue:`16223`)
- The top-level functions ``pd.rolling_*``, ``pd.expanding_*`` and ``pd.ewm*`` have been removed (Deprecated since v0.18).
Instead, use the DataFrame/Series methods :attr:`~DataFrame.rolling`, :attr:`~DataFrame.expanding` and :attr:`~DataFrame.ewm` (:issue:`18723`)
- Warnings against the obsolete usage of the order keyword in ``core.algorithms.factorize``. For example, ``core.algorithms.factorize(data, order)``, emits a warning that the order keyword will be deprecated (:issue:`19727`)

.. _whatsnew_0230.performance:

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)
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 add the gh issue number here as a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I added an additional comment so we can remember to deprecate the test once the order keyword is finally removed.

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
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add a couple of examples to the doc-string here? e.g. doing a rename and removing a kwarg

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.

I added a couple examples for removing a keyword.

I did see a couple good examples of how to raise a warning when a keyword will be deprecated for another:
keyword to keyword
keyword map (multiple keyword deprecations)

Do let me know if you think we should still add some more examples for renaming kwargs.

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

since we have this new capability for deprecate_kwarg if you wanted (in a followup PR), to change existing code to use this kwargs rather than the 'manual' way of warning (e.g. before we had 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.

Sure. I'll take a look around and open a separate PR.

Copy link
Contributor Author

@EricChea EricChea Feb 21, 2018

Choose a reason for hiding this comment

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

@jreback following up. I found that the remaining 'manual' deprecation warnings in the repo target an entire function or class (and in some cases class attributes). I didn't find any for keyword specific deprecations. I suppose I can create an analogous decorator called deprecate_func() or deprecate_obj if that would be of value.

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