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.

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

PERF: Use fused types for map_infer_mask #55736

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 2 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
27 changes: 25 additions & 2 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 numeric_object_t
from pandas._libs.util cimport (
INT64_MAX,
INT64_MIN,
Expand Down Expand Up @@ -2855,12 +2856,34 @@ no_default = _NoDefault.no_default # Sentinel indicating the default value.
NoDefault = Literal[_NoDefault.no_default]


def map_infer_mask(
ndarray[object] arr,
object f,
const uint8_t[:] mask,
bint convert=True,
object na_value=no_default,
cnp.dtype dtype=np.dtype(object)
) -> np.ndarray:
dummy = np.empty(0, 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.

I think you can also just pass a pointer to avoid python runtime interaction

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'm not familiar - can you spell out the syntax here?

Copy link
Member

Choose a reason for hiding this comment

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

See if this works

http://docs.cython.org/en/latest/src/userguide/fusedtypes.html#fused-types-and-arrays

Copy link
Member Author

Choose a reason for hiding this comment

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

Cannot take address of Python variable 'dummy'

Copy link
Member

Choose a reason for hiding this comment

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

probably need to cdef it as ndarray

Copy link
Member

Choose a reason for hiding this comment

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

Yea shouldn't even need an array. Typing from phone so sorry for grammar but should be

cdef numeric_object_t dummy

Then when calling function use address of operator &dummy

The callee should have numeric_object_t *dummy as an argument

Copy link
Member Author

@rhshadrach rhshadrach Oct 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

cdef numeric_object_t dummy

I don't believe we can use fused types unless they are utilized in the args. Doing either one of these:

cdef numeric_object_t dummy
cdef ndarray[numeric_object_t] dummy

leads to compilation error Type is not specialized. Also doing

cdef ndarray dummy
cython.address(dummy)

gives Cannot take address of Python variable 'dummy' as before.

Copy link
Member

Choose a reason for hiding this comment

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

Ah OK. That's unfortunate - I think @jbrockmendel has the best suggestion then to make result caller allocated

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 think @jbrockmendel has the best suggestion then to make result caller allocated

I misunderstood that suggestion before - I think this would only be doable if we are okay with also moving convert out of the function. So instead of something like:

result = lib.map_infer_mask(arr, f, mask.view(np.uint8), map_convert)

we are now doing

result = np.empty_like(arr, dtype="object")
lib.map_infer_mask(result, arr, f, mask.view(np.uint8))
if map_convert:
	result = maybe_convert_objects(result)

This seems worse to me (duplicating the convert code), but happy to make the change if that's what's preferred by others.

Copy link
Member

Choose a reason for hiding this comment

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

this would only be doable if we are okay with also moving convert out of the function

yes im fine with this. though i think if you called the result something other than result you might not have the same TypeError issues, and since we're not iterating over it its no big deal perf-wise

result = _map_infer_mask(
arr,
f,
mask,
dummy,
convert,
na_value,
dtype,
)
return result


@cython.boundscheck(False)
@cython.wraparound(False)
def map_infer_mask(
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[object] arr,
object f,
const uint8_t[:] mask,
numeric_object_t[:] dummy,
Copy link
Member

Choose a reason for hiding this comment

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

Does changing cnp.dtype type=np.dtype(object) to numeric_object_t type = np.dtype(object) work?

I think this is a nice change but would be nice to get rid of some of the indirection

Copy link
Member

Choose a reason for hiding this comment

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

might be cleaner to create result in the calling 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.

@WillAyd: I tried this first, and it compiles but then fails when called with TypeError: an integer is required. In other places where we do this trick, e.g.

pandas/pandas/_libs/groupby.pyx

Lines 1341 to 1345 in 984d755

cdef numeric_object_t _get_min_or_max(
numeric_object_t val,
bint compute_max,
bint is_datetimelike,
):

we pass a value. That's the reason I went with an array here - we can create an empty one without needing to produce a value.

@jbrockmendel - will update.

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)

Expand Down Expand Up @@ -2888,7 +2911,7 @@ def map_infer_mask(
"""
cdef:
Py_ssize_t i, n
ndarray result
ndarray[numeric_object_t] result
Copy link
Member

Choose a reason for hiding this comment

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

can we narrow down what types are actually needed here? is there a danger of types that aren't part of numeric_object_t?

Copy link
Member Author

@rhshadrach rhshadrach Oct 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The test suite only uses bool, int64, and object. I'm going to check calls to make sure that's guaranteed. Assuming it is, does something like

ctypedef fused bool_int64_object_t:
    uint8_t
    int64_t
    object

in dtypes.pxd work?

Copy link
Member Author

Choose a reason for hiding this comment

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

All calls are either hard coded as bool, int64, or object. The only part that was a little tricky was the use in the various _str_map methods. Here, specifying the argument dtype=np.dtype(dtype) in map_infer_mask is guarded by if is_integer_dtype(dtype) or is_bool_dtype(dtype). All calls to the _str_map method are either a string dtype, int64, or bool.

Copy link
Member

Choose a reason for hiding this comment

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

If we create the type here what would we name it? I agree its good to specialize instead of using numeric_object_t but it would be nice if the name could speak to the concept of what the type supports instead of being a literal declaration of its member types

Copy link
Member Author

@rhshadrach rhshadrach Oct 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with the sentiment but have no recommendations for a "natural" name. If we are not happy with the current name uint8_int64_object_t, then the only alternative that comes to my mind is to move it from dtypes.pxd to lib.pyx just above the map_infer_mask function and name it map_infer_mask_t to highlight its local use.

Copy link
Member

Choose a reason for hiding this comment

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

current name uint8_int64_object_t [...] map_infer_mask_t to highlight its local use.

im fine with either of these options

Copy link
Member Author

Choose a reason for hiding this comment

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

@WillAyd - friendly ping on uint8_int64_object_t vs map_infer_mask_t vs something else

Copy link
Member

Choose a reason for hiding this comment

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

Let's go with the first option for now. Can always rename later

object val

n = len(arr)
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
Loading