BUG: Replace internal use of loc with reindex in DataFrame append #26022

cbertinato · 2019-04-07T17:40:41Z

The issue here was that the DataFrame append method was using .loc, which only throws a warning now, but would eventually throw a KeyError whenever that went into effect. Just swapped out that use of .loc for .reindex.

closes A future error warning and a problem with autoconversion of types #22252
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

codecov · 2019-04-07T18:19:54Z

Codecov Report

Merging #26022 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #26022      +/-   ##
==========================================
- Coverage   91.82%   91.81%   -0.01%     
==========================================
  Files         175      175              
  Lines       52551    52551              
==========================================
- Hits        48256    48252       -4     
- Misses       4295     4299       +4

Flag	Coverage Δ
#multiple	`90.38% <100%> (ø)`	⬆️
#single	`40.72% <0%> (-0.14%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/frame.py	`96.79% <100%> (-0.12%)`	⬇️
pandas/io/gbq.py	`75% <0%> (-12.5%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c613c8...1bce70f. Read the comment docs.

codecov · 2019-04-07T18:19:54Z

Codecov Report

Merging #26022 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #26022      +/-   ##
==========================================
- Coverage   91.82%   91.82%   -0.01%     
==========================================
  Files         175      175              
  Lines       52539    52539              
==========================================
- Hits        48246    48242       -4     
- Misses       4293     4297       +4

Flag	Coverage Δ
#multiple	`90.38% <100%> (ø)`	⬆️
#single	`40.73% <0%> (-0.14%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/frame.py	`96.79% <100%> (-0.12%)`	⬇️
pandas/io/gbq.py	`75% <0%> (-12.5%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6de8133...4944b99. Read the comment docs.

jreback

always write tests first; how else would know if this works?

cbertinato · 2019-04-07T23:10:24Z

Sorry about that. In progress.

gfyoung · 2019-04-08T03:38:12Z

pandas/tests/frame/test_combine_concat.py

+        result = df.append(dicts, ignore_index=True, sort=True)
+        expected = df.append(DataFrame(dicts), ignore_index=True, sort=True)
+        assert_frame_equal(result, expected)
+


Separate out test and reference issue number as a comment under test function def.

cbertinato · 2019-04-08T12:49:24Z

Prior to this fix, the test still would have passed, but a FutureWarning would have been thrown, and eventually a KeyError. Should I also throw in a check that a FutureWarning and/or KeyError is not thrown?

codecov · 2019-04-08T13:25:07Z

Codecov Report

Merging #26022 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #26022      +/-   ##
==========================================
- Coverage   91.98%   91.97%   -0.01%     
==========================================
  Files         175      175              
  Lines       52377    52377              
==========================================
- Hits        48180    48176       -4     
- Misses       4197     4201       +4

Flag	Coverage Δ
#multiple	`90.53% <100%> (ø)`	⬆️
#single	`40.71% <0%> (-0.15%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/frame.py	`96.9% <100%> (-0.12%)`	⬇️
pandas/io/gbq.py	`78.94% <0%> (-10.53%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc86509...ff96e0c. Read the comment docs.

gfyoung · 2019-04-08T15:32:58Z

Should I also throw in a check that a FutureWarning and/or KeyError is not thrown?

The way that the test is written is fine. Just need to separate it out.

jreback · 2019-04-09T11:45:49Z

doc/source/whatsnew/v0.25.0.rst

@@ -325,7 +325,7 @@ Indexing
 ^^^^^^^^

 - Improved exception message when calling :meth:`DataFrame.iloc` with a list of non-numeric objects (:issue:`25753`).
-
+- Bug in which :meth:`DataFrame.append` produced a warning indicating that a KeyError will be thrown in the future when the data to be appended contains new columns (:issue:`22252`).


double backticks on KeyError. make this clear that this warning should not have been shown.

jreback · 2019-04-09T11:46:03Z

pandas/tests/frame/test_combine_concat.py

@@ -167,6 +167,15 @@ def test_append_list_of_series_dicts(self):
        expected = df.append(DataFrame(dicts), ignore_index=True, sort=True)
        assert_frame_equal(result, expected)

+        # GH22252


make this a new test

cbertinato · 2019-04-19T18:07:34Z

Anything else to do here?

jreback

test comment, ping on green.

jreback · 2019-04-19T20:27:37Z

pandas/tests/frame/test_combine_concat.py

+        df = DataFrame(np.random.randn(5, 4),
+                       columns=['foo', 'bar', 'baz', 'qux'])
+
+        dicts = [{'foo': 9}, {'bar': 10}]


can you put an assert_produces_warning(None) around this

jreback · 2019-04-20T16:47:34Z

lgtm. can you merge master; ping on green.

cbertinato · 2019-04-21T07:29:50Z

All green!

jreback · 2019-04-21T16:21:07Z

thanks @cbertinato

…6022)

jreback requested changes Apr 7, 2019

View reviewed changes

jreback added Bug Indexing Related to indexing on series/frames, not to indexes themselves Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels Apr 7, 2019

cbertinato force-pushed the 22252-df-append branch from 1bce70f to 47de400 Compare April 8, 2019 00:42

gfyoung reviewed Apr 8, 2019

View reviewed changes

cbertinato force-pushed the 22252-df-append branch from 47de400 to 4944b99 Compare April 8, 2019 12:47

jreback requested changes Apr 9, 2019

View reviewed changes

cbertinato force-pushed the 22252-df-append branch 2 times, most recently from f050f5f to 9ce63ff Compare April 11, 2019 17:06

cbertinato force-pushed the 22252-df-append branch from 9ce63ff to 96490c4 Compare April 17, 2019 13:25

jreback requested changes Apr 19, 2019

View reviewed changes

jreback added this to the 0.25.0 milestone Apr 19, 2019

cbertinato force-pushed the 22252-df-append branch 2 times, most recently from 5f1ac55 to f7d372d Compare April 20, 2019 03:41

jreback approved these changes Apr 20, 2019

View reviewed changes

BUG: Replace internal use of loc with reindex in append

ff96e0c

cbertinato force-pushed the 22252-df-append branch from f7d372d to ff96e0c Compare April 20, 2019 20:13

jreback merged commit 84fa2ef into pandas-dev:master Apr 21, 2019

ryanreh99 added a commit to ryanreh99/pandas that referenced this pull request Apr 22, 2019

BUG: Replace internal use of loc with reindex in append (pandas-dev#2…

5b7d4e5

…6022)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Replace internal use of loc with reindex in DataFrame append #26022

BUG: Replace internal use of loc with reindex in DataFrame append #26022

cbertinato commented Apr 7, 2019 •

edited

Loading

codecov bot commented Apr 7, 2019

codecov bot commented Apr 7, 2019 •

edited

Loading

jreback left a comment

cbertinato commented Apr 7, 2019

gfyoung Apr 8, 2019

cbertinato commented Apr 8, 2019

codecov bot commented Apr 8, 2019 •

edited

Loading

gfyoung commented Apr 8, 2019 •

edited

Loading

jreback Apr 9, 2019

jreback Apr 9, 2019

cbertinato commented Apr 19, 2019

jreback left a comment

jreback Apr 19, 2019

jreback commented Apr 20, 2019

cbertinato commented Apr 21, 2019

jreback commented Apr 21, 2019

BUG: Replace internal use of loc with reindex in DataFrame append #26022

BUG: Replace internal use of loc with reindex in DataFrame append #26022

Conversation

cbertinato commented Apr 7, 2019 • edited Loading

codecov bot commented Apr 7, 2019

Codecov Report

codecov bot commented Apr 7, 2019 • edited Loading

Codecov Report

jreback left a comment

Choose a reason for hiding this comment

cbertinato commented Apr 7, 2019

gfyoung Apr 8, 2019

Choose a reason for hiding this comment

cbertinato commented Apr 8, 2019

codecov bot commented Apr 8, 2019 • edited Loading

Codecov Report

gfyoung commented Apr 8, 2019 • edited Loading

jreback Apr 9, 2019

Choose a reason for hiding this comment

jreback Apr 9, 2019

Choose a reason for hiding this comment

cbertinato commented Apr 19, 2019

jreback left a comment

Choose a reason for hiding this comment

jreback Apr 19, 2019

Choose a reason for hiding this comment

jreback commented Apr 20, 2019

cbertinato commented Apr 21, 2019

jreback commented Apr 21, 2019

cbertinato commented Apr 7, 2019 •

edited

Loading

codecov bot commented Apr 7, 2019 •

edited

Loading

codecov bot commented Apr 8, 2019 •

edited

Loading

gfyoung commented Apr 8, 2019 •

edited

Loading