Skip to content

DEPR: Deprecate the convert parameter completely #17831

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

Merged
merged 1 commit into from
Oct 12, 2017
Merged

DEPR: Deprecate the convert parameter completely #17831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 6 additions & 4 deletions pandas/core/generic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2172,6 +2172,7 @@ def _take(self, indices, axis=0, convert=True, is_copy=True):
selecting rows, "1" means that we are selecting columns, etc.
convert : bool, default True
.. deprecated:: 0.21.0
In the future, negative indices will always be converted.
Copy link
Contributor

Choose a reason for hiding this comment

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

leftover comment

Copy link
Contributor

Choose a reason for hiding this comment

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

@TomAugspurger prob just as easy to fix this on merge

Copy link
Contributor

Choose a reason for hiding this comment

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

I can fix that in #17858

Copy link
Contributor

Choose a reason for hiding this comment

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

Although, why do you say that's leftover? Wasn't that added deliberately @gfyoung?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor

Choose a reason for hiding this comment

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

actually nvm thought was a different comment


Whether to convert negative indices into positive ones.
For example, ``-1`` would map to the ``len(axis) - 1``.
Expand Down Expand Up @@ -2234,14 +2235,15 @@ class max_speed
"""

@Appender(_shared_docs['take'])
def take(self, indices, axis=0, convert=True, is_copy=True, **kwargs):
nv.validate_take(tuple(), kwargs)

if not convert:
def take(self, indices, axis=0, convert=None, is_copy=True, **kwargs):
if convert is not None:
msg = ("The 'convert' parameter is deprecated "
"and will be removed in a future version.")
warnings.warn(msg, FutureWarning, stacklevel=2)
else:
convert = True

convert = nv.validate_take(tuple(), kwargs)
return self._take(indices, axis=axis, convert=convert, is_copy=is_copy)

def xs(self, key, axis=0, level=None, drop_level=True):
Expand Down
11 changes: 6 additions & 5 deletions pandas/core/sparse/series.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ def _ixs(self, i, axis=0):
"""
label = self.index[i]
if isinstance(label, Index):
return self.take(i, axis=axis, convert=True)
return self.take(i, axis=axis)
else:
return self._get_val_at(i)

Expand Down Expand Up @@ -629,14 +629,15 @@ def sparse_reindex(self, new_index):
fill_value=self.fill_value).__finalize__(self)

@Appender(generic._shared_docs['take'])
def take(self, indices, axis=0, convert=True, *args, **kwargs):
convert = nv.validate_take_with_convert(convert, args, kwargs)

if not convert:
def take(self, indices, axis=0, convert=None, *args, **kwargs):
if convert is not None:
msg = ("The 'convert' parameter is deprecated "
"and will be removed in a future version.")
warnings.warn(msg, FutureWarning, stacklevel=2)
else:
convert = True

nv.validate_take_with_convert(convert, args, kwargs)
new_values = SparseArray.take(self.values, indices)
new_index = self.index.take(indices)
return self._constructor(new_values,
Expand Down
4 changes: 4 additions & 0 deletions pandas/tests/frame/test_axis_select_reindex.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -944,6 +944,10 @@ def test_take(self):
expected = df.reindex(df.index.take(order))
assert_frame_equal(result, expected)

with tm.assert_produces_warning(FutureWarning):
result = df.take(order, convert=True, axis=0)
assert_frame_equal(result, expected)

with tm.assert_produces_warning(FutureWarning):
result = df.take(order, convert=False, axis=0)
assert_frame_equal(result, expected)
Expand Down
21 changes: 10 additions & 11 deletions pandas/tests/sparse/test_series.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,28 +528,27 @@ def _compare(idx):
exp = pd.Series(np.repeat(nan, 5))
tm.assert_series_equal(sp.take([0, 1, 2, 3, 4]), exp)

with tm.assert_produces_warning(FutureWarning):
sp.take([1, 5], convert=True)

with tm.assert_produces_warning(FutureWarning):
sp.take([1, 5], convert=False)

def test_numpy_take(self):
sp = SparseSeries([1.0, 2.0, 3.0])
indices = [1, 2]

# gh-17352: older versions of numpy don't properly
# pass in arguments to downstream .take() implementations.
warning = FutureWarning if _np_version_under1p12 else None
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason this is not the case anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I need to put that back. However, I different default will be needed, as convert=None breaks downstream compatibility with numpy:

https://travis-ci.org/pandas-dev/pandas/jobs/285909832

Copy link
Member

Choose a reason for hiding this comment

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

What is the problem with None for numpy? You linked to a sparse failure, but in the sparse code you left the default as None.

If it is needed, another solution might be to push it into the kwargs? (so the -1 is not visible)

BTW, can you push new commits instead of squashing into the previous one? That makes it easier to say what you changed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, sorry, missed your message before I pushed.

The problem is that numpy's take implementation for slightly older versions passes in None for one of its parameters by position. This parameter corresponds to the convert parameter for our version of take. It's messy compatibility stuff that I had to wade through at some point that I patched on both sides.


with tm.assert_produces_warning(warning, check_stacklevel=False):
if not _np_version_under1p12:
tm.assert_series_equal(np.take(sp, indices, axis=0).to_dense(),
np.take(sp.to_dense(), indices, axis=0))

msg = "the 'out' parameter is not supported"
tm.assert_raises_regex(ValueError, msg, np.take,
sp, indices, out=np.empty(sp.shape))
msg = "the 'out' parameter is not supported"
tm.assert_raises_regex(ValueError, msg, np.take,
sp, indices, out=np.empty(sp.shape))

msg = "the 'mode' parameter is not supported"
tm.assert_raises_regex(ValueError, msg, np.take,
sp, indices, mode='clip')
msg = "the 'mode' parameter is not supported"
tm.assert_raises_regex(ValueError, msg, np.take,
sp, indices, out=None, mode='clip')

def test_setitem(self):
self.bseries[5] = 7.
Expand Down