-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
BUG: Make DTI/TDI/PI argsort match their underlying arrays #37965
Conversation
pandas/core/groupby/grouper.py
Outdated
@@ -374,7 +374,8 @@ def _set_grouper(self, obj: FrameOrSeries, sort: bool = False): | |||
# possibly sort | |||
if (self.sort or sort) and not ax.is_monotonic: | |||
# use stable sort to support first, last, nth | |||
indexer = self.indexer = ax.argsort(kind="mergesort") | |||
# TODO: why does putting na_position="first" fix datetimelike cases? | |||
indexer = self.indexer = ax.argsort(kind="mergesort", na_position="first") |
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.
def argsort(
self,
ascending: bool = True,
kind: str = "quicksort",
na_position: str = "last",
in arrays/base.py so this is strange
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 it must involve the fact that on older numpys, dt64.argsort used to put NaT at the beginning instead of the end
@mroeschke will you have time to take a look at this in the not-too-distant future? if not ill mothball, since its stuck in the mud. |
@@ -374,7 +374,10 @@ def _set_grouper(self, obj: FrameOrSeries, sort: bool = False): | |||
# possibly sort | |||
if (self.sort or sort) and not ax.is_monotonic: | |||
# use stable sort to support first, last, nth | |||
indexer = self.indexer = ax.argsort(kind="mergesort") | |||
# TODO: why does putting na_position="first" fix datetimelike cases? |
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 am not really familiar with the history of NaTs with respect to resample. I would have assumed NaT would/should have been dropped by this point since we just exclude them from the result #13164.
I'm okay with this solution as a stop gap solution.
lgtm. any user facing change? |
Changes DTi/TDI/PI.argsort, that should be it. |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
The fix to keep resample working is a kludge I'd like to make unnecessary before merging; cc @mroeschke