BUG: various groupby ewm times issues #40952

stevenschaerer · 2021-04-14T19:55:40Z

closes BUG: Issues with groupby ewm and times #40951
tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry

…g support for times in numba implementation; fix bug in cython implementation

jreback · 2021-04-14T20:01:17Z

mroeschke · 2021-04-14T22:22:48Z

pandas/_libs/window/aggregations.pyx

@@ -1511,6 +1511,7 @@ def ewma(const float64_t[:] vals, const int64_t[:] start, const int64_t[:] end,
        s = start[j]
        e = end[j]
        sub_vals = vals[s:e]
+        sub_deltas = deltas[s:e-1]


Nit: e - 1

mroeschke · 2021-04-14T22:25:21Z

pandas/core/window/ewm.py

+            )
+            # error: Item "str" of "Union[str, ndarray, FrameOrSeries]" has no
+            # attribute "take"
+            self.times = self.times.take(groupby_order)


Ideally we don't want to change this attribute too much since its still public to the user.

Instead could you define _calculate_deltas(self.times.take(groupby_order), self.halflife)

mroeschke · 2021-04-14T22:29:51Z

pandas/tests/window/test_groupby.py

@@ -926,3 +926,71 @@ def test_pairwise_methods(self, method, expected_data):

        expected = df.groupby("A").apply(lambda x: getattr(x.ewm(com=1.0), method)())
        tm.assert_frame_equal(result, expected)
+
+    def test_times(self):


Could you split this into 3 different tests? Some duplication is okay but nice to have each test case isolated.

mroeschke

Looks pretty good, thanks for catching these.

Could you add a whatsnew note for v1.3? Can have 3 separate entries all linking to you original issue

stevenschaerer · 2021-04-15T12:37:31Z

Thanks for your comments, I've incorporated them into the PR. Please have a look when you get a chance.

From what I can tell the failing checks seem to be due to test_empty_multi in test_reductions.py. I saw another recent PR with the same issue (https://github.com/pandas-dev/pandas/actions/runs/751397894). Is there a way to re-run the checks without pushing a commit?

mroeschke · 2021-04-15T18:48:45Z

doc/source/whatsnew/v1.3.0.rst

@@ -792,6 +792,9 @@ Groupby/resample/rolling
 - Bug in :class:`core.window.RollingGroupby` where ``as_index=False`` argument in ``groupby`` was ignored (:issue:`39433`)
 - Bug in :meth:`.GroupBy.any` and :meth:`.GroupBy.all` raising ``ValueError`` when using with nullable type columns holding ``NA`` even with ``skipna=True`` (:issue:`40585`)
 - Bug in :meth:`GroupBy.cummin` and :meth:`GroupBy.cummax` incorrectly rounding integer values near the ``int64`` implementations bounds (:issue:`40767`)
+- Bug in :meth:`core.window.ewm.ExponentialMovingWindowGroupby.mean` where the times argument was ignored in the numba implementation (:issue:`40951`)


in the numba implementation -> when engine='numba'

mroeschke · 2021-04-15T18:49:28Z

doc/source/whatsnew/v1.3.0.rst

@@ -792,6 +792,9 @@ Groupby/resample/rolling
 - Bug in :class:`core.window.RollingGroupby` where ``as_index=False`` argument in ``groupby`` was ignored (:issue:`39433`)
 - Bug in :meth:`.GroupBy.any` and :meth:`.GroupBy.all` raising ``ValueError`` when using with nullable type columns holding ``NA`` even with ``skipna=True`` (:issue:`40585`)
 - Bug in :meth:`GroupBy.cummin` and :meth:`GroupBy.cummax` incorrectly rounding integer values near the ``int64`` implementations bounds (:issue:`40767`)
+- Bug in :meth:`core.window.ewm.ExponentialMovingWindowGroupby.mean` where the times argument was ignored in the numba implementation (:issue:`40951`)
+- Bug in :meth:`core.window.ewm.ExponentialMovingWindowGroupby.mean` where the wrong times were used in the cython implementation in case of multiple groups (:issue:`40951`)


Can remove in the cython implementation

mroeschke · 2021-04-15T18:51:11Z

A few small whatsnew comments otherwise LGTM (the follow up commit should hopefully clear the CI)

jreback · 2021-04-16T00:37:39Z

doc/source/whatsnew/v1.3.0.rst

@@ -792,6 +792,9 @@ Groupby/resample/rolling
 - Bug in :class:`core.window.RollingGroupby` where ``as_index=False`` argument in ``groupby`` was ignored (:issue:`39433`)
 - Bug in :meth:`.GroupBy.any` and :meth:`.GroupBy.all` raising ``ValueError`` when using with nullable type columns holding ``NA`` even with ``skipna=True`` (:issue:`40585`)
 - Bug in :meth:`GroupBy.cummin` and :meth:`GroupBy.cummax` incorrectly rounding integer values near the ``int64`` implementations bounds (:issue:`40767`)
+- Bug in :meth:`core.window.ewm.ExponentialMovingWindowGroupby.mean` where the times argument was ignored when ``engine='numba'`` (:issue:`40951`)


I think we have another tweek for ewm.mean can you move these notes below that one

jreback · 2021-04-16T00:38:11Z

pandas/_libs/window/aggregations.pyx

@@ -1511,6 +1511,7 @@ def ewma(const float64_t[:] vals, const int64_t[:] start, const int64_t[:] end,
        s = start[j]
        e = end[j]
        sub_vals = vals[s:e]
+        sub_deltas = deltas[s:e - 1]


why is this not s:e? (like sub_vals)

Deltas are the (scaled) np.diff of the times vector. so len(deltas) = len(times) - 1 = len(vals) - 1.

kk can you add this comment on the shape of sub_vals vs sub_deltas for future readers

jreback · 2021-04-16T00:38:56Z

pandas/core/window/ewm.py

+            times.view(np.int64), dtype=np.float64  # type: ignore[union-attr]
+        )
+        _halflife = float(Timedelta(halflife).value)
+        self._deltas = np.diff(_times) / _halflife


can you return _deltas and assign in the caller (change the return type as well)

jreback · 2021-04-16T00:39:12Z

pandas/core/window/ewm.py

+        times: str | np.ndarray | FrameOrSeries | None,
+        halflife: float | TimedeltaConvertibleTypes | None,
+    ) -> None:
+        # error: Item "str" of "Union[str, ndarray, FrameOrSeries, None]" has no


can you give a doc-string, describe the params and what this is doing

jreback · 2021-04-16T00:39:34Z

pandas/core/window/numba_.py

@@ -141,7 +142,7 @@ def groupby_ewma(

                    if is_observation or not ignore_na:

-                        old_wt *= old_wt_factor
+                        old_wt *= old_wt_factor ** deltas[start + j - 1]


We want to use delta[i-1] with values[i]. cur = window[j] = values[start + j] (and j starts at 1).

I could introduce a new variable delta_window = deltas[start:stop-1] as in the cython implementation if you think that would make it clearer.

no its fine, if you'd just add some documentation to this effect for future readers

jreback

ok looks good, 2 requests. ping on green.

jreback · 2021-04-16T15:58:00Z

pandas/_libs/window/aggregations.pyx

@@ -1511,6 +1511,7 @@ def ewma(const float64_t[:] vals, const int64_t[:] start, const int64_t[:] end,
        s = start[j]
        e = end[j]
        sub_vals = vals[s:e]
+        sub_deltas = deltas[s:e - 1]


kk can you add this comment on the shape of sub_vals vs sub_deltas for future readers

jreback · 2021-04-16T15:58:15Z

pandas/core/window/ewm.py

@@ -303,6 +295,38 @@ def __init__(
                self.alpha,
            )

+    @staticmethod


just make this a module level function

jreback · 2021-04-16T15:58:46Z

pandas/core/window/numba_.py

@@ -141,7 +142,7 @@ def groupby_ewma(

                    if is_observation or not ignore_na:

-                        old_wt *= old_wt_factor
+                        old_wt *= old_wt_factor ** deltas[start + j - 1]


no its fine, if you'd just add some documentation to this effect for future readers

stevenschaerer · 2021-04-16T17:43:34Z

@jreback: I have incorporated your latest comments. CI is green.

mroeschke · 2021-04-16T18:43:09Z

Thanks @stevenschaerer nice patch!

* times in ewm groupby: sort times in according to grouping; add missing support for times in numba implementation; fix bug in cython implementation * add GH issue id to tests * fix typing validation error * PR comments * trying to fix int64 to int32 casting TypeError * PR comments * PR comments * PR comments

stevenschaerer added 2 commits April 14, 2021 20:54

times in ewm groupby: sort times in according to grouping; add missin…

e7a9d31

…g support for times in numba implementation; fix bug in cython implementation

add GH issue id to tests

8e3ed3b

fix typing validation error

b3e7e99

mroeschke reviewed Apr 14, 2021

View reviewed changes

mroeschke added Bug Window rolling, ewma, expanding labels Apr 14, 2021

stevenschaerer added 2 commits April 15, 2021 11:37

PR comments

b7f38e3

trying to fix int64 to int32 casting TypeError

e1ecb58

mroeschke reviewed Apr 15, 2021

View reviewed changes

PR comments

c5e80d1

mroeschke approved these changes Apr 15, 2021

View reviewed changes

mroeschke added this to the 1.3 milestone Apr 16, 2021

jreback requested changes Apr 16, 2021

View reviewed changes

PR comments

91f7390

jreback requested changes Apr 16, 2021

View reviewed changes

PR comments

f2dc759

mroeschke merged commit ece1217 into pandas-dev:master Apr 16, 2021

stevenschaerer deleted the gh40951-fix-groupby-ewm-times branch April 16, 2021 18:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: various groupby ewm times issues #40952

BUG: various groupby ewm times issues #40952

stevenschaerer commented Apr 14, 2021 •

edited

Loading

jreback commented Apr 14, 2021

mroeschke Apr 14, 2021

mroeschke Apr 14, 2021

mroeschke Apr 14, 2021

mroeschke left a comment

stevenschaerer commented Apr 15, 2021

mroeschke Apr 15, 2021

mroeschke Apr 15, 2021

mroeschke commented Apr 15, 2021 •

edited

Loading

jreback Apr 16, 2021

jreback Apr 16, 2021

stevenschaerer Apr 16, 2021

jreback Apr 16, 2021

jreback Apr 16, 2021

jreback Apr 16, 2021

jreback Apr 16, 2021

stevenschaerer Apr 16, 2021

jreback Apr 16, 2021

jreback left a comment

jreback Apr 16, 2021

jreback Apr 16, 2021

jreback Apr 16, 2021

stevenschaerer commented Apr 16, 2021

mroeschke commented Apr 16, 2021

BUG: various groupby ewm times issues #40952

BUG: various groupby ewm times issues #40952

Conversation

stevenschaerer commented Apr 14, 2021 • edited Loading

jreback commented Apr 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mroeschke left a comment

Choose a reason for hiding this comment

stevenschaerer commented Apr 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mroeschke commented Apr 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenschaerer commented Apr 16, 2021

mroeschke commented Apr 16, 2021

stevenschaerer commented Apr 14, 2021 •

edited

Loading

mroeschke commented Apr 15, 2021 •

edited

Loading