Skip to content
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

ENH(string dtype): Make str.decode return str dtype #60709

Merged
merged 8 commits into from
Jan 29, 2025

Conversation

rhshadrach
Copy link
Member

  • closes #xxxx (Replace xxxx with the GitHub issue number)
  • Tests added and passed if fixing a bug or adding a new feature
  • All code checks passed.
  • Added type annotations to new arguments/methods/functions.
  • Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

@rhshadrach rhshadrach added Enhancement Strings String extension data type and string data labels Jan 12, 2025
@rhshadrach rhshadrach marked this pull request as draft January 12, 2025 21:17
@rhshadrach rhshadrach changed the title TST(string dtype): Make str.decode return str dtype ENH(string dtype): Make str.decode return str dtype Jan 12, 2025
@@ -566,7 +566,7 @@ def test_string_slice_out_of_bounds(any_string_dtype):
def test_encode_decode(any_string_dtype):
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")
Copy link
Member Author

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.

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Comment on lines 5213 to 5216
if get_option("future.infer_string"):
data = ser.to_numpy()
else:
data = ser._values
Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed - thanks.

@jorisvandenbossche jorisvandenbossche added this to the 2.3 milestone Jan 22, 2025
@jorisvandenbossche
Copy link
Member

@rhshadrach can you update this?

@rhshadrach rhshadrach marked this pull request as ready for review January 25, 2025 01:32
@rhshadrach rhshadrach marked this pull request as draft January 25, 2025 02:31
@rhshadrach
Copy link
Member Author

@jorisvandenbossche - the issue with .to_numpy on NumPy-backed Series is that we set the underlying data to read-only. In pytables, we switch out NA values in libwriters.string_array_replace_from_nan_rep, which is causing the tests to fail.

Perhaps there could be a way (e.g. Series._to_numpy) to always get a corresponding NumPy array that isn't read-only? Barring this, it seems to me we could either always (and unnecessarily) make a copy, or use my original branching logic. Open to other ideas too.

@jorisvandenbossche
Copy link
Member

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 data.flags.writeable = True on the result of to_numpy() if they know what they are doing (and in this case we know that we indeed own the memory, because we are reading a file and created that data and not yet returned it to the user).

But this also makes me wonder if we should re-discuss if we have to add some keyword to to_numpy() to get this (e.g. something like writeable=True)

@rhshadrach rhshadrach marked this pull request as ready for review January 29, 2025 21:46
@rhshadrach
Copy link
Member Author

@jorisvandenbossche - ran into a couple more test changes.

@mroeschke mroeschke merged commit c36da3f into pandas-dev:main Jan 29, 2025
42 checks passed
@mroeschke
Copy link
Member

Thanks @rhshadrach

Copy link

lumberbot-app bot commented Jan 29, 2025

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 2.3.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 c36da3f6ded4141add4b3b16c252cedf4641e5ea
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #60709: ENH(string dtype): Make str.decode return str dtype'
  1. Push to a named branch:
git push YOURFORK 2.3.x:auto-backport-of-pr-60709-on-2.3.x
  1. Create a PR against branch 2.3.x, I would have named this PR:

"Backport PR #60709 on branch 2.3.x (ENH(string dtype): Make str.decode return str dtype)"

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 Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

rhshadrach added a commit to rhshadrach/pandas that referenced this pull request Jan 30, 2025
@rhshadrach
Copy link
Member Author

Backport PR: #60821

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Still Needs Manual Backport Strings String extension data type and string data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants