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

overflow checks for Series operations #18802

Closed
jbrockmendel opened this issue Dec 16, 2017 · 4 comments
Closed

overflow checks for Series operations #18802

jbrockmendel opened this issue Dec 16, 2017 · 4 comments
Labels
API Design Needs Discussion Requires discussion from core team before further action

Comments

@jbrockmendel
Copy link
Member

Spending some time on #12534 the only fixes I'm seeing are pretty invasive. We need to use checked_add_with_arr to correctly catch overflows, and for that we need isna(...) masks. But the wrapping/redirection that goes on within ops._arith_method_SERIES make that problematic. In particular the scope in which the masks are defined (for datetime/timedelta dtypes) is different from the scope in which we need to do the overflow checking. Passing those masks back and forth across scopes gets ugly in a hurry.

I'd rather just define __add__, __sub__, __radd__, __rsub__ methods specific to datetimelike-dtypes and have Series dispatch to those when appropriate. Any thoughts?

@gfyoung gfyoung added API Design Needs Discussion Requires discussion from core team before further action labels Dec 17, 2017
@gfyoung
Copy link
Member

gfyoung commented Dec 17, 2017

@jbrockmendel : That's an interesting suggestion. Since this is pretty invasive, I'm not sure if we would actually tackle this for some time. Would be good for 1.0 though.

@gfyoung
Copy link
Member

gfyoung commented Dec 19, 2017

@jbrockmendel : Oh, don't close this! 😄 Keep it open. This is a very valid issue to raise. It just may not be addressed for some time give the invasiveness.

@gfyoung gfyoung reopened this Dec 19, 2017
@gfyoung gfyoung added this to the Next Major Release milestone Dec 19, 2017
@jbrockmendel
Copy link
Member Author

The idea was to roll it into #18824. I’m happy either way.

@gfyoung
Copy link
Member

gfyoung commented Dec 19, 2017

Ah, gotcha. Just for future reference, add a comment that you're doing this so that we're all on the same page. 😄

@gfyoung gfyoung closed this as completed Dec 19, 2017
@gfyoung gfyoung modified the milestones: Next Major Release, No action Dec 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Design Needs Discussion Requires discussion from core team before further action
Projects
None yet
Development

No branches or pull requests

2 participants