BUG: Fix overflow bugs in date_Range #24255

jbrockmendel · 2018-12-12T21:26:34Z

closes BUG: un-caught date_range overflows #24123
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Fixing this turned out to be a small beast. The overflows of interest fall into three categories. First: overflows when computing periods * stride that ATM are not caught, so give incorrect results:

>>> pd.date_range('1969-05-04', periods=200000000, freq='30000D')
DatetimeIndex(['1969-05-04', '2051-06-23', '2133-08-12'], dtype='datetime64[ns]', freq='30000D')

>>> dti = pd.date_range(start='1677-09-22', end='2262-04-11', freq='D')
>>> pd.date_range(start=dti[0], periods=len(dti), freq='D')
DatetimeIndex([], dtype='datetime64[ns]', freq='D')

Second, overflows when periods * stride overflow the int64 bounds but not uint64 bounds, which in some cases are recoverable. The following should not raise:

>>> dti = pd.date_range(start='1677-09-22', end='2262-04-11', freq='D')
>>> pd.date_range(start=dti[0], periods=len(dti)-1, freq='D')
[...]
pandas._libs.tslibs.np_datetime.OutOfBoundsDatetime: Cannot generate range with start=-9223286400000000000 and periods=213502

Last is a similar one with a negative stride where we can recover, but not by converting to uint64:

>>> dti = pd.date_range(start='1677-09-22', end='2262-04-11', freq='D')
>>> pd.date_range(end=dti[-1], periods=len(dti), freq='D')
[...]
pandas._libs.tslibs.np_datetime.OutOfBoundsDatetime: Cannot generate range with end=9223372800000000000 and periods=213503

pep8speaks · 2018-12-12T21:26:37Z

Hello @jbrockmendel! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on December 23, 2018 at 16:48 Hours UTC

codecov · 2018-12-12T22:04:15Z

Codecov Report

Merging #24255 into master will decrease coverage by <.01%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master   #24255      +/-   ##
==========================================
- Coverage   92.21%   92.21%   -0.01%     
==========================================
  Files         162      162              
  Lines       51768    51806      +38     
==========================================
+ Hits        47739    47773      +34     
- Misses       4029     4033       +4

Flag	Coverage Δ
#multiple	`90.61% <92.3%> (-0.01%)`	⬇️
#single	`42.98% <38.46%> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/arrays/datetimes.py	`97.98% <92.3%> (-0.58%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9bc42f9...c6256de. Read the comment docs.

codecov · 2018-12-12T22:04:15Z

Codecov Report

Merging #24255 into master will increase coverage by <.01%.
The diff coverage is 98.73%.

@@            Coverage Diff             @@
##           master   #24255      +/-   ##
==========================================
+ Coverage   92.29%    92.3%   +<.01%     
==========================================
  Files         162      163       +1     
  Lines       51890    51932      +42     
==========================================
+ Hits        47892    47934      +42     
  Misses       3998     3998

