-
-
Notifications
You must be signed in to change notification settings - Fork 19.4k
ENH(string dtype): Make str.decode return str dtype #60709
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
| ser = Series(["a", "b", "a\xe4"], dtype=any_string_dtype).str.encode("utf-8") | ||
| result = ser.str.decode("utf-8") | ||
| expected = ser.map(lambda x: x.decode("utf-8")).astype(object) | ||
| expected = Series(["a", "b", "a\xe4"], dtype="str") |
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.
The change from ser.map to using Series is just to make this test a bit more explicit. Using ser.map(...).astype("str") also passes.
jorisvandenbossche
left a comment
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.
Looks good!
pandas/io/pytables.py
Outdated
| if get_option("future.infer_string"): | ||
| data = ser.to_numpy() | ||
| else: | ||
| data = ser._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.
You can probably simplify this and always to .to_numpy()? (or np.asarray(..))
In the case of object dtype in the else branch, that will return the same (and be as cheap) as _values I think
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.
Confirmed - thanks.
|
@rhshadrach can you update this? |
|
@jorisvandenbossche - the issue with Perhaps there could be a way (e.g. |
|
Hmm, good point. Ideally we would be able to solve this without using private APIs, I think, because it is a good case study for what also other people (external code) could run into. So I think what we have said before is that downstream users could do But this also makes me wonder if we should re-discuss if we have to add some keyword to |
|
@jorisvandenbossche - ran into a couple more test changes. |
|
Thanks @rhshadrach |
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
|
Backport PR: #60821 |
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.