REF: Use moments accumulator from libalgos to compute variance - #66302
REF: Use moments accumulator from libalgos to compute variance#66302Alvaro-Kothe wants to merge 7 commits into
Conversation
a70538c to
55b85c0
Compare
e3cec07 to
dd294b1
Compare
| - APIs that accept an ``engine="numba"`` parameter with ``engine_kwargs`` will no longer pass through a ``nopython`` argument to ``numba.jit``. This argument has had no effect since numba 0.59.0 (:issue:`64483`). | ||
| - Removed the ``freq`` and ``freqstr`` attributes from :class:`.DatetimeArray` and :class:`.TimedeltaArray`. Frequency is now stored only on :class:`DatetimeIndex` and :class:`TimedeltaIndex`; access ``Series.dt.freq`` or wrap the array in an Index to retrieve a frequency. The ``check_freq`` keyword on :func:`testing.assert_extension_array_equal` for these array types has also been removed (:issue:`24566`). | ||
| - | ||
| - :meth:`DataFrame.var`, :meth:`DataFrame.std`, :meth:`DataFrame.sem`, :meth:`DataFrame.skew` and :meth:`DataFrame.kurt` with ``numeric_only=False`` now converts numeric strings to their float representation for its intermediate computations (:issue:`64366`, :issue:`66302`) |
There was a problem hiding this comment.
is this API change central to the refactor here? i dont think its a change we want to make
There was a problem hiding this comment.
There is a requirement that the array must be of floats. So, somewhere we must convert the array datatype to np.float64.
There are some ways to avoid the behavior change:
- Keep and use the old implementation for
objectdtype. - Use
_ensure_numericto emit an error if it finds a string in the array before trying to convert to float64. The main problem in using this function is the error message that prints the whole array and due to error message mismatch, several tests fail.
In the current implementation, it converts numeric strings when computing avg , but fails when computing the central differences and this is the error message asserted in test_numeric_only_flag.
Whatever we decide here must also apply to nankurt and nanskew.
There was a problem hiding this comment.
i lean toward a consistent _ensure_numeric call across the board. Though i agree printing the whole array doesn't make sense
There was a problem hiding this comment.
Updated the code and the benchmark for the case without bottleneck. LMK if the performance regression reported in there is acceptable. Otherwise, we can just patch the skewness and kurtosis case - I opened a PR for them in #66495.
d9bb1fc to
0240c2e
Compare
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.AGENTS.md.This is a continuation of #64848. It wasn't possible to reopen that, so I am opening this new PR instead.
Use the moments accumulator functions introduced in #64366 and #65801 to compute variance, standard error and standard error of the mean.
The performance increase for
varandstdonly applies if bottleneck isn't installed (or is disabled). Otherwise, it prefers the bottleneck implementation.Benchmarks
without bottleneck
Details
with bottleneck
Details