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

BLD/TST: fix bool block failures when strings are passed to replace list #6354

Merged
merged 3 commits into from
Feb 15, 2014
Merged

BLD/TST: fix bool block failures when strings are passed to replace list #6354

merged 3 commits into from
Feb 15, 2014

Conversation

cpcloud
Copy link
Member

@cpcloud cpcloud commented Feb 14, 2014

closes #6353

@@ -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):
Copy link
Member Author

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 Falses directly and not construct and then fill

@cpcloud
Copy link
Member Author

cpcloud commented Feb 14, 2014

Oh darn this will not pass. This allows different size lists thru

@cpcloud
Copy link
Member Author

cpcloud commented Feb 14, 2014

gotta run will get to this tonight after work

@jreback jreback added Bug and removed Bug labels Feb 14, 2014
@jreback jreback added this to the 0.14.0 milestone Feb 14, 2014
@cpcloud
Copy link
Member Author

cpcloud commented Feb 15, 2014

@jreback This line (from TestSeries.test_replace) is masking the same failure, which is a TypeError and it in fact should be a TypeError, but not because it's trying to index a bool object.

ser = Series(tm.makeTimeSeries().index)
self.assertRaises(TypeError, ser.replace, range(1, 3), [np.nan, 0])

@cpcloud
Copy link
Member Author

cpcloud commented Feb 15, 2014

I think we should raise a TypeError on these kinds of replacements. It makes no sense to do something like

s.replace({'asdf': 'fda'})

on a bool Series. I would argue that even something like

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?

@cpcloud
Copy link
Member Author

cpcloud commented Feb 15, 2014

Darn, that's not really symmetric with other types though.

@cpcloud
Copy link
Member Author

cpcloud commented Feb 15, 2014

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

@cpcloud
Copy link
Member Author

cpcloud commented Feb 15, 2014

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

@jreback
Copy link
Contributor

jreback commented Feb 15, 2014

can you make an entire pass thru on boolean? I think it should just return directly (and if they actually try something like:

Series([True,False]).replace(True : 'b')

maybe just raise

but all other's will simply pass thru...
?

@jreback
Copy link
Contributor

jreback commented Feb 15, 2014

should the possibly compare stuff (and raising) just be in mask_missing?

@cpcloud
Copy link
Member Author

cpcloud commented Feb 15, 2014

Tried that already, it breaks things

@jreback
Copy link
Contributor

jreback commented Feb 15, 2014

ok then

merge when ready

@cpcloud
Copy link
Member Author

cpcloud commented Feb 15, 2014

I'm going to write a bit of docs and release notes first ... just to clarify these somewhat dark corners of the replace API

@jreback
Copy link
Contributor

jreback commented Feb 15, 2014

github looks back to form now......pls merge when you can....(of course docs good too!)

cpcloud added a commit that referenced this pull request Feb 15, 2014
BLD/TST: fix bool block failures when strings are passed to replace list
@cpcloud cpcloud merged commit adda113 into pandas-dev:master Feb 15, 2014
@cpcloud cpcloud deleted the bool-block-fail-6353 branch February 15, 2014 22:26
@jreback
Copy link
Contributor

jreback commented Feb 15, 2014

looks cool...thanks!

@cpcloud
Copy link
Member Author

cpcloud commented Feb 15, 2014

got that other replace fix coming soon to a pandas repo near you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Testing pandas testing functions or related to the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BLD: bool block failures on py 3.3 from GH6339
2 participants