BUG: DataFrame.drop raising TypeError when passing empty DatetimeIndex/list #28114
Conversation
|
small comments, otherwise looks good |
* Make separate change * Fix whatsnew * Use is_list_like
|
Pushed a new commit fixing the above. |
|
Is this fixing a regression in 0.25? If not, can you move the release note to 1.0.0.rst? |
…x-27994 * Move whatsnew to v1.0.0.rst
|
The release note has been moved. |
|
@TomAugspurger Can you check if PR can be merged? |
* Handle str passed to DataFrame.drop() when dataframe has DatetimeIndex * Add comments * Fix whatsnew
| @@ -3915,6 +3915,9 @@ def drop( | |||
|
|
|||
| for axis, labels in axes.items(): | |||
| if labels is not None: | |||
| # Check for empty Index, GH 27994 | |||
| if is_list_like(labels) and not len(labels): | |||
There was a problem hiding this comment.
rather than do this, you can just handle the empty case in _drop_axis
There was a problem hiding this comment.
Are you sure @jreback? I tried a little, but I couldn't get it work while making sure to raise KeyError when passing str/List[str] to DataFrame.drop() that has DatetimeIndex.
There was a problem hiding this comment.
I think the suggestion is to include the not len(labels) check in NDFrame._drop_axis. And when labels is empty we just return self from _drop_axis.
| @@ -4720,6 +4720,10 @@ def get_indexer_non_unique(self, target): | |||
| return pself.get_indexer_non_unique(ptarget) | |||
|
|
|||
| if self.is_all_dates: | |||
| # If `target` doesn't consist of dates, `target.asi8` will return | |||
| # None, which will raise TypeError. GH 27994 | |||
| if not target.is_all_dates: | |||
There was a problem hiding this comment.
this likely is not necessary if you implemented as per my comment above
There was a problem hiding this comment.
When passing str, for example, to dataframe with DatetimeIndex, type(self) == type(target) would return False, in which case I believe KeyError should be raised. But underlying arrays aren't empty.
If not here, I feel like Cython code needs changing though I'm not confident.
There was a problem hiding this comment.
What exception do you get without this? Index.is_all_dates can be surprisingly expensive.
* Move whatsnew * Change variable names & keep consistency in tests
|
@katsuya-horiuchi can you merge master and address latest comments? |
| @@ -3915,6 +3915,9 @@ def drop( | |||
|
|
|||
| for axis, labels in axes.items(): | |||
| if labels is not None: | |||
| # Check for empty Index, GH 27994 | |||
| if is_list_like(labels) and not len(labels): | |||
|
pls merge master |
|
Sorry guys I have been extremely busy recently. I will take another look as soon as possible. |
|
can you merge master and update |
|
Closing as I think this is stale but @katsuya-horiuchi ping if you'd like to continue |
ghost
mentioned this pull request
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff