REGR: Fix fillna making a copy when dict was given as fill value and inplace is set
State: closed

- [x] closes #47188 (Replace xxxx with the Github issue number)
- [x] [Tests added and passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#writing-tests) if fixing a bug or adding a new feature
- [x] All [code checks passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#pre-commit).
- [x] Added [type annotations](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#type-hints) to new arguments/methods/functions.
- [x] Added an entry in the latest `doc/source/whatsnew/vX.X.X.rst` file if fixing a bug or adding a new feature.

I think updating inplace is sensible here? This is a subtle change so I think we should backport, because the fact that we are using setitem under th hood should not leak into the interface. If we want to change this, we should deprecate, but only makes sens if we don't get Copy on Write in for 2.0

Labels:
name: Regression
description: Functionality that used to work in a prior pandas version
name: Copy / view semantics

Comments:
> I think updating inplace is sensible here? This is a subtle change so I think we should backport, because the fact that we are using setitem under th hood should not leak into the interface. If we want to change this, we should deprecate, but only makes sens if we don't get Copy on Write in for 2.0

@phofl can you also add your comments to the issue, #47188 to avoid having the discussion in more than one place as I think the expected behavior has not yet been confirmed.
Done
> Done

Thanks. The fix here is different to suggested by @jbrockmendel in https://github.com/pandas-dev/pandas/issues/47188#issuecomment-1143702013 so requesting a review from Brock.
Yep, sure.

Somehow this fails on the ci while passing locally, will have to investigate
Makes sense to me.  The solution I suggested in #47188 might avoid creating an intermediate array, but I think without the solution here it would be a wash.  So it might be worth looking into combining the two at some point.
hmm these are failing actually: https://github.com/pandas-dev/pandas/runs/6858133053?check_suite_focus=true

> Somehow this fails on the ci while passing locally, will have to investigate

yep. I see it passing locally. you don't need the change to `pandas/core/internals/blocks.py` in this PR as the code in `pandas/core/generic.py` doesn't pass the `inplace` argument.

perhaps remove that and see what fails?
Failure is the same, was just interested if it would make any difference.

Tried setting up a new env and tried editing 1.4.2 release, but can not reproduce the failure...
> but can not reproduce the failure...

some of the failures are for duplicate labels. maybe loc and setitem handle them differently, although that doesn't explain why it works on one platform and not another.
I think the duplicated failures are just deprecation warnings, which is expected I think.

What really bugs me is the new test that fails
> What really bugs me is the new test that fails

yeah, difficult without being able to reproduce. maybe just use the ci for troubleshooting and mark this as draft while debugging.
This is really tricky, don't really have an idea where to start, because this should work
> This is really tricky, don't really have an idea where to start, because this should work

it's failing on the array manager tests? can reproduce with

```
PANDAS_DATA_MANAGER=array pytest pandas/tests/frame/methods/test_fillna.py -k test_inplace_dict_update_view
```

can probably just skip them.

Ah we are running the tests twice, I did not know that
Thanks @phofl lgtm except can remove the change to `pandas/core/internals/blocks.py`?

`_maybe_downcast` short-circuits if `downcast` is `False`, so I assume that the `inplace` parameter of `blocks.fillna` maybe an "if can do" and that returning early might change some untested downcasting behavior. Not checked in detail, but if not needed for this PR, since backporting maybe safer not to change?
Yep definitely should remove. Not quite finished yet. As @yikun mentioned, should we also change for eval and update? Could do this in one pr.
> Could do this in one pr.

sure.
Removed eval, this is tricky. Update was straightforward
doesn't this overlap with the PR for the performance change in update?
Good point, forgot about that.

This restores behavior from 1.3.5, so the other pr was not only about performance, it would have also tackled a regression. Did not think about that before


merge this and close #47407?
Yes, if @jreback is ok?

I would open a dedicated issue for eval, the other is already pretty long
yes ok here
Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

1. Checkout backport branch and update it.

```
git checkout 1.4.x
git pull
```

2. Cherry pick the first parent branch of the this PR on top of the older branch:
```
git cherry-pick -x -m1 3364f9af4c5e3ea1d5585217d8c97cfe6631d8e1
```

3. You will likely have some merge/cherry-pick conflict here, fix them and commit:

```
git commit -am 'Backport PR #47327: REGR: Fix fillna making a copy when dict was given as fill value and inplace is set'
```

4. Push to a named branch:

```
git push YOURFORK 1.4.x:auto-backport-of-pr-47327-on-1.4.x
```

5. Create a PR against branch 1.4.x, I would have named this PR:

> "Backport PR #47327 on branch 1.4.x (REGR: Fix fillna making a copy when dict was given as fill value and inplace is set)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the `Still Needs Manual Backport` label once the PR gets merged.

If these instructions are inaccurate, feel free to [suggest an improvement](https://github.com/MeeseeksBox/MeeseeksDev).
                
Thanks @phofl 

There is also #46983. regression from \_\_setitem__ change. I've not looked but might be a similar fix. 
> I would open a dedicated issue for eval, the other is already pretty long

#47449
Thx