-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
PERF: Avoid np.divmod in maybe_sequence_to_range #57812
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
Changes from 3 commits
9b18b6b
9bcc4ea
c9339d5
1e98a41
0b484bd
981e3c9
997416f
d3ae4e9
b8ea98c
21f13d9
2c77071
3daca65
339a914
503d96f
58a6f27
5f31dd8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -678,6 +678,26 @@ def is_range_indexer(ndarray[int6432_t, ndim=1] left, Py_ssize_t n) -> bool: | |
return True | ||
|
||
|
||
@cython.wraparound(False) | ||
@cython.boundscheck(False) | ||
def is_range(ndarray[int6432_t, ndim=1] sequence, int64_t diff) -> bool: | ||
WillAyd marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can this have a more verbose-but-descriptive name? from this name alone id expect this to be a somehow-optimized There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure thing. Renamed to |
||
""" | ||
Check if sequence is equivalent to a range with the specified diff. | ||
""" | ||
cdef: | ||
Py_ssize_t i, n = len(sequence) | ||
|
||
if diff == 0: | ||
return False | ||
|
||
for i in range(n): | ||
|
||
if sequence[i] != sequence[0] + i * diff: | ||
return False | ||
|
||
return True | ||
|
||
|
||
ctypedef fused ndarr_object: | ||
ndarray[object, ndim=1] | ||
ndarray[object, ndim=2] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don’t we only need int64?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes we'll be calling
self._shallow_copy(Index[int]._values)
so I suppose theint
type could be non-int64There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe should just use
np.intp
? That matches what range would use internallyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like
np.intp
didn't work for the 32 bit and Windows build 0b484bdThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you seeing build failures just doing what @jbrockmendel suggests with int64 then? The error message
ValueError: Buffer dtype mismatch, expected 'intp_t' but got 'long long'
on the 32 bit build would seemingly indicate the 32 bit part of the fused type is unused, sincelong long
is by definition at least 64 bitshttps://github.com/pandas-dev/pandas/actions/runs/8268872377/job/22622796389#step:4:43677
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With just
int64_t
, 32 bit and Windows builds are failing withValueError: Buffer dtype mismatch, expected 'int64_t' but got 'long'
https://github.com/pandas-dev/pandas/actions/runs/8284194543/job/22669177062?pr=57812