TestPeriodIndex.test_resample_empty_dataframe is failed when I pass kwargs to Resampler._groupby_and_aggregate

[discort]

• closes TestPeriodIndex.test_resample_empty_dataframe is failed when I pass kwargs to Resampler._groupby_and_aggregate #22339
• tests added / passed
• passes git diff upstream/master -u -- "*.py" | flake8 --diff

test_resample.py::TestPeriodIndex::test_resample_empty_dataframe triggers new changes

[discort]

@jreback

I investigated test_resample.py::TestPeriodIndex::test_resample_empty_dataframe test and found that Grouby.var may fail when ddof = 1. Other methods have _python_agg_general in case of exception, so I added the same behaviour for var.

Let me know if it's acceptable solution.

[codecov]

Codecov Report

Merging #22364 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #22364      +/-   ##
==========================================
+ Coverage   92.05%   92.05%   +<.01%     
==========================================
  Files         169      169              
  Lines       50709    50714       +5     
==========================================
+ Hits        46679    46684       +5     
  Misses       4030     4030

Flag	Coverage Δ
#multiple	90.46% <100%> (ø)	⬆️
#single	42.25% <0%> (-0.01%)	⬇️

Impacted Files	Coverage Δ
pandas/core/resample.py	96.08% <100%> (ø)	⬆️
pandas/core/groupby/groupby.py	96.18% <100%> (+0.02%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a2d1cf...793adcc. Read the comment docs.

[WillAyd]

This needs tests (should be created first)

[discort]

@WillAyd

After adding kwargs to _groupby_and_aggregate, changes are triggered this test test_resample.py::TestPeriodIndex::test_resample_empty_dataframe.

[WillAyd]

Hmm not sure I completely follow - was this test failing before?

[discort]

@WillAyd

Hmm not sure I completely follow - was this test failing before?

This test is failed using following changes.

diff --git a/pandas/core/resample.py b/pandas/core/resample.py
index 4a3625b2d..c82382f96 100644
--- a/pandas/core/resample.py
+++ b/pandas/core/resample.py
@@ -1043,7 +1043,7 @@ class PeriodIndexResampler(DatetimeIndexResampler):

         if is_subperiod(ax.freq, self.freq):
             # Downsampling
-            return self._groupby_and_aggregate(how, grouper=self.grouper)
+            return self._groupby_and_aggregate(how, grouper=self.grouper, **kwargs)
         elif is_superperiod(ax.freq, self.freq):
             if how == 'ohlc':
                 # GH #13083
@@ -1051,7 +1051,7 @@ class PeriodIndexResampler(DatetimeIndexResampler):
                 # for pure aggregating/reducing methods
                 # OHLC reduces along the time dimension, but creates multiple
                 # values for each period -> handle by _groupby_and_aggregate()
-                return self._groupby_and_aggregate(how, grouper=self.grouper)
+                return self._groupby_and_aggregate(how, grouper=self.grouper, **kwargs)
             return self.asfreq()
         elif ax.freq == self.freq:
             return self.asfreq()

[WillAyd]

How is this visible from the end user perspective? The test case you provided works fine on master so asserting that it continues to work doesn't measure much.

However this is visible to an end user you should set up a test case to cover that

[discort]

@WillAyd

Just wanted to make sure that we are on the same page. @jreback suggested to create a new issue for this case here. So, I created #22339 and this PR to close my original PR #22304, it's my goal at the moment.

I can make all changes within PR #22304 if it looks fine to you.

[WillAyd]

A separate PR is fine. Whatever you were planning to xfail in the other PR should be included as a test here

[discort]

@WillAyd

There is already test that covers new changes: TestPeriodIndex.test_resample_empty_dataframe. I can create a separate test for var, for instance: test_resample_empty_dataframe_var which would duplicate the logic from test_resample_empty_dataframe. Are you okay with this approach?

[discort]

follow up ^. It would be more explicit

[jreback]

@discort we haven't merged that other PR yet, so you can build on top of it is probably best. IOW, in #22304 xfail the test and in this PR remove the xfail. We want to be sure things are tested.

[jreback]

can you make this exception more specific

[discort]

Will fix within #22304

see here