Fixed bug: Dataframe.sort_values not raising ValueError for ascending-incompatible value and Series.sort_values raising ValueError for int value #42684
Conversation
gaikwadrahul20
commented
Jul 23, 2021
gaikwadrahul20
commented
- closes ENH: Raise error on ascending='False' #41634
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
- whatsnew entry
|
Apologies for any issues/mistakes. This is my first ever pull request to open source community. |
gaikwadrahul20
changed the title
|
Thanks for the review @phofl. |
| ser = Series([23, 7, 21]) | ||
| expected = np.sort(ser.values)[::-1] | ||
|
|
||
| msg = 'For argument "ascending" expected type bool, received type str.' |
There was a problem hiding this comment.
would it not be better to separate the raises ValueError test and the functional tests?
Also, would it not be better to parametrize the functional tests as follows:
@pytest.mark.parametrize("ascending", [False, 0, 1, True])
def test_sort_values_validate_ascending(self, ascending):
...
sorted_ser = ser.sort_values(ascending=ascending)
...
if ascending:
expected = expected[::-1]
...
There was a problem hiding this comment.
Thanks @attack68. I agree, code will look more clean and short. Pushed a change for this, please check.
Also, I was thinking, does it make more sense to parameterize test_sort_values_validate_ascending_for_value_error test function with msg as a parameter and move code from line 54 to 68 (code attached below) in this test?
msg = 'For argument "ascending" expected type bool, received type NoneType.'
with pytest.raises(ValueError, match=msg):
ts.sort_values(ascending=None)
msg = r"Length of ascending \(0\) must be 1 for Series"
with pytest.raises(ValueError, match=msg):
ts.sort_values(ascending=[])
msg = r"Length of ascending \(3\) must be 1 for Series"
with pytest.raises(ValueError, match=msg):
ts.sort_values(ascending=[1, 2, 3])
msg = r"Length of ascending \(2\) must be 1 for Series"
with pytest.raises(ValueError, match=msg):
ts.sort_values(ascending=[False, False])
msg = 'For argument "ascending" expected type bool, received type str.'
with pytest.raises(ValueError, match=msg):
ts.sort_values(ascending="foobar")
|
Hi @attack68, what are your thoughts on previous comment of mine? if it looks good, is it good to merge? |
| @@ -227,6 +227,8 @@ Indexing | |||
| - Bug in :meth:`Series.loc` when with a :class:`MultiIndex` whose first level contains only ``np.nan`` values (:issue:`42055`) | |||
| - Bug in indexing on a :class:`Series` or :class:`DataFrame` with a :class:`DatetimeIndex` when passing a string, the return type depended on whether the index was monotonic (:issue:`24892`) | |||
| - Bug in indexing on a :class:`MultiIndex` failing to drop scalar levels when the indexer is a tuple containing a datetime-like string (:issue:`42476`) | |||
| - Bug in :meth:`DataFrame.sort_values` and :meth:`Series.sort_values` when passing an ascending value, failed to raise or incorrectly raising ``ValueError`` (:issue:`41634`) | |||
| - Bug in updating values of :class:`pandas.Series` using boolean index, created by using :meth:`pandas.DataFrame.pop` (:issue:`42530`) | |||
There was a problem hiding this comment.
this is an extraneous note (the 2nd) that likley came in during rebase
There was a problem hiding this comment.
@jreback I think this is also taken care by you.
|
thanks @gaikwadrahul20 |
|
Thanks a lot @jreback. |
…-incompatible value and Series.sort_values raising ValueError for int value (pandas-dev#42684)
