-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
BUG: is_categorical shouldnt recognize Dtype objects #33377
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
Conversation
@@ -68,9 +68,6 @@ | |||
ensure_float64 = algos.ensure_float64 | |||
ensure_float32 = algos.ensure_float32 | |||
|
|||
_ensure_datetime64ns = conversion.ensure_datetime64ns | |||
_ensure_timedelta64ns = conversion.ensure_timedelta64ns |
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.
unrelated edit before i figured out what this branch was about; these are never used
@@ -4716,7 +4716,7 @@ def groupby(self, values) -> PrettyDict[Hashable, np.ndarray]: | |||
# TODO: if we are a MultiIndex, we can do better | |||
# that converting to tuples | |||
if isinstance(values, ABCMultiIndex): | |||
values = values.values | |||
values = values._values |
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.
unrelated edit from before i figured out what this branch was about
This will need a deprecation cycle right? |
id classify this as a bugfix, but OK either way |
the other option is just to deprecate it, since we only use is_categorical in a few places and its more likely than not to cause confusion |
Closing in favor of #33385. |
is_categorical(obj) is used in a couple places where we then go on to access
obj.dtype
, so this should not returnTrue
if we have aCategoricalDtype
object.