-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
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: infer-objects raising for bytes Series #50067
Conversation
@@ -1969,6 +1969,9 @@ def convert( | |||
attempt to cast any object types to better types return a copy of | |||
the block (if copy = True) by definition we ARE an ObjectBlock!!!!! | |||
""" | |||
if self.dtype != object: | |||
return [self] |
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 this may need to make a copy in some 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.
Are you sure?
ser = Series(["a"], dtype="object")
result = ser.infer_objects()
result.iloc[0] = "c"
print(ser)
this updates ser as well, same as the bytes version. If we add a copy, the bytes version would not update anymore
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.
- infer_objects in principle makes a copy in cases where it doesn't do inference. 2) I don't think that's what most users (or our internal usages) actually want. 3) ATM the ObjectBlock case that doesn't do inference doesn't make a copy, which is inconsistent with other dtypes. 4) i think adding a copy keyword makes sense xref ENH/PERF: copy=True keyword in methods where copy=False can improve perf #48141, but adding copy keywords in general got pushback recently
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 guess we can ignore this since were not consistent about it 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.
Can address in follow up
Needs rebase |
# Conflicts: # pandas/tests/series/methods/test_infer_objects.py
# Conflicts: # pandas/tests/series/methods/test_infer_objects.py
rebased |
thanks @phofl |
maybe_convert_objects
fails for byte series #49650 (Replace xxxx with the GitHub issue number)doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.cc @jbrockmendel