BUG: Slicing non monotonic DatetimeIndex did not raise for non-existing keys #37796
Conversation
phofl
added
the
Indexing
| @@ -1563,6 +1563,15 @@ def test_loc_getitem_slice_label_td64obj(self, start, stop, expected_slice): | |||
| expected = ser.iloc[expected_slice] | |||
| tm.assert_series_equal(result, expected) | |||
|
|
|||
| def test_loc_getitem_slice_unordered_dt_index(self, frame_or_series): | |||
There was a problem hiding this comment.
can you add the str index example from the OP as well. ping on green.
There was a problem hiding this comment.
Interestingly enough this does not raise any more. I will open an issue for this. We have to define if slice_locs should raise too or if we should doo this after calling the function. Is not related to the DateTime fix.
� Conflicts: � doc/source/whatsnew/v1.2.0.rst
jorisvandenbossche
changed the title
|
If there are "historical reasons", then we could also deprecate it first. @phofl can you also add a test where the resulting dataframe would not be empty? So where eg the start label is missing but would fall somewhere in the middle of the range of the index labels |
i am not sure there is anything to deprecate, this is just a bug |
|
It's also explicitly tested behaviour (I don't know the motivations of the PR that added this test, though) |
its wrong it should raise, pls see the OP |
� Conflicts: � doc/source/whatsnew/v1.2.0.rst
|
Sorry for the confusion with the other issue. It raises for non-monotonic indexes and not for monotonic indexes |
| index=[Timestamp("2017"), Timestamp("2019"), Timestamp("2018")], | ||
| ) | ||
| with pytest.raises(KeyError, match=r"Timestamp\('2020-01-01 00:00:00'\)"): | ||
| obj.loc["2020":"2022"] |
There was a problem hiding this comment.
Can you also add an example where the label falls within the range of the Index?
Eg with the index being [2016, 2019, 2017] and then the slice "2018":"2020" or so
There was a problem hiding this comment.
This works, because of
pandas/pandas/core/indexes/datetimes.py
Line 799 in 72d4b78
Should we remove or deprecate? Harder fix than the previous one
There was a problem hiding this comment.
hmm, i guess we could try to deprecate, @jorisvandenbossche you feel strongly here?
I agree the behaviour seems wrong and we should change it. To be clear I am not arguing for not having it raise an error. I am only saying that if this is explicitly tested behaviour, it might be depended upon (again, I don't know when / for what reason this test was added), and we can consider deprecating it first. |
fair. ok let's see if possible to deprecate. though am tempted to call this wrong and just a bug fix but let's see. |
|
Opened #37819 to deprecate this. If we decide to deprecate first, we can close this one. |
|
closing in favor of #37819 |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diffThe test for historical reasons :) failed after the change. Is this a problem?