Skip to content
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: only convert at end for Block.replace_list #46393

Closed

Conversation

simonjayhawkins
Copy link
Member

In Block.replace_list there is some logic to only convert at the end.

before #40082 the convert parameter of Block._replace_regex was honored, but #40082 changed it not convert. This caused a regression, #44864 and was then changed to always convert in #44897. In this PR, we restore the original behavior and honor the convert parameter, which is unused on master.

before #44940, the conversion was not done in Block._replace_coerce for the non-regex case. #44940 changed Block._replace_coerce to dispatch to Block._replace for the non-regex case. Block._replace converts by default.

i've removed _replace_coerce since it appears that the only logic there now is to decide whether to use Block.replace or Block._replace_regex and is only called from Block.replace_list. (The signatures for Block.replace or Block._replace_regex are now consistent in this PR)

I've labelled as a regression but not sure whether we would want to backport this. I have therefore marked as draft and will wait till the reported regressions #45836 and #45601 that are caused by #44940 to be fixed first to avoid merge conflicts with the backport if we don't backport this.

Alternatively we remove the convert logic from Block.replace_list as dead code.

@simonjayhawkins simonjayhawkins added Regression Functionality that used to work in a prior pandas version replace replace method labels Mar 16, 2022
to_replace,
value,
inplace=inplace,
convert=False,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the convert for the regex case that has been ignored since #40082

)
else:
return self.replace(
to_replace=to_replace, value=value, inplace=inplace, mask=mask
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the dispatch to self.replace which converted by default, and didn't have convert parameter.

@simonjayhawkins
Copy link
Member Author

i've removed _replace_coerce since it appears that the only logic there now is to decide whether to use Block.replace or Block._replace_regex and is only called from Block.replace_list. (The signatures for Block.replace or Block._replace_regex are now consistent in this PR)

i've re-instated this since there is also now some logic for None handling there that ultimately we may either want to make consistent for a list-like and scalar to_replace or maybe handle in Block.replace. Either way, that change would not have been suitable for a backport for the fix #46404

_replace_coerce is only called when to_replace is not a scalar.

handling in Block.replace for a list-like (on main) should probably wait till #45725 is resolved.

@simonjayhawkins simonjayhawkins marked this pull request as ready for review March 20, 2022 11:55
@simonjayhawkins simonjayhawkins marked this pull request as draft March 20, 2022 11:57
@simonjayhawkins
Copy link
Member Author

i've re-instated this

oops. forgot to add the convert argument back in. will do that now.

@github-actions
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Regression Functionality that used to work in a prior pandas version replace replace method Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant