Skip to content

CLN: use cimports for type checks #56002

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 1 commit into from
Nov 17, 2023

Conversation

jbrockmendel
Copy link
Member

The generated C corresponding to the cnp.integer check in is_integer_object is __Pyx_TypeCheck(__pyx_v_obj, __pyx_ptype_5numpy_integer) which looks to me like it goes through python-space paths, can you comment @WillAyd ?

Despite that, perf seems neutral-to-improved:

import numpy as np
from pandas._libs.lib import is_integer

item1 = 2
item2 = np.int64(1)
item3 = "foo"

%timeit is_integer(item1)
63.6 ns ± 1.17 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)  # <- main
62.9 ns ± 0.992 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)  # <- PR

%timeit is_integer(item2)
70.5 ns ± 2.78 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)  # <- main
68.9 ns ± 1.37 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)  # <- PR

%timeit is_integer(item3)
64.7 ns ± 2.95 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)  # <- main
61.6 ns ± 0.469 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)  # <- PR

Also for @WillAyd: we could make similar simplifications in util.is_float_object and util.is_complex_object but that would require removing their nogilness. IIRC that was something you thought we should do regardless?

@WillAyd
Copy link
Member

WillAyd commented Nov 17, 2023

__Pyx_TypeCheck(__pyx_v_obj, __pyx_ptype_5numpy_integer) which looks to me like it goes through python-space paths, can you comment @WillAyd ?

Not sure - would have to see how Pyx_Type_Check is implemented. I suppose it could just use C-level struct access, but likely defers to PyObject_IsInstance at some point

This PR looks great. I am of the opinion that nogil is not worth jumping through hoops for if we cannot measure an improvement with it

@mroeschke mroeschke added this to the 2.2 milestone Nov 17, 2023
@mroeschke mroeschke merged commit bad1e09 into pandas-dev:main Nov 17, 2023
@mroeschke
Copy link
Member

Thanks @jbrockmendel

@jbrockmendel jbrockmendel deleted the cnp-is-checks branch November 17, 2023 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants