API: honor copy=True when passing dict to DataFrame #38939

jbrockmendel · 2021-01-04T06:26:44Z

closes Dataframe change alters original array used in creation #32960
tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry

xref #34872 cc @TomAugspurger used the test_dict_nocopy you wrote but it ended up pretty mangled

…way-init-no-consolidate

…andas into myway-init-no-consolidate

…way-init-no-consolidate

doc/source/whatsnew/v1.3.0.rst

jreback · 2021-01-04T14:22:52Z

pandas/tests/frame/test_arithmetic.py

@@ -1297,7 +1297,7 @@ def test_strings_to_numbers_comparisons_raises(self, compare_operators_no_eq_ne)
            f(df, 0)

    def test_comparison_protected_from_errstate(self):
-        missing_df = tm.makeDataFrame()
+        missing_df = tm.makeDataFrame()._consolidate()


you are already doing this on the creation (I understand by find this fragile)

you are already doing this on the creation

i dont think so

(I understand by find this fragile)

I agree. Silver lining: finding the existing fragility.

actuallly you are doing this on creation, maybe you recently added. prefer NOT to do this in the tests proper (in pandas/testing is ok)

updated to do the consolidation in tm.makeDataFrame

jreback · 2021-01-04T14:23:37Z

pandas/tests/frame/test_reductions.py

@@ -765,7 +765,7 @@ def test_operators_timedelta64(self):
    def test_std_timedelta64_skipna_false(self):
        # GH#37392
        tdi = pd.timedelta_range("1 Day", periods=10)
-        df = DataFrame({"A": tdi, "B": tdi})
+        df = DataFrame({"A": tdi, "B": tdi}, copy=True)


why would copy matter here at all? isn't tdi supposed to be immutable? we need to always copy no?

why would copy matter here at all?

there are a couple of inter-twined issues here. the copy is necesary because we need df["A"] and df["B"] to not share data. We could do df = DataFrame({"A": tdi.values, "B": tdi.values.copy()})

isn't tdi supposed to be immutable?

yes, but thats kinda orthogonal (maybe 45°). e.g. in master

tdi = pd.timedelta_range("1 Day", periods=3) df = pd.DataFrame(tdi) df.iloc[0, 0] = pd.NaT >>> tdi TimedeltaIndex([NaT, '2 days', '3 days'], dtype='timedelta64[ns]', freq='D')

we need to always copy no?

i think we do something like this for dt64tz motivated by a similar immutability concern. ill see if that can be adapted

I think this copy=True can be removed now?

jreback · 2021-01-04T14:26:11Z

pandas/core/internals/managers.py

-                result[rl] = blk.iget((i, loc))
+                out = blk.iget((i, loc))
+                if is_dtype_equal(blk.dtype, dtype) and dtype == "m8[ns]":
+                    # FIXME: kludge for NaT -> tdnat


i know you mention, but this is very kludgy.

yah, im still looking for an alternative here

de-kludged. i need to double check, but think this may solve a separate silent bug. if so ill make a separate PR for that

jreback · 2021-01-04T14:27:46Z

I am leary of this consolidation (or rather non-consolidation). it is going to be quite fragile. why is consolidation tied to the dict copy at all?

…way-init-no-consolidate

jbrockmendel · 2021-01-12T03:35:18Z

I am leary of this consolidation (or rather non-consolidation). it is going to be quite fragile. why is consolidation tied to the dict copy at all?

@TomAugspurger can you weigh in on why this is worthwhile

TomAugspurger · 2021-01-12T12:01:37Z

I think just having the copy keyword apply to all data inputs, rather than just ndarrays.

doc/source/whatsnew/v1.3.0.rst

jreback · 2021-01-13T13:55:48Z

pandas/tests/frame/test_arithmetic.py

@@ -1297,7 +1297,7 @@ def test_strings_to_numbers_comparisons_raises(self, compare_operators_no_eq_ne)
            f(df, 0)

    def test_comparison_protected_from_errstate(self):
-        missing_df = tm.makeDataFrame()
+        missing_df = tm.makeDataFrame()._consolidate()


actuallly you are doing this on creation, maybe you recently added. prefer NOT to do this in the tests proper (in pandas/testing is ok)

pandas/tests/frame/test_constructors.py

pandas/core/frame.py

jorisvandenbossche · 2021-03-16T19:23:07Z

pandas/tests/frame/test_constructors.py

+        df.iloc[0, 1] = 0
+        if not copy:
+            # setitem on non-EA values preserves views
+            assert sum(x is c for x in df._mgr.arrays) == 1


Thanks, yes, that clarifies

Co-authored-by: Joris Van den Bossche <[email protected]>

jorisvandenbossche · 2021-03-26T20:47:17Z

@jbrockmendel can you do a final merge of master here to be sure it's all passing with latest master?

@jreback all good for you? (your "approve" is from before the behaviour changed to preserve the copy=False behaviour for dicts)

jreback

looks good. can you merge master, ping on greenish

jreback · 2021-03-30T13:10:56Z

pandas/core/frame.py

+    copy : bool or None, default None
+        Copy data from inputs.
+        For dict data, the default of None behaves like ``copy=True``.  For DataFrame
+        or 2d ndarray input, the default of None behaves like ``copy=False``.


can you add a versionchanged 1.3

jreback · 2021-03-30T13:11:19Z

pandas/core/frame.py

    ):
+
+        if copy is None:
+            # GH#38939


remove this line (as you mention below)

jreback · 2021-03-30T21:20:40Z

hmm, this looks to be failing a bunch of things

jreback · 2021-03-31T01:03:36Z

thanks @jbrockmendel this has been a long road, and @TomAugspurger for the original version.

jbrockmendel added 12 commits October 5, 2020 10:45

ENH: allow non-consolidation in constructors

cbc97f0

Merge branch 'master' of https://github.com/pandas-dev/pandas into my…

a135d96

…way-init-no-consolidate

mypy fixup

5c94129

ENH: allow non-consolidation in constructors

d653c54

Merge branch 'myway-init-no-consolidate' of github.com:jbrockmendel/p…

e71e319

…andas into myway-init-no-consolidate

Merge branch 'master' of https://github.com/pandas-dev/pandas into my…

cafa718

…way-init-no-consolidate

Merge branch 'master' of https://github.com/pandas-dev/pandas into my…

c706ad6

…way-init-no-consolidate

Merge branch 'master' of https://github.com/pandas-dev/pandas into my…

3f9195e

…way-init-no-consolidate

Merge branch 'master' of https://github.com/pandas-dev/pandas into my…

1cba671

…way-init-no-consolidate

BUG: respect copy=False in constructing DataFrame from dict

396daba

whatsnew

11ae1c9

clean test

b505267

jreback requested changes Jan 4, 2021

View reviewed changes

jreback added the API - Consistency Internal Consistency of API/Behavior label Jan 4, 2021

jbrockmendel added 8 commits January 4, 2021 09:51

Merge branch 'master' of https://github.com/pandas-dev/pandas into my…

a1e9b68

…way-init-no-consolidate

fixed xfail

b70d997

update whatsnew

09213e0

Merge branch 'master' of https://github.com/pandas-dev/pandas into my…

e895de0

…way-init-no-consolidate

de-kludge

31bda58

remove no-longer-used msg

37a2c0c

Merge branch 'master' of https://github.com/pandas-dev/pandas into my…

b93d7d5

…way-init-no-consolidate

Merge branch 'master' of https://github.com/pandas-dev/pandas into my…

aa667a6

…way-init-no-consolidate

fix broken test

185cd99

jreback requested changes Jan 13, 2021

View reviewed changes

jbrockmendel mentioned this pull request Jan 20, 2021

BUG: 2D ndarray of dtype 'object' is always copied upon construction #39272

Merged

4 tasks

Consolidate in tm, update whatsnew

701356f

jreback added the Copy / view semantics label Jan 21, 2021

jbrockmendel added 7 commits March 14, 2021 16:18

