Skip to content

PERF: Use fused types for map_infer_mask #55736

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.

Sign up for GitHub

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 8 commits into from
Nov 7, 2023
Merged

PERF: Use fused types for map_infer_mask #55736

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5921bf1
PERF: Use fused types for map_infer_mask
rhshadrach Oct 27, 2023
5c4d8d9
Simplify
rhshadrach Oct 27, 2023
c4bdb1a
Refactor and docstrings
rhshadrach Oct 28, 2023
c58e682
Rework
rhshadrach Nov 2, 2023
8ff9047
Merge branch 'main' of https://github.com/pandas-dev/pandas into perf…
rhshadrach Nov 2, 2023
4634a24
Remove docstring
rhshadrach Nov 2, 2023
59c836d
Cleanup, whatsnew
rhshadrach Nov 3, 2023
da55ccc
Merge branch 'main' into perf_map_infer_mask_fused
rhshadrach Nov 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions pandas/_libs/dtypes.pxd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,8 @@ ctypedef fused numeric_t:
ctypedef fused numeric_object_t:
numeric_t
object

ctypedef fused uint8_int64_object_t:
uint8_t
int64_t
object
48 changes: 36 additions & 12 deletions pandas/_libs/lib.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ cdef extern from "pandas/parser/pd_parser.h":
PandasParser_IMPORT

from pandas._libs cimport util
from pandas._libs.dtypes cimport uint8_int64_object_t
from pandas._libs.util cimport (
INT64_MAX,
INT64_MIN,
Expand Down Expand Up @@ -2856,8 +2857,6 @@ no_default = _NoDefault.no_default # Sentinel indicating the default value.
NoDefault = Literal[_NoDefault.no_default]


@cython.boundscheck(False)
@cython.wraparound(False)
def map_infer_mask(
ndarray[object] arr,
object f,
Expand All @@ -2876,24 +2875,54 @@ def map_infer_mask(
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
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
input value is used.
dtype : numpy.dtype
The numpy dtype to use for the result ndarray.

Returns
-------
np.ndarray
"""
cdef Py_ssize_t n = len(arr)
result = np.empty(n, dtype=dtype)

_map_infer_mask(
result,
arr,
f,
mask,
convert,
na_value,
dtype,
)
if convert:
return maybe_convert_objects(result)
else:
return result


@cython.boundscheck(False)
@cython.wraparound(False)
def _map_infer_mask(
Copy link
Member

Choose a reason for hiding this comment

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

make this a cdef function?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't believe so. When I try, I get:

Error compiling Cython file:
  ------------------------------------------------------------
  ...
      np.ndarray
      """
      cdef Py_ssize_t n = len(arr)
      result = np.empty(n, dtype=dtype)

      _map_infer_mask(
                    ^
  ------------------------------------------------------------

  /home/richard/dev/pandas/pandas/_libs/lib.pyx:2892:19: no suitable method found

I believe this is cython/cython#2462

I'm not certain, but I believe it happens when calling a cdef function from a def function where the def function does not use the fused types.

ndarray[uint8_int64_object_t] out,
ndarray[object] arr,
object f,
const uint8_t[:] mask,
bint convert=True,
object na_value=no_default,
cnp.dtype dtype=np.dtype(object)
Copy link
Member

Choose a reason for hiding this comment

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

i think dtype and convert are not needed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Doh - this is why you don't refactor late at night 😆 Removed, thanks.

Copy link
Member

Choose a reason for hiding this comment

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

@lithomas1 were you once looking at adding Wextra or increasing the meson warning setting? I know we'd have to do a bit more work to get there but I think that would catch issues like this in the future (via -Wunused-argument, which could alternately be added directly)

):
"""
Helper for map_infer_mask, split off to use fused types based on the result.
"""
cdef:
Py_ssize_t i, n
ndarray result
object val

n = len(arr)
result = np.empty(n, dtype=dtype)
for i in range(n):
if mask[i]:
if na_value is no_default:
Expand All @@ -2907,12 +2936,7 @@ def map_infer_mask(
# unbox 0-dim arrays, GH#690
val = val.item()

result[i] = val

if convert:
return maybe_convert_objects(result)
else:
return result
out[i] = val


@cython.boundscheck(False)
Expand Down
2 changes: 2 additions & 0 deletions pandas/core/arrays/string_.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,6 +624,8 @@ def _str_map(
na_value_is_na = isna(na_value)
if na_value_is_na:
na_value = 1
elif dtype == np.dtype("bool"):
na_value = bool(na_value)
result = lib.map_infer_mask(
arr,
f,
Expand Down