Skip to content

Cythonize is_null_slice #55037

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

Closed
wants to merge 3 commits into from
Closed

Cythonize is_null_slice #55037

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e342258
Cythonize is_null_slice
WillAyd Sep 6, 2023
c6c3f9a
Updated typing
WillAyd Sep 6, 2023
e12f78f
typos
WillAyd Sep 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
1 change: 1 addition & 0 deletions pandas/_libs/lib.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ def is_iterator(obj: object) -> bool: ...
def is_scalar(val: object) -> bool: ...
def is_list_like(obj: object, allow_sets: bool = ...) -> bool: ...
def is_pyarrow_array(obj: object) -> bool: ...
def is_null_slice(obj: object) -> bool: ...
def is_period(val: object) -> TypeGuard[Period]: ...
def is_interval(val: object) -> TypeGuard[Interval]: ...
def is_decimal(val: object) -> TypeGuard[Decimal]: ...
Expand Down
19 changes: 19 additions & 0 deletions pandas/_libs/lib.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ from cpython.object cimport (
)
from cpython.ref cimport Py_INCREF
from cpython.sequence cimport PySequence_Check
from cpython.slice cimport PySlice_Unpack
from cpython.tuple cimport (
PyTuple_New,
PyTuple_SET_ITEM,
Expand Down Expand Up @@ -71,6 +72,7 @@ cdef extern from "Python.h":
# Note: importing extern-style allows us to declare these as nogil
# functions, whereas `from cpython cimport` does not.
bint PyObject_TypeCheck(object obj, PyTypeObject* type) nogil
cdef Py_ssize_t PY_SSIZE_T_MAX

cdef extern from "numpy/arrayobject.h":
# cython's numpy.dtype specification is incorrect, which leads to
Expand Down Expand Up @@ -1250,6 +1252,23 @@ def is_pyarrow_array(obj):
return False


def is_null_slice(obj):
"""
Return True if given object
"""
cdef Py_ssize_t start, stop, step
if isinstance(obj, slice):
try:
PySlice_Unpack(obj, &start, &stop, &step)
except TypeError:
return False

if start == 0 and stop == PY_SSIZE_T_MAX and step == 1:
Copy link
Member

Choose a reason for hiding this comment

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

will this behave differently from the status quo on e.g. slice(0, None, 1)?

Copy link
Member Author

Choose a reason for hiding this comment

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

No - PY_SSIZE_T_MAX is the largest unit used for addressing on the platform. CPython converts None to that internally, essentially letting you index as far out as your platform allows

Copy link
Member

Choose a reason for hiding this comment

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

it's not the None here that im concerned with, its the 0 and 1. On main:

In [1]: import pandas.core.common as com
In [2]: obj = slice(0, None, 1)
In [3]: com.is_null_slice(obj)
Out[3]: False

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm nice catch. That does return True here - are you thinking that is better or worse?

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 the function is intended pretty specifically to catch obj[:], so would keep the status quo behavior

Copy link
Member Author

Choose a reason for hiding this comment

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

OK cool. Thanks for catching that - I think can just close this for now then. Not a super high priority

return True

return False


_TYPE_MAP = {
"categorical": "categorical",
"category": "categorical",
Expand Down
7 changes: 1 addition & 6 deletions pandas/core/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,12 +307,7 @@ def is_null_slice(obj) -> bool:
"""
We have a null slice.
"""
return (
isinstance(obj, slice)
and obj.start is None
and obj.stop is None
and obj.step is None
)
lib.is_null_slice(obj)


def is_empty_slice(obj) -> bool:
Expand Down