Pandas get_dummies validate `columns` input #28463

R1j1t · 2019-09-16T15:16:31Z

closes Pandas get_dummies validate "columns" input #28383
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

The Test code is inspired from a similar tests in
RaiseError: https://github.com/pandas-dev/pandas/blob/master/pandas/tests/arrays/categorical/test_operators.py#L97
Valid list_like: https://github.com/pandas-dev/pandas/blob/master/pandas/tests/reshape/test_pivot.py#L706

This is my first pull request here and so I have taken all the necessary precautions, also I read the contributing guidelines. Not much sure of black pandas .

Problem:
If columns is not passed a list-like value a substantial decrease in performance was observed.

Proposed Solution:
As mentioned in the issue by @WillAyd that function should raise and hence based on similar situation in the codebase, I have edited the file to raise TypeError.

Added validation for the argument passed to columns

The code is inspired from a similar tests in `RaiseError`: https://github.com/pandas-dev/pandas/blob/master/pandas/tests/arrays/categorical/test_operators.py#L97 Valid `list_like`: https://github.com/pandas-dev/pandas/blob/master/pandas/tests/reshape/test_pivot.py#L706

pandas/tests/reshape/test_reshape.py

jbrockmendel · 2019-09-16T19:34:56Z

What used to happen without this change? Would a less-useful error message be raised or would you get an incorrect result? For the whatsnew, add a bullet point in the appropriate subsection in doc/source/whatsnew/v1.0.0.rst

Removed method as thier is only 1 type of call for the function `get_dummies`

Asserting list-like values for `columns` parameter in `get_dummies`

R1j1t · 2019-09-17T14:30:40Z

Doc at doc/source/whatsnew/v1.0.0.rst was updated also, I have updated the PR with problem and proposed solution.

TomAugspurger · 2019-09-17T16:52:43Z

pandas/tests/reshape/test_reshape.py

+            pd.Index(["baz", "zoo"]),
+        ],
+    )
+    def test_get_dummies_with_list_like_values(self, values):


Is this testing new behavior? Are we not already testing this?

In the file reshape.py there was no check on dtype passed to one of the parameters columns and so on the issue it was mentioned add this check. Hence, after adding the test I added these tests specifically to columns parameter. And regarding doubt whether this test was already present, I checked all tests again and there was no test to check the inputs to parameter columns and so I referred to a similar test as mentioned in PR and added it. Please let me know if you spotted a similar test for get_dummies which I might have missed. I hope this clears it.

It looks like

pandas/pandas/tests/reshape/test_reshape.py

Line 90 in 367670e

result = get_dummies(s_df, columns=s_df.columns, sparse=sparse, dtype=dtype)

tests with both columns and dtype?

The line you are have pointed to does not check for string inputs to the columns parameters also it does not test for other list-like objects passed to it.

I agree this test seems superfluous; this PR should just check for invalid values in columns.

I will remove this test. It would be great help if could you elaborate/point me to a similar test for my reference? Because as far i understand if the value is not list-like the error message will be raised because of this PR and if invalid value (i.e. value not in data columns) is passed to columns then error will be raised because value will not be present.

if the value is not list-like the error message will be raised because of this PR and if invalid value (i.e. value not in data columns) is passed to columns then error will be raised because value will not be present.

This test is redundant with

pandas/pandas/tests/reshape/test_reshape.py

Line 90 in 367670e

result = get_dummies(s_df, columns=s_df.columns, sparse=sparse, dtype=dtype)

.

We want to keep your test test_get_dummies_with_string_values, which is testing the code you've added in this PR.

TomAugspurger · 2019-09-17T16:54:02Z

pandas/core/reshape/reshape.py

