DEPR/BUG: Do not ignore sort in concat for DatetimeIndex #62843
+164
−22
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rhshadrach
added
Bug
Deprecate
|
@jbrockmendel - this is ready for a look. I still need to look more into the internal usages of |
| When all objects passed to :func:`concat` have a :class:`DatetimeIndex`, | ||
| passing ``sort=False`` will now result in the non-concatenation axis not | ||
| being sorted. Previously, the result would always be sorted along | ||
| the non-concatenation axis even when ``sort=False`` is passed. |
|
|
||
|
|
||
| def union_indexes(indexes, sort: bool | None = True) -> Index: | ||
| def union_indexes(indexes, sort: bool | None | lib.NoDefault = True) -> Index: |
There was a problem hiding this comment.
is None still needed here?
There was a problem hiding this comment.
By both type checking and test coverage it looks like no, will remove.
| intersect | ||
| or any(not isinstance(index, DatetimeIndex) for index in non_concat_axis) | ||
| or all( | ||
| id(prev) == id(curr) |
I'm fine with this. Though seeing the increased complexity, I'd also be OK with the previous PR's breaking change approach. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.Continuation of #62752
There are three things I'd like to accomplish here:
sort=Falseget an unsorted result. This is a bug and should be fixed without deprecation.sortdo not get a change in behavior, but will get warned when enforcing the deprecation will change behavior.concatdoes not break user code.It seems (3) will be very hard to ascertain. There are many places where
concatis used without specifyingsortbut (a) the index cannot be Datetime or (b) alignment has already been done sosorthas no impact. I plan to take a deeper look into internal usage to see if this deprecation can impact other parts of the API, I've so far only found one in groupby.shift that I think we can call a bug.To accomplish these, it seems to me that we need to do a somewhat expensive check to see if not sorting impacts the result as otherwise users will get many spurious warnings.