TST/CLN: parametrize tests\resample\test_time_grouper.py #24013
Conversation
|
Hello @simonjayhawkins! Thanks for submitting the PR.
|
Codecov Report
@@ Coverage Diff @@
## master #24013 +/- ##
===========================================
- Coverage 92.31% 42.44% -49.87%
===========================================
Files 161 161
Lines 51562 51562
===========================================
- Hits 47599 21886 -25713
- Misses 3963 29676 +25713
Continue to review full report at Codecov.
|
jreback
added
Testing
| ('prod', assert_frame_equal), | ||
| ('var', assert_frame_equal), | ||
| ('std', assert_frame_equal), | ||
| ('mean', assert_frame_equal), |
There was a problem hiding this comment.
you could avoid the assert_func and just change it to do tm.assert_equal
| ('min', assert_frame_equal), | ||
| ('max', assert_frame_equal), | ||
| ('prod', assert_frame_equal), | ||
| ('var', assert_frame_equal), |
There was a problem hiding this comment.
i think you could move all of the func to a fixture in pandas/tests/resample/conftest (might be usable elsewhere)
There was a problem hiding this comment.
so that the fixture is reusable, std has been added to the downsample methods in test_base.py and sem, median, ohlc, quantile and nunique added to the test_aggregate_normal test, although ohlc fails.
pandas/tests/resample/conftest.py
Outdated
| @@ -0,0 +1,26 @@ | |||
| import pytest | |||
|
|
|||
| # The various methods we support | |||
There was a problem hiding this comment.
we might be able to move these to pandas/conftest IF they can also be used in groupby tests (but that would be in a future PR)
| @@ -26,7 +26,7 @@ | |||
|
|
|||
| # The various methods we support | |||
| downsample_methods = ['min', 'max', 'first', 'last', 'sum', 'mean', 'sem', | |||
| 'median', 'prod', 'var', 'ohlc', 'quantile'] | |||
| 'median', 'prod', 'var', 'std', 'ohlc', 'quantile'] | |||
| upsample_methods = ['count', 'size'] | |||
There was a problem hiding this comment.
are these still needed then? (as you have fixtures)
There was a problem hiding this comment.
or these are needed by current methods, until we migrate all?
There was a problem hiding this comment.
the other tests that use this are not yet parametrized. what i probably should have done was import the declarations into conftest.py to avoid duplication.
There was a problem hiding this comment.
ok, if you want to push that change would be good
|
@jreback green! |
|
thanks @simonjayhawkins |
git diff upstream/master -u -- "*.py" | flake8 --diff