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

ENH: Implement array ops for pandas #1100

Merged
merged 1 commit into from
Aug 10, 2017

Conversation

cpcloud
Copy link
Member

@cpcloud cpcloud commented Aug 9, 2017

Closes #1099

@cpcloud cpcloud added this to the 0.11.3 milestone Aug 9, 2017
@cpcloud cpcloud added feature Features or general enhancements pandas The pandas backend labels Aug 9, 2017
@cpcloud cpcloud self-assigned this Aug 9, 2017
@cpcloud
Copy link
Member Author

cpcloud commented Aug 9, 2017

@jreback can you review this when you get a chance?

@cpcloud
Copy link
Member Author

cpcloud commented Aug 9, 2017

Going to pull out the conftest addition and timezone parameterized fixture refactor into a separate PR.

@cpcloud
Copy link
Member Author

cpcloud commented Aug 9, 2017

@jreback I'm going to merge #1102 first, so that this PR doesn't contain any changes unrelated to pandas array ops.

@cpcloud cpcloud force-pushed the pandas-array-ops branch 2 times, most recently from 05ab434 to fc12974 Compare August 9, 2017 19:30
@@ -740,6 +741,9 @@ class MultiQuantile(Quantile):
name='interpolation',
default='linear')]

def output_type(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe should consolidate all of the helper routines somewhere

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 most of these are operation specific so I don't know if there's any consolidation possible here.

@@ -2403,9 +2407,9 @@ class ArraySlice(ValueOp):
input_type = [
rules.array(dt.any),
rules.integer(name='start'),
rules.integer(name='stop')
rules.integer(name='stop', optional=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

step?

Copy link
Member Author

@cpcloud cpcloud Aug 9, 2017

Choose a reason for hiding this comment

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

step isn't supported yet. We could make the translation specific to the backend and have the translation rules raise for SQL backends and have pandas work. I'll make an issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

The downside to this approach is that you don't get a failure at expression creation time, instead you get a failure at compile time.

raise ValueError(
'Array value type must be a primitive type '
'(e.g., number, string, or timestamp)'
)
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this just .astype()? or is this basically np.array(ibis_expr)?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, because we're casting the elements of each list in the Series to a different value type.

@cpcloud
Copy link
Member Author

cpcloud commented Aug 9, 2017

@jreback Ok to merge on green?

@jreback
Copy link
Contributor

jreback commented Aug 10, 2017

yep lgtm.

@jreback jreback merged commit 5961b51 into ibis-project:master Aug 10, 2017
@cpcloud cpcloud deleted the pandas-array-ops branch August 10, 2017 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Features or general enhancements pandas The pandas backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants