BUG: df[col] = arr should not overwrite data in df[col]
State: closed

- [x] closes #33457
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

cc @jorisvandenbossche this still fails 7 tests locally and there's one more (commented in-line) test that looks fishy.  Extra eyeballs would be welcome.

xref  #35271, #35266

Labels:
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves

Comments:
Hello @jbrockmendel! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:

















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

##### Comment last updated at 2021-12-29 22:43:33 UTC
What's the summary of the behavior change from 1.0.5? `DataFrame.__setitem__[array]` will not mutate the existing array inplace? What dtypes does this affect?
@jbrockmendel I attempted a whatsnew in e60023725693586e8b9b47b9d8eaa14e09ea0ca0, if you could take a look.
Going through the failing tests

* test_fancy_getitem_slice_mixed: OK
* TestDataFrameIndexing.test_iloc_row: OK
* TestDataFrameIndexing.test_iloc_col: OK
* TestiLoc2.test_identity_slice_returns_new_object: ... Probably OK
* TestiLoc2.test_iloc_setitem_categorical_updates_inplace: Probably OK
* TestMerge.test_merge_nocopy: Probably OK

Looking into  `test_apply_function_with_indexing` some more now.
One question on desired behavior: `df.loc[:, "A"] = value` *should* mutate the array inplace, right? On this branch, that is lost:

```python
In [2]: df = pd.DataFrame({"A": [1, 2, 3]})

In [3]: df2 = df.iloc[:]

In [4]: df._mgr.blocks[0].values is df2._mgr.blocks[0].values
Out[4]: True

In [5]: df.loc[:, "A"] = 0

In [6]: df2
Out[6]:
   A
0  1
1  2
2  3
```
> One question on desired behavior: df.loc[:, "A"] = value should mutate the array inplace, right? On this branch, that is lost:

Agreed.  I think thats the behavior with the FIXME comment in test_block_internals.
> test_fancy_getitem_slice_mixed: OK

The relevant part of this test reads:

```
        sliced = float_frame.iloc[:, -3:]

        msg = r"\nA value is trying to be set on a copy of a slice from a DataFrame"
        with pytest.raises(com.SettingWithCopyError, match=msg):
            sliced["C"] = 4.0

        assert (float_frame["C"] == 4).all()
```

I think with the new behavior, the last assertion is now incorrect, as we expect the setitem to create a new array.  My question is: do we still need the SettingWithCopyError?
Sorry, but "OK" I meant OK with updating the tests for the new behavior.

>  My question is: do we still need the SettingWithCopyError?

I was wondering this as well. If the warning is about whether or not `float_frame` is updated then it seems like it can be removed. But perhaps the test should be updated to use `sliced.loc[:, "C"] = 4.0`, in which case the warning is still valid?
Down to two failing tests:

```
FAILED pandas/tests/groupby/test_apply_mutate.py::test_apply_function_with_indexing - AssertionError: Series are different
FAILED pandas/tests/reshape/merge/test_merge.py::TestMerge::test_merge_nocopy - AssertionError: assert False
```

`test_apply_function_with_indexing` i've tracked down into `libreduction.apply_frame_axis0`
@jbrockmendel 1.2 whatsnew now merged. also you can add back the release note that was deleted in #35271?
@TomAugspurger I've got this down to one failing test `test_apply_function_with_indexing`, could use some extra eyeballs.

That test reads:

```
df = pd.DataFrame(
        {"col1": ["A", "A", "A", "B", "B", "B"], "col2": [1, 2, 3, 4, 5, 6]}
)

def fn(x):
        x.col2[x.index[-1]] = 0
        return x.col2

result = df.groupby(["col1"], as_index=False).apply(fn)
expected = pd.Series(
        [1, 2, 0, 4, 5, 0],
        index=pd.MultiIndex.from_tuples(
            [(0, 0), (0, 1), (0, 2), (1, 3), (1, 4), (1, 5)]
        ),
        name="col2",
)
tm.assert_series_equal(result, expected)
```

ATM instead of `expected` I'm getting for `result`:

```
>>> result
0  0    1
   1    2
   2    0
1  3    1
   4    2
   5    0
Name: col2, dtype: int64

```

I can get this to pass if instead of `x.col2[x.index[-1]] = 0` I write `x.loc[x.index[-1], "col2"] = 0`

Alternatively, I get the expected result if I disable `fast_apply` in `groupby.ops`.
Haven't looked closely, but I'm probably OK with changing the test to use
`x.loc[x.index[-1], "col2"] = 0` instead. That's the recommended way to do
things anway.

On Sat, Aug 15, 2020 at 5:50 PM jbrockmendel <notifications@github.com>
wrote:

> @TomAugspurger <https://github.com/TomAugspurger> I've got this down to
> one failing test test_apply_function_with_indexing, could use some extra
> eyeballs.
>
> That test reads:
>
> df = pd.DataFrame(
>         {"col1": ["A", "A", "A", "B", "B", "B"], "col2": [1, 2, 3, 4, 5, 6]}
> )
>
> def fn(x):
>         x.col2[x.index[-1]] = 0
>         return x.col2
>
> result = df.groupby(["col1"], as_index=False).apply(fn)
> expected = pd.Series(
>         [1, 2, 0, 4, 5, 0],
>         index=pd.MultiIndex.from_tuples(
>             [(0, 0), (0, 1), (0, 2), (1, 3), (1, 4), (1, 5)]
>         ),
>         name="col2",
> )
> tm.assert_series_equal(result, expected)
>
> ATM instead of expected I'm getting for result:
>
> >>> result
> 0  0    1
>    1    2
>    2    0
> 1  3    1
>    4    2
>    5    0
> Name: col2, dtype: int64
>
>
> I can get this to pass if instead of x.col2[x.index[-1]] = 0 I write x.loc[x.index[-1],
> "col2"] = 0
>
> Alternatively, I get the expected result if I disable fast_apply in
> groupby.ops.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/35417#issuecomment-674454901>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAKAOIUKWFXOLSC3WFE2BI3SA4GMZANCNFSM4PIGHHVQ>
> .
>

> I'm probably OK with changing the test [...] That's the recommended way to do things anway.

Makes sense.  But would that be "breaking" the behavior for the non-recommended usage?
Indeed, though it's unclear whether that usage was "officially" supported. IIUC, the `x2.col[x.index[-1]] = 0` might not work for heterogenous dataframes?
I would be hesitant to break/change the existing test. With all view/copy complexities, the fact that getting one column as a series is a view on the data is one of the few certainties? 
(and thus editing that series in place should update the original dataframe?)

Do you understand the reason why this case is now broken?
> might not work for heterogenous dataframes?

I've cooked up an example where it doesn't work for non-unique columns:

```
df = pd.DataFrame(np.random.randn(3, 3), columns=["A", "A", "B"])
ser = df.B
ser[ser.index[-1]] = 0  # <-- produces `SettingWithCopyWarning`, does not modify `df`
```

> the fact that getting one column as a series is a view on the data is one of the few certainties?

The example above shows that this is not the case.  That said, I agree it _should_ be.

> Do you understand the reason why this case is now broken?

I've tracked it down into libreduction.apply_frame_axis0 and am stuck there
OK, so what I think is happening in `libreduction.apply_frame_axis0` is the following:

Instead of creating a bunch of DataFrames for the different groups, it creates one DataFrame, grabs the ndarrays in each block, and for each group moves the pointer so the same array points to the data for that group.

That trick is only valid if the blocks in this DataFrame remain unchanged throughout this iteration.  I think this can be re-pinned at each stage of the loop (albeit at a perf cost)
Thanks for working on this.

> if the blocks in this DataFrame remain unchanged throughout this iteration. I think this can be re-pinned at each stage of the loop (albeit at a perf cost)

Just thinking out loud, can we store property on the Block that somehow tracks if the block has been mutated? Then the for loop can be something like

```python
mutated = False
for block in blocks:
    func(block)
    if block.mutated:
        # do stuff
```

I suppose that would only work for things going through `Block.set`, but maybe that's enough? And feel free to ignore this if it doesn't make any sense in this context.
> Just thinking out loud, can we store property on the Block that somehow tracks if the block has been mutated? Then the for loop can be something like

This is similar to what the commit-before-last (pushed about an hour ago) did
@jorisvandenbossche are there unaddressed issues here?
Yes, see the discussion in #33457, I am not convinced that that are changes we should be making in 1.2
this is a blocker for being able to (accurately) cache properties on Blocks
I didn't yet look back into the actual matter, but as far as I know, the discussion I mentioned above (https://github.com/pandas-dev/pandas/pull/35417#issuecomment-681997130) is not yet resolved
@jorisvandenbossche the discussion in #35417 is unresolved only inasmuch as you are OK with the inconsistent behavior bug

Tom's [last](https://github.com/pandas-dev/pandas/issues/33457#issuecomment-664549215) comments was to fix this in 1.2 and jeff's [last](https://github.com/pandas-dev/pandas/issues/33457#issuecomment-660172070) expressed opinion was also to fix this.

> Tom's last comments was to fix this in 1.2 and jeff's last expressed opinion was also to fix this.

And it's only after those comments that I argued why I don't think we should change this for 1.2, and since then neither Tom or Jeff reacted again. 
ok moving these off 1.2. IF we made any change like this it cannot be at the last minute.

I am now confused over what we want. What i do know is we DO not want any special cases. I have to read the issue again, but it seems that @jorisvandenbossche is advocating for just that.
We have lots of special cases, and I certainly *also* want to get rid of those. But I think the discussion is more on *how* we should get rid of them (can we just "break" those cases or is there another way, do we want to keep such a breaking change for 2.0 or not, ...)
> you keep rebasing this

Yes, to keep this from drifting.  This seems like a non-sequitur.

> but to repeat what I said above (#35417 (comment)): this still first needs to be discussed further (#33457).

AFAICT the entire discussion consists of the two of us repeating ourselves.  Do you have something new to add?  if not, we're in the same position of "fix inconsistency" vs "keep inconsistency", and de facto it comes down to @jreback.
@jorisvandenbossche so re-reading this thread. IIUC. @jbrockmendel change here makes EA and non-EA act consistently by effectively *always* copying on assignment (since EA already copy this is making non-EA copy). And since this generally is affecting a single column, we might have to copy the *entire* block (or alternatly split the block which later results in consolidation).

fair? so the issue is essentially performance (as I think we agree on the semantics).
i am good with this change. @jorisvandenbossche @TomAugspurger