BUG: Index.get_slice_bounds does not accept datetime.date or tz naive datetime.datetimes #35848
Conversation
| @@ -0,0 +1,76 @@ | |||
| from datetime import date, datetime | |||
There was a problem hiding this comment.
i think this should go in tests.indexes.datetimes.test_indexing
There was a problem hiding this comment.
I am also testing object and numeric Indexes in this file as well. Where should those tests go?
There was a problem hiding this comment.
id put the object ones in tests.indexes.base_class.test_indexing.TestGetSliceBounds and the numeric ones in tests.indexes.numeric.test_indexing.TestGetSliceBounds
pandas/core/indexes/datetimes.py
Outdated
| @@ -672,6 +672,11 @@ def _maybe_cast_slice_bound(self, label, side: str, kind): | |||
| if self._is_strictly_monotonic_decreasing and len(self) > 1: | |||
| return upper if side == "left" else lower | |||
| return lower if side == "left" else upper | |||
| # GH 35690: Ensure the label matches the timezone of the index's tz | |||
There was a problem hiding this comment.
I'll comment in #35690, not entirely sure this is the desired behavior.
Assuming it is the desired behavior, can this casting take place in _maybe_cast_for_get_loc? If not, can you update the docstring to reflect this change
There was a problem hiding this comment.
Sure yeah this can probably go in _maybe_cast_for_get_loc
pandas/core/indexes/datetimes.py
Outdated
| # needed to localize naive datetimes | ||
| # needed to localize naive datetimes or dates (GH 35690) | ||
| if isinstance(key, date) and not isinstance(key, datetime): | ||
| key = datetime.combine(key, time(0, 0)) |
There was a problem hiding this comment.
won't the Timestamp(key) below do this already?
|
Just commented on #35690, the upshot of which is that I think the current behavior is correct. |
pandas/core/indexes/base.py
Outdated
| if side not in ("left", "right"): | ||
| raise ValueError( | ||
| "Invalid value for side kwarg, must be either " | ||
| f"'left' or 'right': {side}" | ||
| ) | ||
|
|
||
| if kind not in ("loc", "getitem", None): | ||
| raise ValueError( |
There was a problem hiding this comment.
this should be an AssertionError as this is an internal method (yes I know @RhysU is actually calling it), but its officially public and merely an interface from .loc
|
Hello @mroeschke! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-09-02 05:43:44 UTC |
|
lgtm @mroeschke ideally we also test via |
|
See comment above, this PR should not be merged. |
… datetime.datetimes (pandas-dev#35848)
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diffThis method appeared under tested so added some additional tests for numeric and object
Index