DEPR: deprecate .select() #17633
DEPR: deprecate .select() #17633
Conversation
jreback
added
API Design
Indexing
Codecov Report
@@ Coverage Diff @@
## master #17633 +/- ##
==========================================
- Coverage 91.22% 91.18% -0.05%
==========================================
Files 163 163
Lines 49652 49709 +57
==========================================
+ Hits 45294 45326 +32
- Misses 4358 4383 +25
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #17633 +/- ##
==========================================
- Coverage 91.26% 91.22% -0.04%
==========================================
Files 163 163
Lines 49869 49916 +47
==========================================
+ Hits 45511 45535 +24
- Misses 4358 4381 +23
Continue to review full report at Codecov.
|
|
I personally don't think we should recommend |
those work, its an option though |
jreback
force-pushed
the
select
branch
3 times, most recently
from
9c6734a to
9c2b402
Compare
|
Hello @jreback! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on October 03, 2017 at 20:10 Hours UTC |
There was a problem hiding this comment.
I personally don't feel comfortable with adding this extra 'try-and-see' guess to the loc method (whether the function should be applied on the object itself or on the index elements). In generally we would like to see less of those logics, not more IMO.
I also don't think it is really needed for deprecating select? It only means that the alternative will be a bit more convoluted.
(and if we add it, it should also be documented)
doc/source/whatsnew/v0.21.0.txt
Outdated
| .. _whatsnew_0210.api_breaking.select: | ||
|
|
||
| Select method is deprecated | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ |
There was a problem hiding this comment.
Can you move this to the 'deprecations' section?
doc/source/whatsnew/v0.21.0.txt
Outdated
|
|
||
| df = DataFrame({'A': [1, 2, 3]}, index=['foo', 'bar', 'baz']) | ||
|
|
||
| .. code_block:: ipython |
There was a problem hiding this comment.
code_block -> codeblock
Well, we already have this ability to accept pretty arbitrary things for and yes its needed for our current tests to pass. Yes its pretty fragile. I think removing this logic and simply deprecating |
It is not arbitrary, it is the calling object (series of frame). Which is clearly different from the index elements.
I would personally go for that, instead of adding this fragile logic There are still 'alternatives' to offer, although a bit more work to type. Eg: it's only a bit annoying that you need the We could also add the 'function' ability to |
|
ok will revise tomorrow |
|
@jorisvandenbossche updated, MUCH simpler. |
jreback
changed the title
|
any final comments |
pandas/core/common.py
Outdated
| @@ -441,13 +441,22 @@ def _get_callable_name(obj): | |||
| return None | |||
|
|
|||
|
|
|||
| def _apply_if_callable(maybe_callable, obj, **kwargs): | |||
| def _apply_if_callable(maybe_callable, obj, axis=None, **kwargs): | |||
There was a problem hiding this comment.
is the axis=None a left-over from the previous version of the PR? (it's not used anymore)
There was a problem hiding this comment.
no used, but its ok to put in here
There was a problem hiding this comment.
sorry, I fail to see how it is useful. It is just adding an unused argument, complicating the code?
pandas/core/generic.py
Outdated
| @@ -2339,6 +2339,8 @@ def select(self, crit, axis=0): | |||
| """ | |||
| Return data corresponding to axis labels matching criteria | |||
|
|
|||
| DEPRECATED: use df.loc[labels.map(crit)] to select via labels | |||
There was a problem hiding this comment.
maybe df.index instead of labels ?
There was a problem hiding this comment.
changed, I put the labels because you can actually select on any axis (though it does default to 0)
There was a problem hiding this comment.
ah, yes, that was a good reason :-)
pandas/core/generic.py
Outdated
| @@ -2349,6 +2351,11 @@ def select(self, crit, axis=0): | |||
| ------- | |||
| selection : type of caller | |||
| """ | |||
| warnings.warn("select is deprecated and will be removed in a " | |||
pandas/core/generic.py
Outdated
| @@ -5756,7 +5766,7 @@ def _where(self, cond, other=np.nan, inplace=False, axis=None, level=None, | |||
| inplace = validate_bool_kwarg(inplace, 'inplace') | |||
|
|
|||
| # align the cond to same shape as myself | |||
| cond = com._apply_if_callable(cond, self) | |||
| cond = com._apply_if_callable(cond, self, axis=axis) | |||
There was a problem hiding this comment.
see comment above, the axis is not used
pandas/core/indexing.py
Outdated
| @@ -107,7 +109,8 @@ def __iter__(self): | |||
|
|
|||
| def __getitem__(self, key): | |||
| if type(key) is tuple: | |||
| key = tuple(com._apply_if_callable(x, self.obj) for x in key) | |||
| key = tuple(com._apply_if_callable(x, self.obj, i) | |||
| for i, x in enumerate(key)) | |||
There was a problem hiding this comment.
here possibly as well, and a bit below as well (leftover)
| expected_weekdays = self.tsframe.reindex(index=index) | ||
|
|
||
| with tm.assert_produces_warning(FutureWarning, | ||
| check_stacklevel=False): |
There was a problem hiding this comment.
in principle the check_stacklevel=False should not be needed
|
|
||
| .. ipython:: python | ||
|
|
||
| df.loc[df.index.map(lambda x: x in ['bar', 'baz'])] |
There was a problem hiding this comment.
Does this need the .values?
There was a problem hiding this comment.
See #17738, this was just changed to not need it
|
updated, any final comments? |
|
its useful to have |
|
But it is not used by the callable. Can you please try to explain to me why you want it there? And "it is useful" is not clarifying |
|
ok I took the axis bizness out. should be good to go now. |
|
Thanks @jreback ! |
xref #12401