-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # 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