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.

Sign up for GitHub

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

Jump to bottom

Deprecate non-keyword arguments for drop_duplicates. #41500

Merged
merged 15 commits into from
May 25, 2021
Merged

Deprecate non-keyword arguments for drop_duplicates. #41500

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
ebd40aa
ENH: Deprecate non-keyword arguments for drop_duplicates.
jmholzer May 16, 2021
8cb7645
leave newline
jmholzer May 16, 2021
fa6574c
ENH: Deprecate non-keyword arguments for drop_duplicates.
jmholzer May 16, 2021
d7c341a
ENH: Deprecate non-keyword arguments for drop_duplicates.
jmholzer May 16, 2021
19aa589
ENH: Deprecate non-keyword arguments for drop_duplicates.
jmholzer May 16, 2021
448e8a1
merge
jmholzer May 20, 2021
0d54ca7
ENH: Deprecate non-keyword arguments for drop_duplicates.
jmholzer May 20, 2021
463c37a
ENH: Deprecate non-keyword arguments for drop_duplicates.
jmholzer May 21, 2021
c0d3d34
merge
jmholzer May 22, 2021
2cb482f
ENH: Deprecate non-keyword arguments for drop_duplicates.
jmholzer May 23, 2021
09fe413
ENH: Deprecate non-keyword arguments for drop_duplicates.
jmholzer May 23, 2021
03d0330
remove redundant line
MarcoGorelli May 23, 2021
be8393d
Merge remote-tracking branch 'upstream/master' into pr/jmholzer/41500
MarcoGorelli May 23, 2021
fbf70a2
ENH: Deprecate non-keyword arguments for drop_duplicates.
jmholzer May 24, 2021
937d9e2
Merge branch 'deprecate-nonkeyword-args-drop_duplicates' of https://g…
jmholzer May 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/source/whatsnew/v1.3.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,6 +647,7 @@ Deprecations
- Deprecated setting :attr:`Categorical._codes`, create a new :class:`Categorical` with the desired codes instead (:issue:`40606`)
- Deprecated behavior of :meth:`DatetimeIndex.union` with mixed timezones; in a future version both will be cast to UTC instead of object dtype (:issue:`39328`)
- Deprecated using ``usecols`` with out of bounds indices for ``read_csv`` with ``engine="c"`` (:issue:`25623`)
- Deprecated passing arguments as positional in :meth:`DataFrame.drop_duplicates` (except for ``subset``) and :meth:`Series.drop_duplicates` (:issue:`41485`)
Copy link
Member

Choose a reason for hiding this comment

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

maybe we should do the same for Index at the same time for consistency.

Copy link
Member

Choose a reason for hiding this comment

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

we will get error: Signature of "drop_duplicates" incompatible with supertype "IndexOpsMixin" [override] otherwise

Copy link
Contributor Author

@jmholzer jmholzer May 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Should I add the warning decorator to Index.drop_duplicates + test in a commit on this branch / pull request?

cc @MarcoGorelli

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that would be great, thanks @jmholzer !

Copy link
Contributor Author

@jmholzer jmholzer May 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I made these changes in the commit 064268f.

However, it looks like a lot of commits that I pulled from the master branch ended up in this pull request also, I don't have enough knowledge of git to understand why this happened (I used rebase, but didn't think the commits from master would show up in the PR). Is this a problem? If so, can I fix it?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah you'll need to rebase (see https://youtu.be/hv8dhOEzQcM), currently this is showing lots of unrelated changes. Something like

git fetch upstream
git rebase -i upstream/master

and then in the interactive window choose which commits to keep/drop/fixup/edit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the guidance, I appreciate it.

I did the following to fix things:

  1. Reverted the changes on my local branch to before the rebase.
  2. Cherry-picked my commit with the changes for Index from the remote.
  3. Pulled remote master changes to my local master branch.
  4. Merged my local feature branch with my local master branch.
  5. Force-pushed my local feature branch to the remote.

That has gotten rid of the spurious commits. I tried using a rebase instead of a merge in step 3, as you suggested, but I couldn't do it cleanly.

Are steps 3-4 acceptable for working on pandas, or is a rebase generally preferred to a merge?

Copy link
Member

Choose a reason for hiding this comment

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

yeah that's fine, commits will get squashed anyway


.. ---------------------------------------------------------------------------

Expand Down
2 changes: 2 additions & 0 deletions pandas/core/frame.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@
Appender,
Substitution,
deprecate_kwarg,
deprecate_nonkeyword_arguments,
doc,
rewrite_axis_style_signature,
)
Expand Down Expand Up @@ -5953,6 +5954,7 @@ def dropna(
else:
return result

@deprecate_nonkeyword_arguments(version="2.0", allowed_args=["self", "subset"])
def drop_duplicates(
self,
subset: Hashable | Sequence[Hashable] | None = None,
Expand Down
2 changes: 2 additions & 0 deletions pandas/core/series.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
from pandas.util._decorators import (
Appender,
Substitution,
deprecate_nonkeyword_arguments,
doc,
)
from pandas.util._validators import (
Expand Down Expand Up @@ -2024,6 +2025,7 @@ def drop_duplicates(self, *, inplace: Literal[True]) -> None:
def drop_duplicates(self, keep=..., inplace: bool = ...) -> Series | None:
...

@deprecate_nonkeyword_arguments(version="2.0", allowed_args=["self"])
MarcoGorelli marked this conversation as resolved.
Show resolved Hide resolved
def drop_duplicates(self, keep="first", inplace=False) -> Series | None:
"""
Return Series with duplicate values removed.
Expand Down
11 changes: 11 additions & 0 deletions pandas/tests/frame/methods/test_drop_duplicates.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,3 +471,14 @@ def test_drop_duplicates_non_boolean_ignore_index(arg):
msg = '^For argument "ignore_index" expected type bool, received type .*.$'
with pytest.raises(ValueError, match=msg):
df.drop_duplicates(ignore_index=arg)


def test_drop_duplicates_pos_args_deprecation():
# GH#41485
df = DataFrame({"a": [1, 1, 2], "b": [1, 1, 3], "c": [1, 1, 3]})
msg = (
r"Starting with Pandas version 2\.0 all arguments of drop_duplicates "
r"except for the arguments 'self' and 'subset' will be keyword-only"
)
with tm.assert_produces_warning(FutureWarning, match=msg):
df.drop_duplicates(["b", "c"], "last")
11 changes: 11 additions & 0 deletions pandas/tests/series/methods/test_drop_duplicates.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,3 +223,14 @@ def test_drop_duplicates_categorical_bool(self, ordered):
return_value = sc.drop_duplicates(keep=False, inplace=True)
assert return_value is None
tm.assert_series_equal(sc, tc[~expected])


def test_drop_duplicates_pos_args_deprecation():
# GH#41485
s = Series(["a", "b", "c", "b"])
msg = (
r"Starting with Pandas version 2\.0 all arguments of drop_duplicates "
r"except for the argument 'self' will be keyword-only"
)
with tm.assert_produces_warning(FutureWarning, match=msg):
s.drop_duplicates("last")