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

EricChea · 2018-02-18T19:54:51Z

What's this PR do?

In response to #19727.
Adds functionality to the deprecate_kwargs utility. Currently, the utility doesn't allow the deprecation of an old keyword without having a new one in place. The change proposed here allows the deprecation of an old keyword without specifying a new keyword by allowing users to pass new_arg_name=None to deprecate_kwargs.

PR checklist

closes (Re)-deprecate order keyword in factorize #19727
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

…der. Add deprecation warning for entire keywords to deprecate_kwaargs util.

jreback · 2018-02-18T20:21:11Z

pandas/tests/util/test_util.py

+        @deprecate_kwarg('old', None)
+        def _f4(old=True, unchanged=True):
+            return old
+
        self.f1 = _f1


why are u adding all this?

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.

codecov · 2018-02-19T04:41:20Z

Codecov Report

❗ No coverage uploaded for pull request base (master@695614d). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #19751   +/-   ##
=========================================
  Coverage          ?   91.58%           
=========================================
  Files             ?      150           
  Lines             ?    48894           
  Branches          ?        0           
=========================================
  Hits              ?    44781           
  Misses            ?     4113           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`89.96% <100%> (?)`
#single	`41.8% <42.85%> (?)`

Impacted Files	Coverage Δ
pandas/core/algorithms.py	`94.16% <100%> (ø)`
pandas/util/_decorators.py	`82.4% <100%> (ø)`

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 695614d...b7ed3c4. Read the comment docs.

jreback

need to add a deprecation notice in 0.23.0 whatsnew

jreback · 2018-02-19T15:50:41Z

pandas/util/_decorators.py

-    new_arg_name : str
-        Name of preferred argument in function
+    new_arg_name : str | None
+        Name of preferred argument in function. Use none to raise warning that


none -> None

Thanks for catching that. Please refer to ab47916

jreback · 2018-02-19T15:51:09Z

pandas/util/_decorators.py

+
+            if new_arg_name is None and old_arg_value is not None:
+                msg = (
+                    "the '{old_name}' keyword is no longer supported"


the {old_name} keyword is deprecated and will be removed in a future version

is our typical phrasing here.

Thank you. Please refer to ab47916

jreback · 2018-02-19T15:51:39Z

pandas/core/algorithms.py

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


+@deprecate_kwarg(old_arg_name='order', new_arg_name=None)


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

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.

pep8speaks · 2018-02-20T01:35:56Z

Hello @EricChea! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on February 21, 2018 at 11:37 Hours UTC

- Change none to None in docs. - Change warning message to be consistent with other phrasing. - Add tests to catch FutureWarning when order keyword is passed to factorize.

jreback · 2018-02-20T01:46:48Z

pandas/tests/test_algos.py

@@ -248,6 +248,12 @@ 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):
+        data = np.array([2**63, 1, 2**63], dtype=np.uint64)


can you add the gh issue number here as a comment

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

jreback · 2018-02-20T01:47:43Z

pandas/util/_decorators.py

-        Name of preferred argument in function
+    new_arg_name : str | 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


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

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.

jreback · 2018-02-20T01:48:02Z

pandas/util/_decorators.py

+
+            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"


you might need to break this line (too long) for the linter

Thanks for catching that. I need to remember to run git diff master -u -- "*.py" | flake8 --diff

jreback · 2018-02-20T01:49:00Z

pandas/util/_decorators.py

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


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)

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

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

jreback · 2018-02-20T11:38:54Z

can you add a whatsnew note in the deprecation section, otherwise lgtm.

TomAugspurger · 2018-02-20T11:47:55Z

pandas/util/_decorators.py

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


Usually this is written as str or None.

Thanks for catching this -- didn't realize this is the case. Please refer to 4c83659

TomAugspurger · 2018-02-20T11:48:25Z

pandas/util/_decorators.py

@@ -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!
+
+


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

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

…rder is used with functionalize

jorisvandenbossche · 2018-02-20T15:19:35Z

doc/source/whatsnew/v0.23.0.txt

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


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.

Makes sense. Please refer to b5daae0

jreback · 2018-02-21T11:38:04Z

thanks @EricChea

small correction on merge, the note was in the incorrect deprecation section.

EricChea · 2018-02-21T13:17:11Z

@jreback thanks for doing that.

…ev#19751)

[Utils][Warnings] Add deprecation warning for factorize() keyword, or…

261044a

…der. Add deprecation warning for entire keywords to deprecate_kwaargs util.

jreback requested changes Feb 18, 2018

View reviewed changes