Flag	Coverage Δ
#multiple	`90.71% <98.73%> (ø)`	⬆️
#single	`42.98% <55.69%> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/arrays/datetimes.py	`97.77% <100%> (+0.04%)`	⬆️
pandas/core/arrays/_ranges.py	`98.66% <98.66%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7bf6f2...43d3d83. Read the comment docs.

pandas/core/arrays/datetimes.py

jreback

why are you not using algos.checked_add_with_arr?
I am -1 on using uint64 here at all. why add all of this complexity, simply raise?

jbrockmendel · 2018-12-13T01:13:31Z

why are you not using algos.checked_add_with_arr?

We are in one place, but it is mostly unnecessary, adds a lot of machinery for NA-masking that isn't relevant here.

I am -1 on using uint64 here at all. why add all of this complexity, simply raise?

Why would we want to raise on valid inputs? It's entirely possible that there is a simpler way to do this; I banged my head against it for a while before I got it working, so by the end elegance was not a priority.

These overflows were found when debugging the pickle problems in #24074. There is a test that creates a DTI (something like) dti = pd.date_range(start='1677-09-22', end='2262-04-11', freq='D'), pickles it, then tries to unpickle it. In the unpickling, it calls validate_frequency, which calls cls._generate_range(start=dti[0], periods=len(dti), freq=dti.freq), which in master currently returns an incorrect result because of a silent overflow in multiplication.

jreback · 2018-12-13T01:44:56Z

Why would we want to raise on valid inputs? It's entirely possible that there is a simpler way to do this; I banged my head against it for a while before I got it working, so by the end elegance was not a priority

how are these valid inputs iif you are overflowing int64? I would rather not jump thru hoops and have over complex code.

jbrockmendel · 2018-12-13T01:54:14Z

how are these valid inputs iif you are overflowing int64?

Because the biggest number we can get by adding an int64 to np.iinfo(np.int64).min is -1, but we can have date_ranges that start near the lower implementation bound and proceed well past 1970-01-01.

I claim that for any DTI produced with dti = pd.date_range(start=start, end=end, freq=freq), we should be able to write dti2 = pd.date_range(start=dti[0], periods=len(dti), freq=dti.freq) and have assert dti2.equals(dti). Ditto for dti3 = pd.date_range('end=dti[-1], periods=len(dti), freq=dti.freq).

jbrockmendel · 2018-12-14T02:04:55Z

Travis fail is Hypothesis

jreback · 2018-12-15T21:36:55Z

pandas/core/arrays/datetimes.py

@@ -1747,7 +1747,8 @@ def _generate_regular_range(cls, start, end, periods, freq):
    return data


-def _generate_range_overflow_safe(endpoint, periods, stride, side='start'):
+def _generate_range_overflow_safe(endpoint, periods, stride,
+                                  side='start'):
    """
    Calculate the second endpoint for passing to np.arange, checking
    to avoid an integer overflow.  Catch OverflowError and re-raise


can you doc-string this a bit more (say what the parameters mean)

jreback · 2018-12-15T21:40:04Z

pandas/core/arrays/datetimes.py

+    periods : int
+    stride : int
+    side : {'start', 'end'}
+


so pls move all of this new code to the same place as checked_add_with_arr

algos seems like a weird place to put datetime-specific code, no? I guess in principle this code could be used for generating int64 ranges...

jreback · 2018-12-15T21:40:21Z

pandas/tests/indexes/datetimes/test_date_range.py

@@ -88,6 +88,33 @@ def test_date_range_nat(self):
        with pytest.raises(ValueError, match=msg):
            date_range(start=pd.NaT, end='2016-01-01', freq='D')

+    def test_date_range_multiplication_overflow(self):


so 2 test cases fully exercise all of this code???

Good call, I'm tracking down untested cases now.

jreback · 2018-12-15T21:41:06Z

pandas/core/arrays/datetimes.py

+                if result <= i64max + np.uint64(stride):
+                    return result
+            else:
+                return _generate_range_recurse(endpoint, periods,


what are these 2 cases?

why is this recursion? why can't you do this iteratively?

why is this recursion? why can't you do this iteratively?

generate_range_recurse breaks the problem down into two smaller pieces and calls _generate_range_overflow_safe twice. It is iterative in the sense that it makes two calls, as opposed to repeated calls to itself. It is recursive inasmuch as it is called via _generate_range_overflow_safe, which it also calls. I'm open to other ideas for names.

what are these 2 cases?

Which two cases are you asking about?

jbrockmendel · 2018-12-16T00:43:07Z

Updated with a couple of tests for previously un-hit cases, removed some code that turned out to be un-reachable, fleshed out docstring params.

It turns out the call to np.arange can overflow. Now catch that too (with test)

jbrockmendel · 2018-12-21T17:19:13Z

gentle ping. I think this will fix the inability to do frequency validation discussed in #24024.

pandas/core/arrays/datetimes.py

jreback · 2018-12-23T16:06:31Z

pandas/core/arrays/_ranges.py

+                             "if a 'period' is given.")
+
+        with np.errstate(over="raise"):
+            try:


comment on why you need this

jreback · 2018-12-23T16:11:08Z

pandas/core/arrays/_ranges.py

+        return _generate_range_overflow_safe_signed(
+            endpoint, periods, stride, side)
+
+    elif ((endpoint > 0 and side == 'start' and stride > 0) or


isn't this case already hendled in _generate_range_overflow_safe_signed? l169

No. The check on 169 is specific to the case where periods * stride doesn't overflow int64 bounds. The check here on 127 is for the case where it does.

That said, this check could be removed. It is a fast-path to fail early.

k make sure that all of these paths are hit and tested - make as simple as possible

Just double-checked: every branch in here is hit in the tests

jreback · 2018-12-23T17:32:54Z

thanks

mroeschke reviewed Dec 12, 2018

View reviewed changes

pandas/core/arrays/datetimes.py Outdated Show resolved Hide resolved

mroeschke reviewed Dec 12, 2018

View reviewed changes

pandas/core/arrays/datetimes.py Outdated Show resolved Hide resolved

jreback requested changes Dec 13, 2018

View reviewed changes

jreback added Bug Datetime Datetime data dtype labels Dec 13, 2018

jbrockmendel added 4 commits December 13, 2018 16:35

Fix overflow bugs in date_Range

db4da3b

add GH reference

87acff1

cleanup, simplification, better error message

67f98f1

handle missed corner case

1bf5642

jbrockmendel force-pushed the over branch from 3f932b1 to 1bf5642 Compare December 14, 2018 00:54

Merge branch 'master' of https://github.com/pandas-dev/pandas into over

9460f31

jreback requested changes Dec 15, 2018

View reviewed changes

jbrockmendel added 2 commits December 15, 2018 14:52

Merge branch 'master' of https://github.com/pandas-dev/pandas into over

e825a08

more tests, docstrings

9af17d4

jbrockmendel mentioned this pull request Dec 19, 2018

REF: DatetimeLikeArray #24024

Merged

12 tasks

jreback reviewed Dec 21, 2018

View reviewed changes

pandas/core/arrays/datetimes.py Outdated Show resolved Hide resolved

jreback reviewed Dec 21, 2018

View reviewed changes

pandas/core/arrays/datetimes.py Outdated Show resolved Hide resolved

jbrockmendel added 3 commits December 21, 2018 15:23

Merge branch 'master' of https://github.com/pandas-dev/pandas into over

676d6e0

move range-generation functions to arrays._ranges

40c2f5b

avoid over-writing tz

018ccca

jreback requested changes Dec 23, 2018

View reviewed changes

jbrockmendel added 2 commits December 23, 2018 08:42

Merge branch 'master' of https://github.com/pandas-dev/pandas into over

03a90c4

overflow comment

43d3d83

jreback approved these changes Dec 23, 2018

View reviewed changes

jreback added this to the 0.24.0 milestone Dec 23, 2018

jreback merged commit 1cd077a into pandas-dev:master Dec 23, 2018

jbrockmendel deleted the over branch December 23, 2018 17:33

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

BUG: Fix overflow bugs in date_Range (pandas-dev#24255)

5509363

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

BUG: Fix overflow bugs in date_Range (pandas-dev#24255)

accafca

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fix overflow bugs in date_Range #24255

BUG: Fix overflow bugs in date_Range #24255

jbrockmendel commented Dec 12, 2018

pep8speaks commented Dec 12, 2018 •

edited

Loading

codecov bot commented Dec 12, 2018

codecov bot commented Dec 12, 2018 •

edited

Loading

jreback left a comment

jbrockmendel commented Dec 13, 2018

jreback commented Dec 13, 2018

jbrockmendel commented Dec 13, 2018

jbrockmendel commented Dec 14, 2018

jreback Dec 15, 2018

jbrockmendel Dec 15, 2018

jreback Dec 15, 2018

jbrockmendel Dec 15, 2018

jreback Dec 15, 2018

jbrockmendel Dec 15, 2018

jreback Dec 15, 2018

jbrockmendel Dec 15, 2018

jbrockmendel commented Dec 16, 2018

jbrockmendel commented Dec 21, 2018

jreback Dec 23, 2018

jreback Dec 23, 2018

jbrockmendel Dec 23, 2018

jreback Dec 23, 2018

jbrockmendel Dec 23, 2018

jreback commented Dec 23, 2018

BUG: Fix overflow bugs in date_Range #24255

BUG: Fix overflow bugs in date_Range #24255

Conversation

jbrockmendel commented Dec 12, 2018

pep8speaks commented Dec 12, 2018 • edited Loading

Comment last updated on December 23, 2018 at 16:48 Hours UTC

codecov bot commented Dec 12, 2018

Codecov Report

codecov bot commented Dec 12, 2018 • edited Loading

Codecov Report

jreback left a comment

Choose a reason for hiding this comment

jbrockmendel commented Dec 13, 2018

jreback commented Dec 13, 2018

jbrockmendel commented Dec 13, 2018

jbrockmendel commented Dec 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Dec 16, 2018

jbrockmendel commented Dec 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Dec 23, 2018

pep8speaks commented Dec 12, 2018 •

edited

Loading

codecov bot commented Dec 12, 2018 •

edited

Loading