BUG: combine all implementations of kurtosis and skewness computations and align results with SciPy#64366
Conversation
11a22d6 to
56add91
Compare
mroeschke
left a comment
There was a problem hiding this comment.
This is very exciting! I would love to see all our reductions implementations in nanops, groupby, and rolling use a shared implementation like this
|
Any idea why the perf hit for the slower cases? Is it a perf-vs-accuracy tradeoff? |
|
I think there's at least one other "align with scipy" PR that this will close |
I believe this also closes #53639. Thanks; please let me know if there are others.
I didn't manage to figure it out. All the regressions are focused on |
|
A hypothesis is that NumPy have optimizations for it's |
Can this cause issues such as #61031? |
No. This PR doesn't enforce contiguity in the memoryview. |
It also introduces behavior changes, mainly on degenerate distributions for nanops and window methods, now it returns NaN. Also changes the behavior on low variance windows, to make it tolerant against low variance distributions, where it no longer clips the result to 0.0.
type: fix mypy errors docs: use round in skew() and kurt() methods to ignore fperr
9378baf to
f9d932f
Compare
mroeschke
left a comment
There was a problem hiding this comment.
Generally looks good. A few follow up questions
|
On today's dev call we discussed #62536 and whether this (and follow-ups for other reductions) could make it irrelevant. Does numba do something smart that we can adapt here? |
I am not familiar with Numba to know how it compiles the code. But performance-wise it's still possible to optimize this implementation even further, mainly by adding parallelization and SIMD (Wikipedia says it's possible). If necessary, I can include a merge function in this PR that will be useful for future optimizations. |
|
I know it would break the "just one implementation", but what if we just kept the numpy version for int dtype? |
Not fond of the idea, but if it's necessary to maintain performance may be valid. Another option is to try to create moments reduction for integer types. |
|
I put up an implementation using parallelization with |
mroeschke
left a comment
There was a problem hiding this comment.
Overall I think this is a net improvement. @jbrockmendel feel free to merge if satisfied
|
Handful of small things, nothing big |
|
Thanks @Alvaro-Kothe |
Rolling.skewandRolling.kurtreturn NaN for low-variance windows #62946doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.AGENTS.md.axis={0, 1, None}Details