Skip to content

Refactor string methods for StringArray + return IntegerArray for numeric results #29640

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

Merged
merged 15 commits into from
Nov 19, 2019
37 changes: 35 additions & 2 deletions doc/source/user_guide/text.rst
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ Text Data Types

.. versionadded:: 1.0.0

There are two main ways to store text data
There are two ways to store text data in pandas:

1. ``object`` -dtype NumPy array.
2. :class:`StringDtype` extension type.
Expand Down Expand Up @@ -63,7 +63,40 @@ Or ``astype`` after the ``Series`` or ``DataFrame`` is created
s
s.astype("string")

Everything that follows in the rest of this document applies equally to
.. _text.differences:

Behavior differences
^^^^^^^^^^^^^^^^^^^^

These are places where the behavior of ``StringDtype`` objects differ from
``object`` dtype

l. For ``StringDtype``, :ref:`string accessor methods<api.series.str>`
that return **numeric** output will always return a nullable integer dtype,
rather than either int or float dtype, depending on the presence of NA values.

.. ipython:: python

s = pd.Series(["a", None, "b"], dtype="string")
s
s.str.count("a")
s.dropna().str.count("a")

Both outputs are ``Int64`` dtype. Compare that with object-dtype

.. ipython:: python

s.astype(object).str.count("a")
s.astype(object).dropna().str.count("a")

When NA values are present, the output dtype is float64.

2. Some string methods, like :meth:`Series.str.decode` are not available
on ``StringArray`` because ``StringArray`` only holds strings, not
bytes.


Everything else that follows in the rest of this document applies equally to
``string`` and ``object`` dtype.

.. _text.string_methods:
Expand Down
2 changes: 1 addition & 1 deletion doc/source/whatsnew/v1.0.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ Previously, strings were typically stored in object-dtype NumPy arrays.
``StringDtype`` is currently considered experimental. The implementation
and parts of the API may change without warning.

The text extension type solves several issues with object-dtype NumPy arrays:
The ``'string'`` extension type solves several issues with object-dtype NumPy arrays:
Copy link
Contributor

Choose a reason for hiding this comment

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

does this need any updating an/or reference to your new section that you added in text.rst?

Copy link
Member

Choose a reason for hiding this comment

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

@TomAugspurger you can probably update the sentence "The usual string accessor methods work. Where appropriate, the return type of the Series or columns of a DataFrame will also have string dtype." 20 lines below this line, to include that it can also return IntegerDtype in certain cases.


1. You can accidentally store a *mixture* of strings and non-strings in an
``object`` dtype array. A ``StringArray`` can only store strings.
Expand Down
24 changes: 20 additions & 4 deletions pandas/_libs/lib.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -2208,31 +2208,47 @@ def maybe_convert_objects(ndarray[object] objects, bint try_float=0,
return objects


_no_default = object()


@cython.boundscheck(False)
@cython.wraparound(False)
def map_infer_mask(ndarray arr, object f, const uint8_t[:] mask, bint convert=1):
def map_infer_mask(ndarray arr, object f, const uint8_t[:] mask, bint convert=1,
object na_value=_no_default, object dtype=object):
"""
Substitute for np.vectorize with pandas-friendly dtype inference

Parameters
----------
arr : ndarray
f : function
mask : ndarray
uint8 dtype ndarray indicating values not to apply `f` to.
convert : bool, default True
Whether to call `maybe_convert_objects` on the resulting ndarray
na_value : Any, optional
The result value to use for masked values. By default, the
input value is used
dtype : numpy.dtype
The numpy dtype to use for the result ndarray.

Returns
-------
mapped : ndarray
"""
cdef:
Py_ssize_t i, n
ndarray[object] result
ndarray result
object val

n = len(arr)
result = np.empty(n, dtype=object)
result = np.empty(n, dtype=dtype)
for i in range(n):
if mask[i]:
val = arr[i]
if na_value is _no_default:
val = arr[i]
else:
val = na_value
else:
val = f(arr[i])

Expand Down
106 changes: 95 additions & 11 deletions pandas/core/strings.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
from functools import wraps
import re
import textwrap
from typing import Dict, List
from typing import TYPE_CHECKING, Any, Callable, Dict, List
import warnings

import numpy as np
Expand All @@ -15,10 +15,14 @@
ensure_object,
is_bool_dtype,
is_categorical_dtype,
is_extension_array_dtype,
is_integer,
is_integer_dtype,
is_list_like,
is_object_dtype,
is_re,
is_scalar,
is_string_dtype,
)
from pandas.core.dtypes.generic import (
ABCDataFrame,
Expand All @@ -28,9 +32,14 @@
)
from pandas.core.dtypes.missing import isna

from pandas._typing import ArrayLike, Dtype
from pandas.core.algorithms import take_1d
from pandas.core.base import NoNewAttributesMixin
import pandas.core.common as com
from pandas.core.construction import extract_array

