-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
BLD/TST: fix bool block failures when strings are passed to replace list #6354
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
@@ -2453,7 +2453,13 @@ def replace_list(self, src_list, dest_list, inplace=False, regex=False): | |||
def comp(s): | |||
if isnull(s): | |||
return isnull(values) | |||
return values == getattr(s, 'asm8', s) | |||
res = values == getattr(s, 'asm8', s) | |||
if not isinstance(res, np.ndarray): |
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 think if this is True
then res
will always be False
in which case I can just return an array of False
s directly and not construct and then fill
Oh darn this will not pass. This allows different size lists thru |
gotta run will get to this tonight after work |
@jreback This line (from ser = Series(tm.makeTimeSeries().index)
self.assertRaises(TypeError, ser.replace, range(1, 3), [np.nan, 0]) |
I think we should raise a s.replace({'asdf': 'fda'}) on a s = Series([True, False])
s.replace({'asdf': 'fda', True: 'abc'}) should raise as well, because only part of the replacement is valid. @jreback What do you think? |
Darn, that's not really symmetric with other types though. |
Problem is that the time series behavior is throwing typeerrors for the wrong reasons, but i don't see a general purpose way to fix both this issue and that one without special case one of them |
But since we have to maintain the time series behavior for back compat and the bool behavior is now being defined, i think we should raise a typeerror on any of these kinds of conversions...doing that doesn't break any existing code, but makes it a bit more confusing for users to figure out what works .... e.g., s = Series([True, False])
s.replace({'a': 'b'}) will raise, but s.replace('a', 'b') will not |
can you make an entire pass thru on boolean? I think it should just return directly (and if they actually try something like:
maybe just raise but all other's will simply pass thru... |
should the possibly compare stuff (and raising) just be in mask_missing? |
Tried that already, it breaks things |
ok then merge when ready |
I'm going to write a bit of docs and release notes first ... just to clarify these somewhat dark corners of the replace API |
github looks back to form now......pls merge when you can....(of course docs good too!) |
BLD/TST: fix bool block failures when strings are passed to replace list
looks cool...thanks! |
got that other replace fix coming soon to a pandas repo near you |
closes #6353