fix(ToDatetime): accept pandas columns of datetime.date objects - #2231
fix(ToDatetime): accept pandas columns of datetime.date objects#2231Sanjays2402 wants to merge 2 commits into
Conversation
Pandas has no dedicated dtype for datetime.date, so a column built from date objects has the object dtype: it is neither a date column nor a string column, so ToDatetime rejected it and the DatetimeEncoder never saw it. The equivalent polars column has the Date dtype and was handled correctly, making the behaviour inconsistent between the two backends. Cast object columns that contain only datetime.date to Datetime before the dtype checks in fit_transform. Columns that are not entirely made of dates, and columns that fail to convert, are left untouched so string parsing and the existing rejections are unaffected. Adds a non-regression test and updates test_to_datetime_func, which previously excluded the pandas date column because of this bug.
rcap107
left a comment
rcap107
left a comment
There was a problem hiding this comment.
Hi @Sanjays2402, thanks for this PR.
It looks good so far, but coverage of the tests needs to be improved. Since the missing lines are in the _cast_date_objects method, you can add a test specifically for that function, rather than using ToDatetime.
Other than that, I think this issue had some kind of hidden caveat that I currently do not remember, and I'd like to weigh in with some other maintainers that are currently away before merging.
| return column | ||
| try: | ||
| return pd.to_datetime(column) | ||
| except Exception: |
There was a problem hiding this comment.
Please change this exception to the specific exception that is raised by pd.to_datetime instead
| if not (sbd.is_pandas(column) and sbd.is_object(column)): | ||
| return column | ||
| not_null = sbd.drop_nulls(column) | ||
| if sbd.shape(not_null)[0] == 0: |
There was a problem hiding this comment.
this line isn't covered by tests and coverage is complaining about it
| try: | ||
| return pd.to_datetime(column) | ||
| except Exception: | ||
| return column |
There was a problem hiding this comment.
same for this line, it should be covered
2 participants
Description
Fixes: #2084
Pandas has no dedicated dtype for
datetime.date, so a column built from date objects has theobjectdtype: it is neither a date column nor a string column, soToDatetimerejected it and theDatetimeEncoder/TableVectorizernever saw it. The equivalent polars column has theDatedtype and was handled correctly, so the two backends behaved differently.Changes
objectcolumns that contain onlydatetime.dateto Datetime before the dtype checks inToDatetime.fit_transform. Columns that are not entirely made of dates, and columns that fail to convert, are returned unchanged, so string parsing and the existing rejections are unaffected.test_to_datetime_func, which previously excluded the pandasdate-colbecause of this bug.Testing
new test fails on main with RejectColumn and passes with the fix (stash-verified); skrub/tests/test_to_datetime.py and test_datetime_encoder.py passChecklist