if TYPE_CHECKING:
from pandas.arrays import StringArray

_cpython_optimized_encoders = (
"utf-8",
Expand Down Expand Up @@ -109,9 +118,78 @@ def cat_safe(list_of_columns: List, sep: str):

def _na_map(f, arr, na_result=np.nan, dtype=object):
# should really _check_ for NA
if is_extension_array_dtype(arr.dtype):
Copy link
Contributor

Choose a reason for hiding this comment

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

can you sync the signatures up with _map, also rename _map -> _map_arraylike (or similar)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, don't follow this. I think they do match, aside from @jbrockmendel's request to call it func rather than f.

I guess na_map calls it na_result while _map calls it na_value.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am really -1 on 2 different branches here. If they have exactly the same signature a little less negative. again I would rename _map to be more informative here

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean with "-1 on 2 different branches here" ?
The whole purpose of this PR is to add a separate branch in case of StringArray (because we can be more efficient and want to be more specific in the result dtype)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's why my initial PR was so much more complex, since it tried to handle both cases similarly. I think that was more complex than this.

As Joris says, the main point of divergence is that for StringArray we usually know the result dtype exactly. It doesn't depend on the presence of NAs. Additionally,

  1. We're still using map_infer_dtype for both, so the core implementation is the same.
  2. We'll eventually deprecate .str on object-dtype, so we will end up with just this implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, at the very least these signatures for what you call _map and _stringarray_map should be exactly the same.
and _map -> _map_object and _stringarray_map -> _map_stringarray.

I think this is crucial for not adding technical debt.

Copy link
Member

Choose a reason for hiding this comment

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

@jreback is your suggestion to add na_mask to the _stringarray_map signature and have it just not be used? I think this relates to the "this should be a StringArray method" discussion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

_map and _na_map already have inconsistent signatures. I'm not sure why it's that way on master, but I'm a bit against adding unused arguments in this case.

What's the technical debt we're adding here? By definition, we need to handle StringArray differently, since its result type doesn't depend on the presence of NAs.

Copy link
Member

Choose a reason for hiding this comment

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

_map and _na_map already have inconsistent signatures. I'm not sure why it's that way on master, but I'm a bit against adding unused arguments in this case.

And there is also a good reason for that, as _map has an additional argument na_mask that is used internally in _map (for a recursive call).
I think refactoring _map is outside of the scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you create an issue to clean this up (_map and _na_map), and/or refactor of this, post this PR.

# just StringDtype
arr = extract_array(arr)
return _stringarray_map(f, arr, na_value=na_result, dtype=dtype)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rename these as indicated above.

Copy link
Member

Choose a reason for hiding this comment

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

I would rename these as indicated above.

Can you be explicit in what exact names you want to propose? (I am getting a bit lost in all the comments)
Tom already renamed _ea_map to _stringarray_map

Copy link
Contributor

Choose a reason for hiding this comment

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

right we should change _map -> _object_map (though I actually prefer the opposite, _map_object and _map_stringarray), but either is ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, make sense. I renamed to _map_object and _map_stringarray

return _map(f, arr, na_mask=True, na_value=na_result, dtype=dtype)


def _stringarray_map(
func: Callable[[str], Any], arr: "StringArray", na_value: Any, dtype: Dtype
Copy link
Member

Choose a reason for hiding this comment

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

na_value restricted to scalar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I don't think so.

) -> ArrayLike:
"""
Map a callable over valid elements of a StringArrray.

Parameters
----------
func : Callable[[str], Any]
Apply to each valid element.
arr : StringArray
na_value : Any
The value to use for missing values. By default, this is
the original value (NA).
dtype : Dtype
Copy link
Member

Choose a reason for hiding this comment

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

Elsewhere we say "np.dtype or ExtensionDtype". Is this the new policy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if we have a policy. If it matters, this is an internal docstring, so I'm OK to use things from pandas._typing that we wouldn't have in a public docstring yet.

The result dtype to use. Specifying this aviods an intermediate
object-dtype allocation.

Returns
-------
ArrayLike
An ExtensionArray for integer or string dtypes, otherwise
an ndarray.

"""
from pandas.arrays import IntegerArray, StringArray

mask = isna(arr)

assert isinstance(arr, StringArray)
arr = np.asarray(arr)

if is_integer_dtype(dtype):
na_value_is_na = isna(na_value)
if na_value_is_na:
na_value = 1
result = lib.map_infer_mask(
arr,
func,
mask.view("uint8"),
convert=False,
na_value=na_value,
dtype=np.dtype("int64"),
)

if not na_value_is_na:
mask[:] = False

return IntegerArray(result, mask)

elif is_string_dtype(dtype) and not is_object_dtype(dtype):
# i.e. StringDtype
result = lib.map_infer_mask(
arr, func, mask.view("uint8"), convert=False, na_value=na_value
)
return StringArray(result)
# TODO: BooleanArray
else:
# This is when the result type is object. We reach this when
# -> We know the result type is truly object (e.g. .encode returns bytes
# or .findall returns a list).
# -> We don't know the result type. E.g. `.get` can return anything.
return lib.map_infer_mask(arr, func, mask.view("uint8"))


def _map(f, arr, na_mask=False, na_value=np.nan, dtype=object):
if not len(arr):
return np.ndarray(0, dtype=dtype)
Expand Down Expand Up @@ -634,7 +712,7 @@ def str_replace(arr, pat, repl, n=-1, case=None, flags=0, regex=True):
raise ValueError("Cannot use a callable replacement when regex=False")
f = lambda x: x.replace(pat, repl, n)

return _na_map(f, arr)
return _na_map(f, arr, dtype=str)


def str_repeat(arr, repeats):
Expand Down Expand Up @@ -685,7 +763,7 @@ def scalar_rep(x):
except TypeError:
return str.__mul__(x, repeats)

return _na_map(scalar_rep, arr)
return _na_map(scalar_rep, arr, dtype=str)
else:

def rep(x, r):
Expand Down Expand Up @@ -1150,7 +1228,7 @@ def str_join(arr, sep):
4 NaN
dtype: object
"""
return _na_map(sep.join, arr)
return _na_map(sep.join, arr, dtype=str)


def str_findall(arr, pat, flags=0):
Expand Down Expand Up @@ -1381,7 +1459,7 @@ def str_pad(arr, width, side="left", fillchar=" "):
else: # pragma: no cover
raise ValueError("Invalid side")

return _na_map(f, arr)
return _na_map(f, arr, dtype=str)


def str_split(arr, pat=None, n=None):
Expand Down Expand Up @@ -1487,7 +1565,7 @@ def str_slice(arr, start=None, stop=None, step=None):
"""
obj = slice(start, stop, step)
f = lambda x: x[obj]
return _na_map(f, arr)
return _na_map(f, arr, dtype=str)


def str_slice_replace(arr, start=None, stop=None, repl=None):
Expand Down Expand Up @@ -1578,7 +1656,7 @@ def f(x):
y += x[local_stop:]
return y

return _na_map(f, arr)
return _na_map(f, arr, dtype=str)


def str_strip(arr, to_strip=None, side="both"):
Expand All @@ -1603,7 +1681,7 @@ def str_strip(arr, to_strip=None, side="both"):
f = lambda x: x.rstrip(to_strip)
else: # pragma: no cover
raise ValueError("Invalid side")
return _na_map(f, arr)
return _na_map(f, arr, dtype=str)


def str_wrap(arr, width, **kwargs):
Expand Down Expand Up @@ -1667,7 +1745,7 @@ def str_wrap(arr, width, **kwargs):

tw = textwrap.TextWrapper(**kwargs)

return _na_map(lambda s: "\n".join(tw.wrap(s)), arr)
return _na_map(lambda s: "\n".join(tw.wrap(s)), arr, dtype=str)


def str_translate(arr, table):
Expand All @@ -1687,7 +1765,7 @@ def str_translate(arr, table):
-------
Series or Index
"""
return _na_map(lambda x: x.translate(table), arr)
return _na_map(lambda x: x.translate(table), arr, dtype=str)


def str_get(arr, i):
Expand Down Expand Up @@ -3025,7 +3103,7 @@ def normalize(self, form):
import unicodedata

f = lambda x: unicodedata.normalize(form, x)
result = _na_map(f, self._parent)
result = _na_map(f, self._parent, dtype=str)
return self._wrap_result(result)

_shared_docs[
Expand Down Expand Up @@ -3223,31 +3301,37 @@ def rindex(self, sub, start=0, end=None):
lambda x: x.lower(),
name="lower",
docstring=_shared_docs["casemethods"] % _doc_args["lower"],
dtype=str,
)
upper = _noarg_wrapper(
lambda x: x.upper(),
name="upper",
docstring=_shared_docs["casemethods"] % _doc_args["upper"],
dtype=str,
)
title = _noarg_wrapper(
lambda x: x.title(),
name="title",
docstring=_shared_docs["casemethods"] % _doc_args["title"],
dtype=str,
)
capitalize = _noarg_wrapper(
lambda x: x.capitalize(),
name="capitalize",
docstring=_shared_docs["casemethods"] % _doc_args["capitalize"],
dtype=str,
)
swapcase = _noarg_wrapper(
lambda x: x.swapcase(),
name="swapcase",
docstring=_shared_docs["casemethods"] % _doc_args["swapcase"],
dtype=str,
)
casefold = _noarg_wrapper(
lambda x: x.casefold(),
name="casefold",
docstring=_shared_docs["casemethods"] % _doc_args["casefold"],
dtype=str,
)

_shared_docs[
Expand Down
Loading