TST: add tests for period resample sum with min_count #29762
Conversation
| @@ -694,6 +694,20 @@ def test_resample_timestamp_to_period(simple_date_range_series): | |||
| tm.assert_series_equal(result, expected) | |||
|
|
|||
|
|
|||
| def test_datetime_resample_sum_min_count(): | |||
There was a problem hiding this comment.
can you move this to the base class where its tests all of DTI, PI, TDI; pls use the fixtures.
jreback
added
Resample
| @@ -694,6 +694,20 @@ def test_resample_timestamp_to_period(simple_date_range_series): | |||
| tm.assert_series_equal(result, expected) | |||
|
|
|||
|
|
|||
| def test_datetime_resample_sum_min_count(): | |||
| # GH 19974 | |||
| index = date_range(start="2018", freq="M", periods=6) | |||
There was a problem hiding this comment.
this is not actually testing for the issue as this is a DTI and not a PI.
There was a problem hiding this comment.
Yes, this test was using DTI as that was referenced as the expected behaviour in the issue.
Not it's removed and the only test left is using PI - test_period_resample_sum_min_count in pandas/tests/resample/test_period_index.py.
ganevgv
changed the title
| @@ -548,6 +548,19 @@ def test_quarterly_resampling(self): | |||
| exp = ts.to_timestamp().resample("A").mean().to_period() | |||
| tm.assert_series_equal(result, exp) | |||
|
|
|||
| def test_period_resample_sum_min_count(self): | |||
There was a problem hiding this comment.
can this test be done more generally in test_base?
There was a problem hiding this comment.
thanks for the feedback @jreback - moved the test and generalised to include tests for DTI, PI and TDI.
pandas/tests/resample/test_base.py
Outdated
| series[:5] = 1 | ||
| series[5:] = np.nan | ||
|
|
||
| if isinstance(series.index, TimedeltaIndex) and freq != "D": |
There was a problem hiding this comment.
put this particular case in the test_timedelta.py as its only relevant there and makes the test setup way too complicated.
There was a problem hiding this comment.
makes sense - moved the tdi test to test_timedelta.py
pandas/tests/resample/test_base.py
Outdated
|
|
||
| if isinstance(series.index, DatetimeIndex) and result_name == "dti": | ||
| index = DatetimeIndex(result_index, freq=result_freq) | ||
| expected = Series(result_data, index, name=result_name) |
There was a problem hiding this comment.
you are duplicating a lot of code here, pls share
pandas/tests/resample/test_base.py
Outdated
| expected = Series(result_data, index, name=result_name) | ||
| tm.assert_series_equal(result, expected) | ||
|
|
||
| if isinstance(series.index, PeriodIndex) and result_name == "pi": |
There was a problem hiding this comment.
instead of doing this you can simply add a 'box' argument that does the appropriate conversion in the cases
e.g.
for 'pi'
PeriodIndex
and so on.
There was a problem hiding this comment.
thanks for the feedback @jreback - changed the test so that there's no duplication anymore
| @pytest.mark.parametrize( | ||
| "_index_factory,_series_name,_index_start,_index_end", [DATE_RANGE, PERIOD_RANGE] | ||
| ) | ||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
@ganevgv rather than nesting a dict, just make data, index paramters e.g.
@pytest.mark.parameterize('freq, data, index')
otherwise this is very hard to follow.
There was a problem hiding this comment.
@jreback - removed the nested dict and changed the format to:
@pytest.mark.parameterize("freq, result_name, result_data, result_index, result_freq")
3fc7c4b to
f7c1515
Compare
|
can you merge master and will look again |
ganevgv
force-pushed
the
tests/datetime_resample_sum_min_count
branch
from
f7c1515 to
3d6bfa3
Compare
|
@jreback - rebased onto latest master. |
|
@ganevgv - sorry can you merge master again - there are some conflicts |
|
superseded by #31196 |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff