Fix pivot index bug #37771

Jacob-Stevens-Haas · 2020-11-11T22:05:50Z

Previously, the variable "cols" was created solely to concatenate
the lists "index" and "columns" when the "values" parameter was
left as None. It did so in a way that it modified the variable
passed as "index" using list.extend(), affecting the caller's
namespace.

This change simply uses an anonymous list in place of the
variable "cols"

Had some trouble setting up the dev environment to build pandas from source,
so no tests checked or docs built. However, it's fairly minor change, so hoping
that it builds green regardless...

closes BUG: pivot() modifies arguments #37635
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Previously, the variable "cols" was created solely to concatenate the lists "index" and "columns" when the "values" parameter was left as None. It did so in a way that it modified the variable passed as "index" using list.extend(), affecting the caller's namespace. This change simply uses an anonymous list in place of the variable "cols"

arw2019

Thanks @Jacob-Stevens-Haas for the PR!

Can you add tests? That's the first thing we look for when reviewing

Jacob-Stevens-Haas · 2020-11-12T03:14:47Z

Thanks for the quick reply @arw2019!

Quick question: I'm essentially copying TestPivot.test_pivot(). The point of the bugfix is to prevent the pivot() method from modifying the passed argument index. Should I still include the assert statements from TestPivot.test_pivot() to verify that the pivot itself worked, or just include an assert statement focused on the bugfix?

arw2019 · 2020-11-12T03:22:34Z

Thanks for the quick reply @arw2019!

Quick question: I'm essentially copying TestPivot.test_pivot(). The point of the bugfix is to prevent the pivot() method from modifying the passed argument index. Should I still include the assert statements from TestPivot.test_pivot() to verify that the pivot itself worked, or just include an assert statement focused on the bugfix?

It's easiest if you post the test then will look. As a general rule though we like DRY code yes

This is a amended commit to solve pre-commit test: "rst ``code`` is two backticks"

arw2019 · 2020-11-12T03:51:44Z

pandas/tests/reshape/test_pivot.py

@@ -2153,3 +2153,20 @@ def test_pivot_index_none(self):

        expected.columns.name = "columns"
        tm.assert_frame_equal(result, expected)
+
+    def test_pivot_index_list_values_none(self):


I might be wrong but I don't think this is testing the OP...?

I'd use the example from the issue. Do the pivot and test the result of the pivot operation, then test that the arguments have not changed

Actually definitely use that. This isn't quite the same use case

arw2019 · 2020-11-12T03:52:22Z

pandas/tests/reshape/test_pivot.py

+        frame = DataFrame(data)
+        frame.pivot(index=index, columns=columns, values=None)
+
+        expected_ser = Series(["index"])


Why are you converting to Series? you want to check that index itself (the list) did not change, no?

Was trying to use the assert functions in pandas._testing. Should I just use standard assert?

yes use a standard assert here

* Use test case from the issue * Use standard assert instead of making series and using pandas._testing

Now I think I've got the hang of when to use one backtick versus two

pep8speaks · 2020-11-12T05:05:41Z

Hello @Jacob-Stevens-Haas! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-11-13 18:14:02 UTC

arw2019

Looks good, some comments

arw2019 · 2020-11-12T05:15:32Z

pandas/tests/reshape/test_pivot.py

