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

BUG: Catch overflow in both directions for checked add #14453

Merged
merged 1 commit into from Oct 22, 2016

Conversation

Projects
None yet
4 participants
@gfyoung
Member

gfyoung commented Oct 19, 2016

Port of #14324 after project name change.

@gfyoung

This comment has been minimized.

Show comment
Hide comment
@gfyoung

gfyoung Oct 19, 2016

Member

@jreback : Doesn't seem like Appveyor is picking up this PR.

Member

gfyoung commented Oct 19, 2016

@jreback : Doesn't seem like Appveyor is picking up this PR.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Oct 19, 2016

Current coverage is 85.25% (diff: 68.75%)

Merging #14453 into master will decrease coverage by <.01%

@@             master     #14453   diff @@
==========================================
  Files           140        140          
  Lines         50640      50654    +14   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          43175      43184     +9   
- Misses         7465       7470     +5   
  Partials          0          0          

Powered by Codecov. Last update 65362aa...2dc49e4

codecov-io commented Oct 19, 2016

Current coverage is 85.25% (diff: 68.75%)

Merging #14453 into master will decrease coverage by <.01%

@@             master     #14453   diff @@
==========================================
  Files           140        140          
  Lines         50640      50654    +14   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          43175      43184     +9   
- Misses         7465       7470     +5   
  Partials          0          0          

Powered by Codecov. Last update 65362aa...2dc49e4

@jreback jreback added this to the 0.19.1 milestone Oct 20, 2016

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Oct 20, 2016

Contributor

lgtm. can you add a whatsnew note. put in a new API Changes section in 0.19.1

Contributor

jreback commented Oct 20, 2016

lgtm. can you add a whatsnew note. put in a new API Changes section in 0.19.1

@jorisvandenbossche

This comment has been minimized.

Show comment
Hide comment
@jorisvandenbossche

jorisvandenbossche Oct 20, 2016

Member

Please no api change section for 0.19.1. We put it in the bug section for 0.19.0, so then can put it here there as well I think (we maybe should have put it in the api changes section for 0.19.0 ..)

Member

jorisvandenbossche commented Oct 20, 2016

Please no api change section for 0.19.1. We put it in the bug section for 0.19.0, so then can put it here there as well I think (we maybe should have put it in the api changes section for 0.19.0 ..)

BUG: Catch overflow in both directions for checked add
1) Add checks to ensure that add overflow does not
   occur both in the positive or negative directions.
2) Add benchmarks to ensure that operations involving
   this checked add function are significantly impacted.
@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Oct 20, 2016

Contributor

bug fix section is fine

Contributor

jreback commented Oct 20, 2016

bug fix section is fine

@jorisvandenbossche jorisvandenbossche merged commit 83a380c into pandas-dev:master Oct 22, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jorisvandenbossche

This comment has been minimized.

Show comment
Hide comment
Member

jorisvandenbossche commented Oct 22, 2016

@gfyoung Thanks!

@gfyoung gfyoung deleted the gfyoung:add-overflow-bug branch Oct 22, 2016

@gfyoung

This comment has been minimized.

Show comment
Hide comment
@gfyoung

gfyoung Oct 24, 2016

Member

@jreback , @jorisvandenbossche : So while it's good that we're catching underflow, when I try to integrate this check with tdi + tdi OR tdi + td, we run into issues:

>>> import pandas as pd
>>>
>>> tdi = pd.TimedeltaIndex([pd.NaT])
>>> td = pd.Timedelta('1 days')
>>>
>>> tdi - td   # Should return TimedeltaIndex([pd.NaT])
...
OverflowError: Overflow in int64 addition

This is because we are doing the checked add with the asi8 values, meaning that NaT becomes np.iinfo(np.int64).min, so the subtraction underflows as expected but not as we want. Not sure what the best way is to NOT do checked add for those elements whose value is NaN. Thoughts?

Member

gfyoung commented Oct 24, 2016

@jreback , @jorisvandenbossche : So while it's good that we're catching underflow, when I try to integrate this check with tdi + tdi OR tdi + td, we run into issues:

>>> import pandas as pd
>>>
>>> tdi = pd.TimedeltaIndex([pd.NaT])
>>> td = pd.Timedelta('1 days')
>>>
>>> tdi - td   # Should return TimedeltaIndex([pd.NaT])
...
OverflowError: Overflow in int64 addition

This is because we are doing the checked add with the asi8 values, meaning that NaT becomes np.iinfo(np.int64).min, so the subtraction underflows as expected but not as we want. Not sure what the best way is to NOT do checked add for those elements whose value is NaN. Thoughts?

@jorisvandenbossche

This comment has been minimized.

Show comment
Hide comment
@jorisvandenbossche

jorisvandenbossche Oct 24, 2016

Member

Additionally mask for that in the checking code?

Member

jorisvandenbossche commented Oct 24, 2016

Additionally mask for that in the checking code?

@gfyoung

This comment has been minimized.

Show comment
Hide comment
@gfyoung

gfyoung Oct 24, 2016

Member

Hmmm...I suppose, though that will impact performance again 😢 . I can give it a shot anyways and see what happens.

Member

gfyoung commented Oct 24, 2016