jreback requested changes Feb 19, 2018

View reviewed changes

jreback added the Deprecate Functionality to remove in pandas label Feb 19, 2018

jsexauer mentioned this pull request Feb 19, 2018

DEPR: Clean up list of deprecations from prior versions #6581

Closed

1 task

jreback added this to the 0.23.0 milestone Feb 20, 2018

PR review for deprecating keyword, order, from factorize().

ab47916

- Change none to None in docs. - Change warning message to be consistent with other phrasing. - Add tests to catch FutureWarning when order keyword is passed to factorize.

jreback requested changes Feb 20, 2018

View reviewed changes

eric and others added 2 commits February 19, 2018 21:40

Add examples of how to deprecate a keyword, and fix spacing.

616c357

doc

18eb031

TomAugspurger reviewed Feb 20, 2018

View reviewed changes

eric and others added 3 commits February 20, 2018 08:59

Add to what's new: deprecation of keyword ability, and warning when o…

6a34d9e

…rder is used with functionalize

Change '|' to 'or' in docs.

4c83659

Merge branch 'master' into dep_keyword

1b10ca2

jorisvandenbossche changed the title ~~[Utils][Warnings] Add deprecation warning for factorize() keyword, or…~~ DEPR: Add deprecation warning for factorize() order keyword Feb 20, 2018

jorisvandenbossche reviewed Feb 20, 2018

View reviewed changes

eric and others added 3 commits February 20, 2018 10:40

Remove deprecate_kwarg util changes from whats new.

b5daae0

doc

bd9717e

Merge branch 'master' into PR_TOOL_MERGE_PR_19751

b7ed3c4

jreback approved these changes Feb 21, 2018

View reviewed changes

jreback merged commit eb149ce into pandas-dev:master Feb 21, 2018

EricChea deleted the dep_keyword branch February 22, 2018 05:17

harisbal pushed a commit to harisbal/pandas that referenced this pull request Feb 28, 2018

DEPR: Add deprecation warning for factorize() order keyword (pandas-d…

e7e1712

…ev#19751)

jreback mentioned this pull request Nov 20, 2019

DEPR: deprecations log for removed issues #13777

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

EricChea commented Feb 18, 2018 •

edited

Loading

jreback Feb 18, 2018

EricChea Feb 18, 2018

codecov bot commented Feb 19, 2018 •

edited

Loading

jreback left a comment

jreback Feb 19, 2018

EricChea Feb 20, 2018 •

edited

Loading

jreback Feb 19, 2018

EricChea Feb 20, 2018 •

edited

Loading

jreback Feb 19, 2018

EricChea Feb 20, 2018 •

edited

Loading

pep8speaks commented Feb 20, 2018 •

edited

Loading

jreback Feb 20, 2018

EricChea Feb 20, 2018

jreback Feb 20, 2018

EricChea Feb 20, 2018 •

edited

Loading

jreback Feb 20, 2018

EricChea Feb 20, 2018

jreback Feb 20, 2018

EricChea Feb 20, 2018

EricChea Feb 21, 2018 •

edited

Loading

jreback commented Feb 20, 2018

TomAugspurger Feb 20, 2018

EricChea Feb 20, 2018

TomAugspurger Feb 20, 2018

EricChea Feb 20, 2018

jorisvandenbossche Feb 20, 2018

EricChea Feb 20, 2018

jreback commented Feb 21, 2018

EricChea commented Feb 21, 2018

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


		@deprecate_kwarg(old_arg_name='order', new_arg_name=None)

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

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

Conversation

EricChea commented Feb 18, 2018 • edited Loading

What's this PR do?

PR checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 19, 2018 • edited Loading

Codecov Report

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricChea Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricChea Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricChea Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

pep8speaks commented Feb 20, 2018 • edited Loading

Comment last updated on February 21, 2018 at 11:37 Hours UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricChea Feb 20, 2018 • edited Loading

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

Choose a reason for hiding this comment

EricChea Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

jreback commented Feb 20, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Feb 21, 2018

EricChea commented Feb 21, 2018

EricChea commented Feb 18, 2018 •

edited

Loading

codecov bot commented Feb 19, 2018 •

edited

Loading

EricChea Feb 20, 2018 •

edited

Loading

EricChea Feb 20, 2018 •

edited

Loading

EricChea Feb 20, 2018 •

edited

Loading

pep8speaks commented Feb 20, 2018 •

edited

Loading

EricChea Feb 20, 2018 •

edited

Loading

EricChea Feb 21, 2018 •

edited

Loading