+
+    def test_pivot_index_list_values_none(self):
+        # Tests when index is a list and values None. See GH#37635
+        frame = pd.DataFrame({


nit-pick but call it df (convention)

arw2019 · 2020-11-12T05:16:27Z

pandas/tests/reshape/test_pivot.py

+        index = ["lev1", "lev2"]
+        columns = ["lev3"]
+        frame.pivot(index=index, columns=columns, values=None)
+


check here that you got the correct result from the pivot (might need to hard-code)

arw2019 · 2020-11-12T05:17:31Z

pandas/tests/reshape/test_pivot.py

@@ -2153,3 +2153,18 @@ def test_pivot_index_none(self):

        expected.columns.name = "columns"
        tm.assert_frame_equal(result, expected)
+
+    def test_pivot_index_list_values_none(self):


call it test_pivot_index_list_values_none_modifies_args

arw2019 · 2020-11-12T05:17:56Z

pandas/tests/reshape/test_pivot.py

@@ -2153,3 +2153,18 @@ def test_pivot_index_none(self):

        expected.columns.name = "columns"
        tm.assert_frame_equal(result, expected)
+
+    def test_pivot_index_list_values_none(self):
+        # Tests when index is a list and values None. See GH#37635


just do

# GH37635

* My previous change caused the append = index is None to never set append = True * black pandas fixed my KR braces in test_pivot.py * Also black pandas fixed my KR braces on test_pivot.py

* Naming the test * Commenting the test * `frame` is now named `df` * Added check that the frame actually pivots correctly * Also: solved inconsistent pandas namespace in tests (precommit CI)

arw2019

Lgtm pending CI turning anything up

arw2019 · 2020-11-12T06:22:03Z

pandas/tests/reshape/test_pivot.py

+                "d": [1.0, float("nan"), 3.0, 5.0],
+            }
+        )
+        expected.index = MultiIndex.from_arrays(


put the index and column names into the data frame constructor instead of setting them like this (can pass them in as keyword args)

arw2019 · 2020-11-12T06:22:44Z

pandas/tests/reshape/test_pivot.py

+
+        expected = DataFrame(
+            {
+                "a": [1.0, 3.0, 5.0, float("nan")],


Do "np.nan" here?

arw2019 · 2020-11-12T06:24:30Z

doc/source/whatsnew/v1.2.0.rst

@@ -553,6 +553,7 @@ Reshaping
 - Bug in :meth:`DataFrame.agg` with ``func={'name':<FUNC>}`` incorrectly raising ``TypeError`` when ``DataFrame.columns==['Name']`` (:issue:`36212`)
 - Bug in :meth:`Series.transform` would give incorrect results or raise when the argument ``func`` was dictionary (:issue:`35811`)
 - Bug in :meth:`DataFrame.pivot` did not preserve :class:`MultiIndex` level names for columns when rows and columns both multiindexed (:issue:`36360`)
+- Bug in :meth:`DataFrame.pivot` modified caller's ``index`` parameter when ``columns`` was passed but ``values`` was not (:issue:`37635`)


... modified index argument

arw2019 · 2020-11-12T14:23:56Z

pandas/tests/reshape/test_pivot.py

+        )
+        index = ["lev1", "lev2"]
+        columns = ["lev3"]
+        pivoted = df.pivot(index=index, columns=columns, values=None)


a total nit but can you call this result

ivanovmg

@Jacob-Stevens-Haas, thank you for the bug fix!
Please see the comment below.

ivanovmg · 2020-11-12T14:26:12Z

pandas/tests/reshape/test_pivot.py

+    def test_pivot_index_list_values_none_modifies_args(self):
+        # GH37635


This PR is about making sure that the index list does not change.
Would it be more reasonable to test here only that, without comparing pivoted table with the expected one?
This is related to one of your prior questions.

Thanks for the quick reply @arw2019!

Quick question: I'm essentially copying TestPivot.test_pivot(). The point of the bugfix is to prevent the pivot() method from modifying the passed argument index. Should I still include the assert statements from TestPivot.test_pivot() to verify that the pivot itself worked, or just include an assert statement focused on the bugfix?

I had that originally, but a previous review asked that I

check here that you got the correct result from the pivot (might need to hard-code)

Doesn't much matter to me - now that the code is written, it's easy to delete or leave in as desired.

I don't feel strongly on this
But I'd say it's useful to check that the pivot is doing what it's supposed to before checking that the args aren't changed

I looked through the test module and noticed that there are no tests on pivot when index is a list and values are None.
Thus, there is no repetition of any kind and it is perfectly fine to have the test as it is.
Thank you!

ivanovmg

Looks good to me.

ivanovmg · 2020-11-12T18:46:48Z

Only test_pivot_index_list_values_none_immutable_args may be a better name, since in fact the arguments must stay unchanged.

Jacob-Stevens-Haas · 2020-11-12T19:06:06Z

Only test_pivot_index_list_values_none_immutable_args may be a better name, since in fact the arguments must stay unchanged.

That too was a code-review-requested name, so I'll wait for @arw2019 to comment on that suggestion

Separate issue that I may need guidance on - the test that's failing on the Azure pipeline is TestRegistration.test_pandas_plots_register, which asserts that a Series.plot() doesn't raise any warnings. Going through relevant code for that test and Series.plot() now, but it's odd that the only changes between that test passing and failing were to test_pivot.py and whatsnew... no actual code changes to the package.

arw2019 · 2020-11-12T19:10:13Z

Only test_pivot_index_list_values_none_immutable_args may be a better name, since in fact the arguments must stay unchanged.

That too was a code-review-requested name, so I'll wait for @arw2019 to comment on that suggestion

Yeah that's a better name!

Separate issue that I may need guidance on - the test that's failing on the Azure pipeline is TestRegistration.test_pandas_plots_register, which asserts that a Series.plot() doesn't raise any warnings. Going through relevant code for that test and Series.plot() now, but it's odd that the only changes between that test passing and failing were to test_pivot.py and whatsnew... no actual code changes to the package.

Don't worry about it, it's unrelated (that test if flaky)

* renamed test variable to 'result' * renamed test

Jacob-Stevens-Haas · 2020-11-12T21:06:40Z

🥳 . Now that it's passing tests, is there anything else that needs to be done? Also, should I also try to apply the changes to the 1.1.x branch too? (1.0.x branch is stale too... is that one at end of life?)

jreback · 2020-11-13T13:17:09Z

pandas/tests/reshape/test_pivot.py

+
+        tm.assert_frame_equal(result, expected)
+
+        assert index == ["lev1", "lev2"]


can you also assert that columns is the same as original, ping on green.

Done but not green; new comment below.

I'd say it's green, LGTM

Gotcha, thanks! @jreback?

* Added test that columns variable is immutable * Changed name of test per code review (missed in prev commit)

Jacob-Stevens-Haas · 2020-11-13T20:29:15Z

Ok so same error to last time (TestRegistration.test_pandas_plots_register) on Windows37. Errors on Windows38 are all in listed as "new" tests, mostly with memory errors... Don't believe these are a consequence of my edits, but is it something I need to solve in this PR?

jreback · 2020-11-14T02:21:25Z

thanks @Jacob-Stevens-Haas

yeah CI has been having troubles with some windows lately.

arw2019 suggested changes Nov 11, 2020

View reviewed changes

Jake Stevens-Haas added 2 commits November 11, 2020 19:26

DOC: Documentation for GH37635

9a7a60a

This is a amended commit to solve pre-commit test: "rst ``code`` is two backticks"

TST: Added test for GH37635

edebfaa

Jacob-Stevens-Haas force-pushed the fix-pivot-index-bug branch from d683ad3 to edebfaa Compare November 12, 2020 03:44

arw2019 reviewed Nov 12, 2020

View reviewed changes

arw2019 added the Reshaping Concat, Merge/Join, Stack/Unstack, Explode label Nov 12, 2020

Jake Stevens-Haas added 2 commits November 11, 2020 20:48

TST: correct GH37635 test for code review

6fbebe4

* Use test case from the issue * Use standard assert instead of making series and using pandas._testing

DOC: Corrected whats_new for GH37635

d673a9d

Now I think I've got the hang of when to use one backtick versus two

CLN: newline at end of file

5833b33

arw2019 suggested changes Nov 12, 2020

View reviewed changes

Jake Stevens-Haas added 2 commits November 11, 2020 21:41

BUG: Fixed regression in TestPivot.test_pivot_index_none

51acab0

* My previous change caused the append = index is None to never set append = True * black pandas fixed my KR braces in test_pivot.py * Also black pandas fixed my KR braces on test_pivot.py

TST: Code review on GH37635

752ca20

* Naming the test * Commenting the test * `frame` is now named `df` * Added check that the frame actually pivots correctly * Also: solved inconsistent pandas namespace in tests (precommit CI)

arw2019 reviewed Nov 12, 2020

View reviewed changes

TST: Code review on GH37635

4cff34b

arw2019 reviewed Nov 12, 2020

View reviewed changes

arw2019 approved these changes Nov 12, 2020

View reviewed changes

ivanovmg suggested changes Nov 12, 2020

View reviewed changes

ivanovmg approved these changes Nov 12, 2020

View reviewed changes

TST: Code review on GH37635

2e249c9

* renamed test variable to 'result' * renamed test

ivanovmg requested a review from jreback November 13, 2020 10:25

jreback added this to the 1.2 milestone Nov 13, 2020

jreback added the Bug label Nov 13, 2020

jreback requested changes Nov 13, 2020

View reviewed changes

TST: code review GH37635

9e9ed00

* Added test that columns variable is immutable * Changed name of test per code review (missed in prev commit)

jreback approved these changes Nov 14, 2020

View reviewed changes

jreback merged commit eeaf828 into pandas-dev:master Nov 14, 2020

Jacob-Stevens-Haas deleted the fix-pivot-index-bug branch November 14, 2020 18:46

simonjayhawkins mentioned this pull request Nov 18, 2020

CI: unpin numpy for CI / Checks github action #36092

Merged

		def test_pivot_index_list_values_none_modifies_args(self):
		# GH37635


		tm.assert_frame_equal(result, expected)

		assert index == ["lev1", "lev2"]

Fix pivot index bug #37771

Fix pivot index bug #37771

Conversation

Jacob-Stevens-Haas commented Nov 11, 2020 • edited Loading

arw2019 left a comment

Choose a reason for hiding this comment

Jacob-Stevens-Haas commented Nov 12, 2020

arw2019 commented Nov 12, 2020

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

pep8speaks commented Nov 12, 2020 • edited Loading

Comment last updated at 2020-11-13 18:14:02 UTC

arw2019 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

arw2019 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

ivanovmg left a comment

Choose a reason for hiding this comment

ivanovmg Nov 12, 2020 • edited Loading

Choose a reason for hiding this comment

Jacob-Stevens-Haas Nov 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

ivanovmg left a comment

Choose a reason for hiding this comment

ivanovmg commented Nov 12, 2020

Jacob-Stevens-Haas commented Nov 12, 2020

arw2019 commented Nov 12, 2020

Jacob-Stevens-Haas commented Nov 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

Jacob-Stevens-Haas Nov 13, 2020 • edited Loading

Choose a reason for hiding this comment

Jacob-Stevens-Haas commented Nov 13, 2020

jreback commented Nov 14, 2020

Jacob-Stevens-Haas commented Nov 11, 2020 •

edited

Loading

pep8speaks commented Nov 12, 2020 •

edited

Loading

ivanovmg Nov 12, 2020 •

edited

Loading

Jacob-Stevens-Haas Nov 12, 2020 •

edited

Loading

Jacob-Stevens-Haas commented Nov 12, 2020 •

edited

Loading

Jacob-Stevens-Haas Nov 13, 2020 •

edited

Loading