Hmmm...I suppose, though that will impact performance again 😢 . I can give it a shot anyways and see what happens.

jorisvandenbossche added a commit to jorisvandenbossche/pandas that referenced this pull request Nov 2, 2016

[Backport 14453] BUG: Catch overflow in both directions for checked a…
…dd (#14453)

1) Add checks to ensure that add overflow does not
   occur both in the positive or negative directions.
2) Add benchmarks to ensure that operations involving
   this checked add function are significantly impacted.
(cherry picked from commit 83a380c)

amolkahat added a commit to amolkahat/pandas that referenced this pull request Nov 26, 2016

BUG: Catch overflow in both directions for checked add (#14453)
1) Add checks to ensure that add overflow does not
   occur both in the positive or negative directions.
2) Add benchmarks to ensure that operations involving
   this checked add function are significantly impacted.

gfyoung added a commit to gfyoung/pandas that referenced this pull request Dec 7, 2016

BUG: Prevent addition overflow with TimedeltaIndex
Expands checked-add array addition introduced in
gh-14237 to include all other addition cases (i.e.
TimedeltaIndex and TimeDelta). Follow-up to gh-14453.

gfyoung added a commit to gfyoung/pandas that referenced this pull request Dec 7, 2016

BUG: Prevent addition overflow with TimedeltaIndex
Expands checked-add array addition introduced in
gh-14237 to include all other addition cases (i.e.
TimedeltaIndex and Timedelta). Follow-up to gh-14453.

gfyoung added a commit to gfyoung/pandas that referenced this pull request Dec 10, 2016

BUG: Prevent addition overflow with TimedeltaIndex
Expands checked-add array addition introduced in
gh-14237 to include all other addition cases (i.e.
TimedeltaIndex and Timedelta). Follow-up to gh-14453.

gfyoung added a commit to gfyoung/pandas that referenced this pull request Dec 11, 2016

BUG: Prevent addition overflow with TimedeltaIndex
Expands checked-add array addition introduced in
gh-14237 to include all other addition cases (i.e.
TimedeltaIndex and Timedelta). Follow-up to gh-14453.

gfyoung added a commit to gfyoung/pandas that referenced this pull request Dec 15, 2016

BUG: Prevent addition overflow with TimedeltaIndex
Expands checked-add array addition introduced in
gh-14237 to include all other addition cases (i.e.
TimedeltaIndex and Timedelta). Follow-up to gh-14453.

gfyoung added a commit to gfyoung/pandas that referenced this pull request Dec 15, 2016

BUG: Prevent addition overflow with TimedeltaIndex
Expands checked-add array addition introduced in
gh-14237 to include all other addition cases (i.e.
TimedeltaIndex and Timedelta). Follow-up to gh-14453.

gfyoung added a commit to gfyoung/pandas that referenced this pull request Dec 15, 2016

BUG: Prevent addition overflow with TimedeltaIndex
Expands checked-add array addition introduced in
gh-14237 to include all other addition cases (i.e.
TimedeltaIndex and Timedelta). Follow-up to gh-14453.

gfyoung added a commit to gfyoung/pandas that referenced this pull request Dec 15, 2016

BUG: Prevent addition overflow with TimedeltaIndex
Expands checked-add array addition introduced in
gh-14237 to include all other addition cases (i.e.
TimedeltaIndex and Timedelta). Follow-up to gh-14453.

gfyoung added a commit to gfyoung/pandas that referenced this pull request Dec 15, 2016

BUG: Prevent addition overflow with TimedeltaIndex
Expands checked-add array addition introduced in
gh-14237 to include all other addition cases (i.e.
TimedeltaIndex and Timedelta). Follow-up to gh-14453.

In addition, move checked add function to core/algorithms.

gfyoung added a commit to gfyoung/pandas that referenced this pull request Dec 17, 2016

BUG: Prevent addition overflow with TimedeltaIndex
Expands checked-add array addition introduced in
gh-14237 to include all other addition cases (i.e.
TimedeltaIndex and Timedelta). Follow-up to gh-14453.

In addition, move checked add function to core/algorithms.

jorisvandenbossche added a commit that referenced this pull request Dec 17, 2016

BUG: Prevent addition overflow with TimedeltaIndex (#14816)
Expands checked-add array addition introduced in
gh-14237 to include all other addition cases (i.e.
TimedeltaIndex and Timedelta). Follow-up to gh-14453.

In addition, move checked add function to core/algorithms.

ischurov added a commit to ischurov/pandas that referenced this pull request Dec 19, 2016

BUG: Prevent addition overflow with TimedeltaIndex (#14816)
Expands checked-add array addition introduced in
gh-14237 to include all other addition cases (i.e.
TimedeltaIndex and Timedelta). Follow-up to gh-14453.

In addition, move checked add function to core/algorithms.

ShaharBental added a commit to ShaharBental/pandas that referenced this pull request Dec 26, 2016

BUG: Prevent addition overflow with TimedeltaIndex (#14816)
Expands checked-add array addition introduced in
gh-14237 to include all other addition cases (i.e.
TimedeltaIndex and Timedelta). Follow-up to gh-14453.

In addition, move checked add function to core/algorithms.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment