FIX: Raise errors when wrong string arguments are passed to resample
#19307
Conversation
| @@ -963,6 +963,15 @@ def test_resample_basic(self): | |||
| rng = date_range('1/1/2000 00:00:00', '1/1/2000 00:13:00', freq='min', | |||
| name='index') | |||
| s = Series(np.random.randn(14), index=rng) | |||
|
|
|||
There was a problem hiding this comment.
can you add as a separate test.
jreback
added
Error Reporting
|
tests look good. pls push the fix when you can. |
pandas/core/resample.py
Outdated
| # Check for correctness of the keyword arguments which would | ||
| # otherwise silently use the default if misspelled | ||
| if label not in {None, 'left', 'right'}: | ||
| raise AttributeError('Unsupported value %s for `label`' % label) |
There was a problem hiding this comment.
These should probably be ValueErrors.
doc/source/whatsnew/v0.23.0.txt
Outdated
| @@ -485,7 +485,7 @@ Groupby/Resample/Rolling | |||
|
|
|||
| - Bug when grouping by a single column and aggregating with a class like ``list`` or ``tuple`` (:issue:`18079`) | |||
| - Fixed regression in :func:`DataFrame.groupby` which would not emit an error when called with a tuple key not in the index (:issue:`18798`) | |||
| - | |||
| - Bug in :func:`DataFrame.resample` which silently ignored unsupported (or misstyped) options for `label`, `closed` and `convention` (:issue:`19303`) | |||
There was a problem hiding this comment.
Use double backticks for label, closed and convention.
|
Hello @cchwala! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on January 21, 2018 at 15:47 Hours UTC |
Codecov Report
@@ Coverage Diff @@
## master #19307 +/- ##
=========================================
Coverage ? 91.5%
=========================================
Files ? 150
Lines ? 48745
Branches ? 0
=========================================
Hits ? 44606
Misses ? 4139
Partials ? 0
Continue to review full report at Codecov.
|
pandas/core/resample.py
Outdated
| if closed not in {None, 'left', 'right'}: | ||
| raise ValueError('Unsupported value %s for `closed`' % closed) | ||
| if convention not in {None, 'start', 'end', 'e', 's'}: | ||
| raise ValueError('Unsupported value %s for `convention`' |
There was a problem hiding this comment.
don't use '%', use .format() instead.
pandas/tests/test_resample.py
Outdated
| @@ -985,6 +986,19 @@ def test_resample_basic(self): | |||
| expect = s.groupby(grouper).agg(lambda x: x[-1]) | |||
| assert_series_equal(result, expect) | |||
|
|
|||
| def test_resample_string_kwargs(self): | |||
| rng = date_range('1/1/2000 00:00:00', '1/1/2000 00:13:00', freq='min', | |||
There was a problem hiding this comment.
add the issue number here as a comment
pandas/tests/test_resample.py
Outdated
| s = Series(np.random.randn(14), index=rng) | ||
|
|
||
| # Check that wrong keyword argument strings raise an error | ||
| with pytest.raises(ValueError) as e_info: |
There was a problem hiding this comment.
@jreback There is also a linting error here because of the unused e_info. I will also remove these, okay?
|
thanks @cchwala |
git diff upstream/master -u -- "*.py" | flake8 --diffAs a first step, the initial commit just extends the resampling tests so that they fail when a misstyped string argument, e.g. for
label, passes silently without raising.I have the fix for the code ready and will commit when the tests have failed once.