-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
BUG: Fix unwanted type casting while replacing values in a DataFrame #33067
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
Conversation
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.
always tests first.
Thank, I'll follow this way in the future. I added a test. When you use a value, which is actually replaced in the test, the pd.NA is converted to np.nan, if the original DataFrame had float64 as datatype. The example given in the issue did not replace a value from the original DataFrame. Don't know, if this is a desired behavior. |
to_replace=to_replace, | ||
value=value, | ||
inplace=inplace, | ||
filter=filter, | ||
regex=regex, | ||
convert=convert, | ||
) | ||
blocks_converted = [] | ||
for ls_elem in block_replaced: |
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.
why is this logic not just in Block.replace itself? seems too complicated 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.
I wanted to ensure, that the conversion would only take place, after the block was forcefully cast to an ObjectBlock before.
I can add it to the replace function, if that would be better.
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 wanted to ensure, that the conversion would only take place, after the block was forcefully cast to an ObjectBlock before.
I can add it to the replace function, if that would be better.
the regression was caused by a clean-up to Block.replace (#27768), see #32988 (comment)
so fixing Block.replace as @jreback suggests seems more appropriate
i've labelled this as a regression, see #32988 (comment) cc @jbrockmendel |
can you merge master and see if you can simplify |
to_replace=to_replace, | ||
value=value, | ||
inplace=inplace, | ||
filter=filter, | ||
regex=regex, | ||
convert=convert, | ||
) | ||
blocks_converted = [] | ||
for ls_elem in block_replaced: |
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 wanted to ensure, that the conversion would only take place, after the block was forcefully cast to an ObjectBlock before.
I can add it to the replace function, if that would be better.
the regression was caused by a clean-up to Block.replace (#27768), see #32988 (comment)
so fixing Block.replace as @jreback suggests seems more appropriate
replace
casts columns toobject
#32988black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
As mentioned in #32988 here I think, that I have found a way to fix this, but I don't know, if this results in a desired behavior.
I would add tests after ensuring, that this does not break anything else.
Any thoughts about this?