-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
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
DEPR: Disallow dtype inference when setting Index into DataFrame #56102
Conversation
Thanks @phofl |
# TODO: Remove kludge in sanitize_array for string mode when enforcing | ||
# this deprecation | ||
warnings.warn( | ||
"Setting an Index with object dtype into a DataFrame will no longer " |
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.
is "will no longer" clear enough that this refers to a future version/deprecation?
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.
good point, I'll put up a PR to clarify
return sanitize_array(value, self.index, copy=True, allow_2d=True), None | ||
arr = sanitize_array(value, self.index, copy=True, allow_2d=True) | ||
if ( | ||
isinstance(value, Index) |
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.
might be more performant to do the Index check before the sanitize_array?
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.
Why? We have to sanitise anyway?
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.
if you know you have an Index (and no dtype), sanitize_array boils down to:
if len(value) != len(index): raise ...
return value._values
so a lot of sanitize_array may be made unnecessary in this case
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.
That's what I want in the future, but we go through maybe_infer_to_datetimelike
at the moment which changes the dtype, so we can't shortcut this before we enforce the deprecation
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.cc @jbrockmendel I think we chatted about this. Series keeps the dtype, so this is mostly for consistency and to make our lives a little easier