-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
REGR: Replace changes the dtype of other columns #46636
REGR: Replace changes the dtype of other columns #46636
Conversation
@jbrockmendel ok here? |
pandas/core/internals/blocks.py
Outdated
nb = nb.copy() | ||
putmask_inplace(nb.values, mask, value) | ||
return [nb] | ||
return [self] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably need self.copy() if not inplace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, in #46404 (the cause of the latest regression), I reverted some of the code change in #44940 (the cause of the previous regression). In this PR I have reverted some more of the changes in #44940 that obviously (now!) should also have been included in the previous regression fix. I don't see the original code making a copy if not mask.any().
but yes, in #44940 the code path was changed to dispatch to Block.replace which does make a copy even if the replace is a no-op. So as a pure regression fix we could keep this as is, but also happy to do a copy for consistency with other replacements in 1.4.x
my biggest concern (need to check this though) is categorical since the special casing was also moved in #44940.
I will mark this a draft for now, (till we get closer to the next release) and see if we can get the issues on master sorted so that we can dispatch to Block.replace when the replacement value is None
. (doing that might end up being difficult to backport though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty actively trying to dis-engage, not gonna fuss about it either way. i trust your judgment here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my biggest concern (need to check this though) is categorical since the special casing was also moved in #44940.
There is an issue raised for the categorical issue #46672, so can keep that out of this PR for now.
I will mark this a draft for now, (till we get closer to the next release) and see if we can get the issues on master sorted so that we can dispatch to Block.replace when the replacement value is
None
. (doing that might end up being difficult to backport though)
I guess no harm in getting this bit sorted and then doing that as a separate PR. (maybe after #46672 is sorted).
one possibly-missed copy, otherwise seems reasonable |
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
done. |
Thanks @simonjayhawkins. |
* REGR: Replace changes the dtype of other columns * copy if not inplace
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.