BUG: correct wrong error message in df.pivot when columns=None #30925

charlesdong1991 · 2020-01-11T16:33:22Z

closes BUG: Wrong error message is raised when columns=None in df.pivot #30924
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

WillAyd · 2020-01-11T23:23:39Z

Not at a computer to check but why is this not optional? Wouldn't not specifying it just return a Series?

charlesdong1991 · 2020-01-12T08:40:34Z

emm, not sure about this behaviour, but from documentation, it is supposed to return a reshaped DataFrame, so probably this is why this is not optional @WillAyd

simonjayhawkins · 2020-01-13T14:07:11Z

pandas/core/reshape/pivot.py

@@ -427,6 +427,9 @@ def _convert_by(by):
 @Substitution("\ndata : DataFrame")
 @Appender(_shared_docs["pivot"], indents=1)
 def pivot(data: "DataFrame", index=None, columns=None, values=None) -> "DataFrame":
+    if columns is None:
+        raise ValueError("`columns` is not optional.")


since I don't think we can change the function signature, then to be semi consistent with standard error messages, then maybe TypeError: pivot() missing 1 required argument: 'columns'

(or an enhancement to the code so that columns is the remaining columns not in index and values arguments, and then raise a message that at least one of columns and values must be specified

simonjayhawkins · 2020-01-13T17:07:13Z

pandas/tests/reshape/test_pivot.py

+        df = pd.DataFrame(
+            {"col1": ["a", "b", "c"], "col2": [1, 2, 3], "col3": [1, 2, 3]}
+        )
+        msg = 'pivot["(][")] missing 1 required argument: ["\']columns["\']'


if you use re.escape, this may be more readable.

thanks for your quick response, i tried re.escape, but it seemed to work in an opposite way. @simonjayhawkins could you pls enlighten me a bit?

WillAyd · 2020-01-13T17:14:43Z

OK I think I see the point here. If you can address @simonjayhawkins comments and put a note in for 1.0.1 when available I think should be good

simonjayhawkins · 2020-01-13T17:20:13Z

OK I think I see the point here. If you can address @simonjayhawkins comments and put a note in for 1.0.1 when available I think should be good

@WillAyd maybe i missed the discussion, but the first patch release following 1.0.0 will be for regressions only. This should be 1.1.0?

WillAyd · 2020-01-13T17:21:12Z

Yea makes sense. Let's do 1.1 instead (comment still applies)

simonjayhawkins · 2020-01-13T17:27:43Z

pandas/tests/reshape/test_pivot.py

+        df = pd.DataFrame(
+            {"col1": ["a", "b", "c"], "col2": [1, 2, 3], "col3": [1, 2, 3]}
+        )
+        msg = 'pivot["(][")] missing 1 required argument: ["\']columns["\']'


Suggested change

msg = 'pivot["(][")] missing 1 required argument: ["\']columns["\']'

msg = re.escape("pivot() missing 1 required argument: 'columns'")

also need to import re

ahh, i see!!! thanks! @simonjayhawkins

charlesdong1991 · 2020-01-13T17:39:03Z

thanks @WillAyd and @simonjayhawkins I will add a whatsnew note once 1.1 is there

charlesdong1991 · 2020-01-14T07:57:46Z

ping @WillAyd @simonjayhawkins

simonjayhawkins · 2020-01-14T21:46:50Z

Thanks @charlesdong1991 lgtm. @WillAyd

WillAyd

@jreback in case there's any historical reason not to do this, but otherwise looks close

WillAyd · 2020-01-14T23:15:53Z

pandas/core/reshape/pivot.py

@@ -427,6 +427,9 @@ def _convert_by(by):
 @Substitution("\ndata : DataFrame")
 @Appender(_shared_docs["pivot"], indents=1)
 def pivot(data: "DataFrame", index=None, columns=None, values=None) -> "DataFrame":
+    if columns is None:
+        raise ValueError("pivot() missing 1 required argument: 'columns'")


This should be a TypeError no? I think @simonjayhawkins had that original comment

sorry, i thought ValueError is more appropriate here for this case.

I changed to TypeError as suggested by @simonjayhawkins ! thanks both!

This should be a TypeError no? I think @simonjayhawkins had that original comment

I think it should be TypeError, however, I would regard a change from ValueError to TypeError as a breaking change. Not sure what others think.

nvm. The exception in the issue is a KeyError, so either is a change.

get it! thanks!

charlesdong1991 · 2020-01-15T08:30:25Z

ping @jreback

WillAyd · 2020-01-17T23:47:55Z

pandas/tests/reshape/test_pivot.py

+        df = pd.DataFrame(
+            {"col1": ["a", "b", "c"], "col2": [1, 2, 3], "col3": [1, 2, 3]}
+        )
+        msg = re.escape("pivot() missing 1 required argument: 'columns'")


Suggested change

msg = re.escape("pivot() missing 1 required argument: 'columns'")

msg = r"pivot\(\) missing 1 required argument: 'columns'"

Minor nit but just use raw strings no need to import re for this

WillAyd · 2020-01-17T23:48:08Z

doc/source/whatsnew/v1.1.0.rst

@@ -141,6 +141,8 @@ Reshaping

 -
 - Bug in :meth:`DataFrame.pivot_table` when only MultiIndexed columns is set (:issue:`17038`)
+- Correct incorrect error message in :meth:`DataFrame.pivot` when ``columns`` is set to ``None``. (:issue:`30924`)


Suggested change

- Correct incorrect error message in :meth:`DataFrame.pivot` when ``columns`` is set to ``None``. (:issue:`30924`)

- Fix incorrect error message in :meth:`DataFrame.pivot` when ``columns`` is set to ``None``. (:issue:`30924`)

Just reads a little better

charlesdong1991 · 2020-01-18T14:56:28Z

ping @WillAyd

jreback · 2020-01-18T16:23:36Z

thanks @charlesdong1991

charlesdong1991 added 5 commits December 3, 2018 17:43

remove \n from docstring

7e461a1

fix conflicts

1314059

Merge remote-tracking branch 'upstream/master'

8bcb313

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

776e9ae

correct error message

4895497

WillAyd added the Reshaping Concat, Merge/Join, Stack/Unstack, Explode label Jan 11, 2020

charlesdong1991 requested a review from WillAyd January 13, 2020 08:02

simonjayhawkins reviewed Jan 13, 2020

View reviewed changes

code change on Simon review

f4380cd

simonjayhawkins reviewed Jan 13, 2020

View reviewed changes

simonjayhawkins added the Error Reporting Incorrect or improved errors from pandas label Jan 13, 2020

simonjayhawkins added this to the 1.1 milestone Jan 13, 2020

charlesdong1991 added 3 commits January 13, 2020 19:03

update test

e60ffee

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

61ac061

add whatsnew

d03452f

charlesdong1991 requested a review from simonjayhawkins January 14, 2020 07:57

simonjayhawkins approved these changes Jan 14, 2020

View reviewed changes

WillAyd requested changes Jan 14, 2020

View reviewed changes

change to typeerror and resolve conflict and merge master

abce670

WillAyd requested changes Jan 17, 2020

View reviewed changes

charlesdong1991 added 3 commits January 18, 2020 11:32

fix conflict

28d3d41

better whatsnew

c650a05

remove re

16478e5

jreback merged commit f792d8c into pandas-dev:master Jan 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: correct wrong error message in df.pivot when columns=None #30925

BUG: correct wrong error message in df.pivot when columns=None #30925

charlesdong1991 commented Jan 11, 2020 •

edited

Loading

WillAyd commented Jan 11, 2020

charlesdong1991 commented Jan 12, 2020 •

edited

Loading

simonjayhawkins Jan 13, 2020

simonjayhawkins Jan 13, 2020

charlesdong1991 Jan 13, 2020

WillAyd commented Jan 13, 2020

simonjayhawkins commented Jan 13, 2020

WillAyd commented Jan 13, 2020

simonjayhawkins Jan 13, 2020

charlesdong1991 Jan 13, 2020 •

edited

Loading

charlesdong1991 commented Jan 13, 2020

charlesdong1991 commented Jan 14, 2020

simonjayhawkins commented Jan 14, 2020

WillAyd left a comment

WillAyd Jan 14, 2020

charlesdong1991 Jan 15, 2020

simonjayhawkins Jan 15, 2020

simonjayhawkins Jan 15, 2020

charlesdong1991 Jan 15, 2020

charlesdong1991 commented Jan 15, 2020

WillAyd Jan 17, 2020

WillAyd Jan 17, 2020

charlesdong1991 commented Jan 18, 2020

jreback commented Jan 18, 2020

	msg = 'pivot["(][")] missing 1 required argument: ["\']columns["\']'
	msg = re.escape("pivot() missing 1 required argument: 'columns'")

	msg = re.escape("pivot() missing 1 required argument: 'columns'")
	msg = r"pivot\(\) missing 1 required argument: 'columns'"

	- Correct incorrect error message in :meth:`DataFrame.pivot` when ``columns`` is set to ``None``. (:issue:`30924`)
	- Fix incorrect error message in :meth:`DataFrame.pivot` when ``columns`` is set to ``None``. (:issue:`30924`)

BUG: correct wrong error message in df.pivot when columns=None #30925

BUG: correct wrong error message in df.pivot when columns=None #30925

Conversation

charlesdong1991 commented Jan 11, 2020 • edited Loading

WillAyd commented Jan 11, 2020

charlesdong1991 commented Jan 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd commented Jan 13, 2020

simonjayhawkins commented Jan 13, 2020

WillAyd commented Jan 13, 2020

Choose a reason for hiding this comment

charlesdong1991 Jan 13, 2020 • edited Loading

Choose a reason for hiding this comment

charlesdong1991 commented Jan 13, 2020

charlesdong1991 commented Jan 14, 2020

simonjayhawkins commented Jan 14, 2020

WillAyd left a 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

charlesdong1991 commented Jan 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlesdong1991 commented Jan 18, 2020

jreback commented Jan 18, 2020

charlesdong1991 commented Jan 11, 2020 •

edited

Loading

charlesdong1991 commented Jan 12, 2020 •

edited

Loading

charlesdong1991 Jan 13, 2020 •

edited

Loading