PERF: Use fused types for map_infer_mask #55736
Conversation
rhshadrach
added
Performance
pandas/_libs/lib.pyx
Outdated
| ndarray[object] arr, | ||
| object f, | ||
| const uint8_t[:] mask, | ||
| numeric_object_t[:] dummy, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
might be cleaner to create result in the calling function?
There was a problem hiding this comment.
@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
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.
pandas/_libs/lib.pyx
Outdated
| np.ndarray | ||
| """ | ||
| # Passed so we can use infused types depending on the result dtype | ||
| dummy = np.empty(0, dtype=dtype) |
There was a problem hiding this comment.
I think you can also just pass a pointer to avoid python runtime interaction
There was a problem hiding this comment.
I'm not familiar - can you spell out the syntax here?
There was a problem hiding this comment.
There was a problem hiding this comment.
Cannot take address of Python variable 'dummy'
There was a problem hiding this comment.
probably need to cdef it as ndarray
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah OK. That's unfortunate - I think @jbrockmendel has the best suggestion then to make result caller allocated
There was a problem hiding this comment.
I think @jbrockmendel has the best suggestion then to make
resultcaller 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.
There was a problem hiding this comment.
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
pandas/_libs/lib.pyx
Outdated
| @@ -2888,7 +2936,7 @@ def map_infer_mask( | |||
| """ | |||
| cdef: | |||
| Py_ssize_t i, n | |||
| ndarray result | |||
| ndarray[numeric_object_t] result | |||
There was a problem hiding this comment.
can we narrow down what types are actually needed here? is there a danger of types that aren't part of numeric_object_t?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
current name
uint8_int64_object_t[...]map_infer_mask_tto highlight its local use.
im fine with either of these options
There was a problem hiding this comment.
@WillAyd - friendly ping on uint8_int64_object_t vs map_infer_mask_t vs something else
There was a problem hiding this comment.
Let's go with the first option for now. Can always rename later
…_map_infer_mask_fused
|
|
||
| @cython.boundscheck(False) | ||
| @cython.wraparound(False) | ||
| def _map_infer_mask( |
There was a problem hiding this comment.
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.
|
Can you add a whatsnew? Otherwise, LGTM. |
pandas/_libs/lib.pyx
Outdated
| const uint8_t[:] mask, | ||
| bint convert=True, | ||
| object na_value=no_default, | ||
| cnp.dtype dtype=np.dtype(object) |
There was a problem hiding this comment.
i think dtype and convert are not needed here?
There was a problem hiding this comment.
Doh - this is why you don't refactor late at night 😆 Removed, thanks.
There was a problem hiding this comment.
@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)
|
If there are no objections, I'd like to try to get this in tonight so I can rerun ASVs in #55179 |
|
Thanks @rhshadrach |
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.Just a hack at this point, seeing if this is a terrible idea. This takes us from a 40% perf hit using Cython 3.0.4 (from #55179) to a 35% perf improvement for
strings.Methods.time_len('string[python]'). I haven't run a full ASV yet, but can if this seems at all viable. I'm not certain if the fused types really cover all uses for map_infer_mask (the test suite seems to think so).cc @jbrockmendel @WillAyd
ASVs