Merge branch 'master' into myway-init-no-consolidate

52344bb

update docstring

3ddc3d3

whatsnew, comment

e6bae0f

Merge branch 'master' into myway-init-no-consolidate

7cab084

Merge branch 'master' into myway-init-no-consolidate

e8e3d84

mypy fixup

5c44953

Merge branch 'master' into myway-init-no-consolidate

e32f630

jorisvandenbossche reviewed Mar 16, 2021

View reviewed changes

jorisvandenbossche approved these changes Mar 16, 2021

View reviewed changes

jbrockmendel and others added 3 commits March 16, 2021 14:50

Merge branch 'master' into myway-init-no-consolidate

abd890a

Update pandas/core/frame.py

1b7f7ca

Co-authored-by: Joris Van den Bossche <[email protected]>

Merge branch 'master' into myway-init-no-consolidate

b326b5f

jbrockmendel mentioned this pull request Mar 26, 2021

REF: move shift logic from BlockManager to DataFrame #40536

Merged

Merge branch 'master' into myway-init-no-consolidate

b7aed5d

jreback requested changes Mar 30, 2021

View reviewed changes

jbrockmendel added 2 commits March 30, 2021 08:02

Merge branch 'master' into myway-init-no-consolidate

4d20fe7

add versionchanged

ad5485a

jreback approved these changes Mar 30, 2021

View reviewed changes

jbrockmendel added 2 commits March 30, 2021 14:58

Merge branch 'master' into myway-init-no-consolidate

6bed6ac

update bc .values has changed to DTA/TDA

98b6dff

jreback merged commit 66e6ba3 into pandas-dev:master Mar 31, 2021

jbrockmendel deleted the myway-init-no-consolidate branch March 31, 2021 01:05

vladu pushed a commit to vladu/pandas that referenced this pull request Apr 5, 2021

API: honor copy=True when passing dict to DataFrame (pandas-dev#38939)

5e7f7d8

JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request Jul 3, 2021

API: honor copy=True when passing dict to DataFrame (pandas-dev#38939)

c928ed1

gsakkis mentioned this pull request Jul 12, 2021

BUG: extraneous copy of extension arrays in v1.3.0 #42501

Closed

brl0 mentioned this pull request Jul 23, 2021

BUG: Do not modify original DataFrame during creation of GeoDataFrame geopandas/geopandas#1324

Closed

simonjayhawkins mentioned this pull request Jan 15, 2022

BUG: DataFrame constructor with copy=False and missing columns creates columns that are views of each other #45369

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: honor copy=True when passing dict to DataFrame #38939

API: honor copy=True when passing dict to DataFrame #38939

jbrockmendel commented Jan 4, 2021

jreback Jan 4, 2021

jbrockmendel Jan 4, 2021

jreback Jan 13, 2021

jbrockmendel Jan 20, 2021

jreback Jan 4, 2021

jbrockmendel Jan 4, 2021

jorisvandenbossche Mar 5, 2021

jreback Jan 4, 2021

jbrockmendel Jan 4, 2021

jbrockmendel Jan 4, 2021

jreback commented Jan 4, 2021

jbrockmendel commented Jan 12, 2021

TomAugspurger commented Jan 12, 2021

jreback Jan 13, 2021

jorisvandenbossche Mar 16, 2021

jorisvandenbossche commented Mar 26, 2021 •

edited

Loading

jreback left a comment

jreback Mar 30, 2021

jreback Mar 30, 2021

jreback commented Mar 30, 2021

jreback commented Mar 31, 2021

API: honor copy=True when passing dict to DataFrame #38939

API: honor copy=True when passing dict to DataFrame #38939

Conversation

jbrockmendel commented Jan 4, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jan 4, 2021

jbrockmendel commented Jan 12, 2021

TomAugspurger commented Jan 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Mar 26, 2021 • edited Loading

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Mar 30, 2021

jreback commented Mar 31, 2021

jorisvandenbossche commented Mar 26, 2021 •

edited

Loading