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

Conversation

Projects
None yet
2 participants
@cpcloud
Member

cpcloud commented Feb 14, 2014

closes #6353

@cpcloud

View changes

Show outdated Hide outdated pandas/core/internals.py
@cpcloud

This comment has been minimized.

Show comment
Hide comment
@cpcloud

cpcloud Feb 14, 2014

Member

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

Member

cpcloud commented Feb 14, 2014

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

@cpcloud

This comment has been minimized.

Show comment
Hide comment
@cpcloud

cpcloud Feb 14, 2014

Member

gotta run will get to this tonight after work

Member

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

This comment has been minimized.

Show comment
Hide comment
@cpcloud

cpcloud Feb 15, 2014

Member

@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])
Member

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

This comment has been minimized.

Show comment
Hide comment
@cpcloud

cpcloud Feb 15, 2014

Member

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?

Member

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

This comment has been minimized.

Show comment
Hide comment
@cpcloud

cpcloud Feb 15, 2014

Member

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

Member

cpcloud commented Feb 15, 2014

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

@cpcloud

This comment has been minimized.

Show comment
Hide comment
@cpcloud

cpcloud Feb 15, 2014

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@cpcloud

cpcloud Feb 15, 2014

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Feb 15, 2014

Contributor

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...
?

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

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Feb 15, 2014

Contributor

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

Contributor

jreback commented Feb 15, 2014

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

@cpcloud

This comment has been minimized.

Show comment
Hide comment
@cpcloud

cpcloud Feb 15, 2014

Member

Tried that already, it breaks things

Member

cpcloud commented Feb 15, 2014

Tried that already, it breaks things

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Feb 15, 2014

Contributor

ok then

merge when ready

Contributor

jreback commented Feb 15, 2014

ok then

merge when ready

@cpcloud

This comment has been minimized.

Show comment
Hide comment
@cpcloud

cpcloud Feb 15, 2014

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Feb 15, 2014

Contributor

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

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

Merge pull request #6354 from cpcloud/bool-block-fail-6353
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 cpcloud:bool-block-fail-6353 branch Feb 15, 2014

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Feb 15, 2014

Contributor

looks cool...thanks!

Contributor

jreback commented Feb 15, 2014

looks cool...thanks!

@cpcloud

This comment has been minimized.

Show comment
Hide comment
@cpcloud

cpcloud Feb 15, 2014

Member

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

Member

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