@@ -863,6 +863,8 @@ def get_dummies(
        # determine columns being encoded
        if columns is None:
            data_to_encode = data.select_dtypes(include=dtypes_to_encode)
+        elif not is_list_like(columns):
+            raise TypeError("Input must be a list-like of list-likes")


The message should state which parameter is incorrect (columns?)

And is it supposed to be a list-like of list-likes? This block is just checking that it's a sequence, but not making any assertion about what each element is.

The message should state which parameter is incorrect

I have updated the Error message based on your comment but let me point out this code which was the reason I did not update the error message.

making any assertion about what each element is.

I think this is out of scope because for all the functions definitions (involving list-like object as parameters ) I referred to in pandas, none of them had this check. @jbrockmendel Is this something which should have been checked?

doc/source/whatsnew/v1.0.0.rst

Co-Authored-By: Tom Augspurger <[email protected]>

jreback · 2019-09-18T12:33:18Z

pandas/tests/reshape/test_reshape.py

+            pd.Index(["baz", "zoo"]),
+        ],
+    )
+    def test_get_dummies_with_list_like_values(self, values):


I agree this test seems superfluous; this PR should just check for invalid values in columns.

TomAugspurger

Looks like CI is failing. A whitespace issue in the whatsnew.

TomAugspurger · 2019-09-19T15:52:13Z

pandas/tests/reshape/test_reshape.py

+            pd.Index(["baz", "zoo"]),
+        ],
+    )
+    def test_get_dummies_with_list_like_values(self, values):


if the value is not list-like the error message will be raised because of this PR and if invalid value (i.e. value not in data columns) is passed to columns then error will be raised because value will not be present.

This test is redundant with

pandas/pandas/tests/reshape/test_reshape.py

Line 90 in 367670e

result = get_dummies(s_df, columns=s_df.columns, sparse=sparse, dtype=dtype)

.

We want to keep your test test_get_dummies_with_string_values, which is testing the code you've added in this PR.

based on the comments, i have removed the list-like objects check

R1j1t · 2019-09-20T07:23:31Z

As per the instructions, I have removed the test_get_dummies_with_list_like_values

WillAyd

lgtm as well - I just cleaned up some of the whatsnew whitespace ping on green

R1j1t · 2019-09-26T07:42:31Z

Any update on the PR?

jbrockmendel · 2019-09-27T17:37:12Z

needs one more rebase, otherwise looks good to go cc @jreback

WillAyd · 2019-10-11T22:19:30Z

Just fixed up the merge conflict. This should be good to go when CI is green

jbrockmendel · 2019-10-11T22:54:06Z

pandas/tests/reshape/test_reshape.py

@@ -608,6 +608,23 @@ def test_get_dummies_all_sparse(self):
        )
        tm.assert_frame_equal(result, expected)

+    @pytest.mark.parametrize("values", ["baz", "zoo"])


are these two values testing meaningfully distinct cases?

I thought of adding 2 different values just for the check but they are testing the same case. I will remove it.

Only one now!

jbrockmendel · 2019-10-11T22:54:36Z

looks like trailing whitespace is choking the linter

R1j1t · 2019-10-12T07:08:07Z

I removed trailing white spaces. Now it should pass

WillAyd · 2019-10-22T01:31:39Z

Thanks @R1j1t !

R1j1t · 2019-10-22T13:29:41Z

Thank everyone for the reviews. It was something new for me (adding test and updating docs) but I am sure this is my first of the many PR. I will start small but I love pandas and hence contributing in my free time.

R1j1t added 3 commits September 16, 2019 17:17

Updated Reshape.py to validate columns in get_dummies

87fa4ea

Added validation for the argument passed to columns

updated test_reshape.py with PEP-8 code rec

ef2d577

jbrockmendel reviewed Sep 16, 2019

View reviewed changes

pandas/tests/reshape/test_reshape.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Sep 16, 2019

View reviewed changes

pandas/tests/reshape/test_reshape.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Sep 16, 2019

View reviewed changes

pandas/tests/reshape/test_reshape.py Outdated Show resolved Hide resolved

code formated using black

faa8f07

R1j1t added 2 commits September 17, 2019 19:41

updated test_reshape.py

81c2597

Removed method as thier is only 1 type of call for the function `get_dummies`

updated file based on the changes in PR

3aa8749

Asserting list-like values for `columns` parameter in `get_dummies`

TomAugspurger reviewed Sep 17, 2019

View reviewed changes

R1j1t and others added 3 commits September 17, 2019 22:51

Update doc/source/whatsnew/v1.0.0.rst

af99037

Co-Authored-By: Tom Augspurger <[email protected]>

updated error message based on comments received

2353462

updated the test with the new error message

7d68181

jreback requested changes Sep 18, 2019

View reviewed changes

jreback added Error Reporting Incorrect or improved errors from pandas Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels Sep 18, 2019

TomAugspurger reviewed Sep 19, 2019

View reviewed changes

Removed list-like object check

ef1b9cd

based on the comments, i have removed the list-like objects check

R1j1t requested review from jreback and TomAugspurger September 20, 2019 07:44

Merge remote-tracking branch 'upstream/master' into R1j1t-master

38f13f9

TomAugspurger approved these changes Sep 23, 2019

View reviewed changes

whitespace fixup

4da4149

WillAyd approved these changes Sep 23, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into R1j1t-master

aea5019

jbrockmendel reviewed Oct 11, 2019

View reviewed changes

R1j1t added 2 commits October 12, 2019 12:34

Removed trailing space

9cd8d40

Removed redundant value in test

f266f47

WillAyd added this to the 1.0 milestone Oct 22, 2019

WillAyd merged commit fe97cad into pandas-dev:master Oct 22, 2019

HawkinsBA pushed a commit to HawkinsBA/pandas that referenced this pull request Oct 29, 2019

Pandas get_dummies validate columns input (pandas-dev#28463)

c4f0b5c

Reksbril pushed a commit to Reksbril/pandas that referenced this pull request Nov 18, 2019

Pandas get_dummies validate columns input (pandas-dev#28463)

a80e6f6

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

Pandas get_dummies validate columns input (pandas-dev#28463)

c74d0f9

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

Pandas get_dummies validate columns input (pandas-dev#28463)

340f3ca

bongolegend pushed a commit to bongolegend/pandas that referenced this pull request Jan 1, 2020

Pandas get_dummies validate columns input (pandas-dev#28463)

bef9ef3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pandas get_dummies validate `columns` input #28463

Pandas get_dummies validate `columns` input #28463

R1j1t commented Sep 16, 2019 •

edited

Loading

jbrockmendel commented Sep 16, 2019

R1j1t commented Sep 17, 2019

TomAugspurger Sep 17, 2019

R1j1t Sep 17, 2019

TomAugspurger Sep 17, 2019

R1j1t Sep 18, 2019

jreback Sep 18, 2019

R1j1t Sep 19, 2019

TomAugspurger Sep 19, 2019

TomAugspurger Sep 17, 2019

R1j1t Sep 17, 2019 •

edited

Loading

jreback Sep 18, 2019

TomAugspurger left a comment

TomAugspurger Sep 19, 2019

R1j1t commented Sep 20, 2019

WillAyd left a comment

R1j1t commented Sep 26, 2019

jbrockmendel commented Sep 27, 2019

WillAyd commented Oct 11, 2019

jbrockmendel Oct 11, 2019

R1j1t Oct 12, 2019

R1j1t Oct 12, 2019

jbrockmendel commented Oct 11, 2019

R1j1t commented Oct 12, 2019

WillAyd commented Oct 22, 2019

R1j1t commented Oct 22, 2019

Pandas get_dummies validate columns input #28463

Pandas get_dummies validate columns input #28463

Conversation

R1j1t commented Sep 16, 2019 • edited Loading

jbrockmendel commented Sep 16, 2019

R1j1t commented Sep 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

R1j1t Sep 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

R1j1t commented Sep 20, 2019

WillAyd left a comment

Choose a reason for hiding this comment

R1j1t commented Sep 26, 2019

jbrockmendel commented Sep 27, 2019

WillAyd commented Oct 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Oct 11, 2019

R1j1t commented Oct 12, 2019

WillAyd commented Oct 22, 2019

R1j1t commented Oct 22, 2019

Pandas get_dummies validate `columns` input #28463

Pandas get_dummies validate `columns` input #28463

R1j1t commented Sep 16, 2019 •

edited

Loading

R1j1t Sep 17, 2019 •

edited

Loading