-
-
Notifications
You must be signed in to change notification settings - Fork 19.3k
BUG: Remove special-casing for date objects in DatetimeIndex indexing (GH#62158) #62198
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?
BUG: Remove special-casing for date objects in DatetimeIndex indexing (GH#62158) #62198
Conversation
|
@jbrockmendel hi I have a question about handling the new deprecation warning Thanks! |
…estamp in indexing and merging tests
…estamp in indexing and merging tests
…estamp in indexing and merging tests
|
Mostly looks good, will need a whatsnew note |
…et_indexer and update whatsnew note (pandas-dev#62158)
|
@jbrockmendel Thanks for the feedback! I’ve applied the suggested changes and updated the PR. Please let me know if there’s anything else that needs adjustment. |
|
Can you get the CI passing? |
|
Sure, I’ll look into the CI failures. |
|
Hi @jbrockmendel , I’ve identified that one of the CI failures is due to the |
|
test_align_date_objects_with_datetimeindex involves alignment that will change when dates no longer match to datetimes |
|
@jbrockmendel Thanks for the help! I’ve updated the changes, but CI is still failing. I’ll look into this in more detail. |
256908c to
1c59ae7
Compare
|
@jbrockmendel i’m stuck- getting 3 check fails, i tried to fix them but haven't been successful. Could you please take a look and guide me on this? |
|
The test_align_date_objects_with_datetimeindex ones look like the behavior is correct and the test The pyarrow one looks like you need to add a check to exclude |
|
@jbrockmendel still getting CI check fails, there are 3 test failure. I've updated as per suggestions. |
Are you sure? The CI logs show pretty clearly what's wrong. |
|
@jbrockmendel i may have missed something, could you please point out where the mistake is ? |
pandas/core/indexes/datetimes.py
Outdated
| else: | ||
| return indexer | ||
|
|
||
| def get_indexer(self, target, method=None, limit=None, tolerance=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.
this override really shouldnt be needed
|
Look in the CI log: |
| import pyarrow as pa | ||
|
|
||
| if dtype.kind != "M": | ||
| return False |
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.
shouldn't this depend on self.dtype?
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 have updated with self.dtype.kind or dtype.kind, please correct me if i should only use self.dtype.kind
pandas/core/indexes/base.py
Outdated
| if pa.types.is_date(pa_dtype): | ||
| return False | ||
| if pa.types.is_timestamp(pa_dtype): | ||
| if (getattr(pa_dtype, "tz", None) is 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.
i think the condition on L6315 implies that the attribute always exists?
|
FAILED pandas/tests/indexes/datetimes/test_indexing.py::TestGetIndexer::test_get_indexer_date_objs - AssertionError: numpy array are different |
pandas/core/indexes/base.py
Outdated
| elif is_numeric_dtype(self.dtype): | ||
| return is_numeric_dtype(dtype) | ||
| # GH#62158 | ||
| elif self.dtype.kind == "M" and dtype == object: |
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 object dtype is always comparable?
|
|
||
| pa_dtype = dtype.pyarrow_dtype | ||
| if self.dtype.kind != "M" or dtype.kind != "M": | ||
| return False |
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 seems weird. wouldn't this return False if both are int64 dtypes?
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 added checks inside the arrow dtype, also in _should_compare hope I'm not going in the wrong direction. please correct me if I am.
This PR resolves (GH#62158) by removing the legacy special-casing for Python date objects in DatetimeIndex indexing logic.
What’s Changed:
Removed the special-case handling of date objects in DatetimeIndex.getitem.
Updated/added tests in pandas/tests/frame/indexing/test_indexing.py to reflect the new, consistent behavior.
Please let me know if my approach or fix needs any improvements . I’m open to feedback and happy to make changes based on suggestions.
Thankyou !