-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
PERF: Improve performance in rolling.mean(engine="numba") #43612
Conversation
how does this compare to the cython mean? |
|
is there an issue somewhere for discussing making numba required and just using this instead of the cython versions? |
Not an ongoing issue, but the last time it was discussed was in #28987. Looks like a lot of the discussion back then revolved around stability & maturity but those issues may not be as bad anymore. |
we ought to have this discussion as that would greatly simplify code generally. this is a good start though. |
pandas/core/_numba/kernels.py
Outdated
|
||
|
||
@numba.jit(nopython=True, nogil=True, parallel=False) | ||
def is_monotonic_increasing(bounds): |
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.
similar questions about typing (even if it doesn't actually help perf we should do it)
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.
looks good
@@ -0,0 +1 @@ | |||
from pandas.core._numba.kernels.mean_ import sliding_mean # noqa:F401 |
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.
alt can use __all__
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.
Added
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.
cc @pandas-dev/pandas-core if any comments
pandas/core/_numba/kernels/mean_.py
Outdated
@numba.jit(nopython=True, nogil=True, parallel=False) | ||
def is_monotonic_increasing(bounds: np.ndarray) -> bool: | ||
n = len(bounds) | ||
if n == 1: |
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.
I don't understand this block. n==1 and n < 2 -> n==0?
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.
Good point I was able to simplify this block.
pandas/core/_numba/kernels/mean_.py
Outdated
min_periods: int, | ||
) -> np.ndarray: | ||
N = len(start) | ||
nobs = 0.0 |
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.
Could nobs ever overflow int64
or uint64
?
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.
I suppose with sufficient observations (nobs
) this could overflow, but this value should be less than or equal to the window size so the user would also have to provide a window size that overflows u/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.
There is an actual build for the maximum size of a NumPy array, np.intp
. This is int64
on Windows64 and Linux, and I suspect on OSX. It seems that nobs should be in integer which should be slightly faster than a float.
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.
Agreed, changed nobs to a int.
pandas/core/_numba/executor.py
Outdated
end: np.ndarray, | ||
min_periods: int, | ||
): | ||
result = np.empty((len(start), values.shape[1])) |
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.
can dtype by specified?
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.
Good point. Added dtype.
pandas/core/_numba/kernels/mean_.py
Outdated
def is_monotonic_increasing(bounds: np.ndarray) -> bool: | ||
n = len(bounds) | ||
if n == 1: | ||
return bounds[0] == bounds[0] |
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.
Is this to stop single element NaN
sequences from being monotonic increasing?
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.
I believe so, yes. This snippet was taken from translating this function, but I was able to remove this condition since we know the inputs should be int64
s will no NaN
s
Line 792 in ae049ae
if n == 1: |
thanks @mroeschke |
This also starts to add a shared aggregation function (mean) that can shared between rolling/groupby/DataFrame when using the numba engine.