REGR: Fix fillna making a copy when dict was given as fill value and inplace is set #47327
Conversation
phofl
added
Regression
@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 |
Thanks. The fix here is different to suggested by @jbrockmendel in #47188 (comment) 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 |
pandas/core/internals/blocks.py
Outdated
| @@ -1183,6 +1183,9 @@ def fillna( | |||
| # test_fillna_dtype_conversion_equiv_replace | |||
| nbs = self.where(value, ~mask.T, _downcast=False) | |||
|
|
|||
| if inplace: | |||
| return nbs | |||
There was a problem hiding this comment.
can just return early in the if inplace: block above?
yep. I see it passing locally. you don't need the change to 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... |
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 |
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 |
it's failing on the array manager tests? can reproduce with 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
|
|
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. |
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 |
This comment was marked as resolved.
This comment was marked as resolved.
…t was given as fill value and inplace is set
|
|
Thx |
…en dict was given as fill value and inplace is set) (#47448) * Backport PR #47327: REGR: Fix fillna making a copy when dict was given as fill value and inplace is set Co-authored-by: Patrick Hoefler <[email protected]>
doc/source/whatsnew/vX.X.X.rstfile 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