DEPR: Add deprecation warning for factorize() order keyword by EricChea · Pull Request #19751 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation29 Commits10 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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.
| @deprecate_kwarg('old', None) |
|---|
| def _f4(old=True, unchanged=True): |
| return old |
| self.f1 = _f1 |
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?
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 theoldkeyword 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to add a deprecation notice in 0.23.0 whatsnew
| 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 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
none -> None
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching that. Please refer to ab47916
| if new_arg_name is None and old_arg_value is not None: |
| msg = ( |
| "the '{old_name}' keyword is no longer supported" |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the {old_name} keyword is deprecated and will be removed in a future version
is our typical phrasing here.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Please refer to ab47916
| @@ -436,6 +437,7 @@ def isin(comps, values): |
|---|
| return f(comps, values) |
| @deprecate_kwarg(old_arg_name='order', new_arg_name=None) |
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
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.
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.
| @@ -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) |
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
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.
| 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 |
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
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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" |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you might need to break this line (too long) for the linter
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching that. I need to remember to run git diff master -u -- "*.py" | flake8 --diff
| 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) |
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)
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.
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.
eric and others added 2 commits
can you add a whatsnew note in the deprecation section, otherwise lgtm.
| @@ -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 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually this is written as str or None.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this -- didn't realize this is the case. Please refer to 4c83659
| @@ -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! |
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?
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.
eric and others added 3 commits
…rder is used with functionalize
[Utils][Warnings] Add deprecation warning for factorize() keyword, or… DEPR: Add deprecation warning for factorize() order keyword
| @@ -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. |
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.
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
eric and others added 3 commits
thanks @EricChea
small correction on merge, the note was in the incorrect deprecation section.
harisbal pushed a commit to harisbal/pandas that referenced this pull request
