-
-
Notifications
You must be signed in to change notification settings - Fork 19.1k
DEPR: Remove special date-datetime64 case in indexing #62232
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.
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
base: main
Are you sure you want to change the base?
DEPR: Remove special date-datetime64 case in indexing #62232
Conversation
x, "M" | ||
) or isinstance(x, DatetimeTZDtype) | ||
_infer_matches = ("datetime", "datetime64", "date") | ||
_infer_matches = ("datetime", "datetime64") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might be worth doing, but the motivating issue I opened was only about the check in indexes.base
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in my testing after removing the check in indexes.base, the example in the original issue still fails because of _infer_matches called under _validate_listlike
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, looking at the other places where _infer_matches is used, im OK with that change, but will need to make sure we have appropriate testing and whatsnew note for e.g. equals
result2 = target.get_indexer(index) | ||
tm.assert_numpy_array_equal(result2, expected) | ||
|
||
def test_get_indexer_date_objs(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of removing the test entirely, test the new behavior, with a comment about the history/change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(havent looked closely, but this likely applies to tests below)
ts_slice = ts[5:] | ||
ts2 = ts_slice.copy() | ||
ts2.index = [x.date() for x in ts2.index] | ||
ts2.index = pd.to_datetime([x.date() for x in ts2.index]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of this, test the new behavior with the old index
def test_categorical_coerces_timestamp(all_parsers): | ||
parser = all_parsers | ||
dtype = {"b": CategoricalDtype([Timestamp("2014")])} | ||
dtype = {"b": CategoricalDtype([Timestamp("2014-01-01 12:00:00")])} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this change needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when read_csv reads "b\n2014-01-01\n2014-01-01"
it converts the data into date objects, while the Timestamps are in datetime64. With the deprecation in this PR, this is no longer supported. Updating the data with the time "b\n2014-01-01 12:00:00\n2014-01-01 12:00:00"
ensures read_csv produces datetime64 values that match the Timestamp categories
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we want to continue testing the original input data
|
||
def test_categorical_coerces_datetime(all_parsers): | ||
parser = all_parsers | ||
dti = pd.DatetimeIndex(["2017-01-01", "2018-01-01", "2019-01-01"], freq=None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this removed?
this looks almost ready. can you address comments and get the code checks CI passing |
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.