Fix wraparound/overflow in date_range #19740

jbrockmendel · 2018-02-17T18:53:45Z

closes date_range produces wrong values when overflowing #14187
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

jreback · 2018-02-17T19:28:25Z

pandas/core/indexes/datetimes.py

+                    # raise instead of incorrectly wrapping around GH#19740
+                    e = b + np.int64(periods) * stride
+            except (FloatingPointError, OverflowError):
+                raise libts.OutOfBoundsDatetime('Cannot generate range with '


there is already a routine to do this checking (not called here) pls use it

greo for overflow

OK, I'll track that down (grep turns up a lot of stackoverflow...). In the interim, #19582, #19611, #19650, #19659 , #19661 have been reviewed, should be straightforward to finish off. Test refactoring in #19736, #19739 should be easy but haven't been reviewed.

grep the codebase (I put it below)

codecov · 2018-02-17T19:56:56Z

Codecov Report

Merging #19740 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #19740      +/-   ##
==========================================
+ Coverage   91.58%    91.6%   +0.02%     
==========================================
  Files         150      150              
  Lines       48862    48873      +11     
==========================================
+ Hits        44750    44770      +20     
+ Misses       4112     4103       -9

Flag	Coverage Δ
#multiple	`89.98% <100%> (+0.02%)`	⬆️
#single	`41.79% <63.63%> (+0.04%)`	⬆️

Impacted Files	Coverage Δ
pandas/core/indexes/datetimes.py	`95.35% <100%> (+0.03%)`	⬆️
pandas/core/ops.py	`96.45% <0%> (-0.39%)`	⬇️
pandas/util/testing.py	`83.64% <0%> (-0.21%)`	⬇️
pandas/core/indexes/base.py	`96.45% <0%> (-0.01%)`	⬇️
pandas/core/arrays/categorical.py	`94.9% <0%> (+0.01%)`	⬆️
pandas/core/indexes/category.py	`97.31% <0%> (+0.03%)`	⬆️
pandas/core/frame.py	`97.22% <0%> (+0.06%)`	⬆️
pandas/plotting/_converter.py	`66.95% <0%> (+1.73%)`	⬆️

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 44c822d...54bd775. Read the comment docs.

jreback · 2018-02-18T16:45:06Z

from pandas.core.algorithms import checked_add_with_arr
does it all

jbrockmendel · 2018-02-18T19:35:52Z

from pandas.core.algorithms import checked_add_with_arr does it all

OK. Do we care about catching OverflowError and re-raising as OutOfBoundsDatetime? If not then this is super-simple. If so then most of the code added here is still needed.

jreback · 2018-02-18T19:42:11Z

i think we should reraise as oob

jbrockmendel · 2018-02-19T17:01:26Z

Agreed. Done.

jreback · 2018-02-19T17:51:12Z

pandas/core/indexes/datetimes.py

@@ -2166,6 +2166,44 @@ def _generate_regular_range(start, end, periods, offset):
    return data


+def _reraise_overflow_as_oob(endpoint, periods, stride, side='start'):
+    """


I think it might make sense to :
move this to datetimelike.py
remove the side arg (and just o on passing in)

the reason is we have several other uses of checked_add_with_arr which are catching a scalar add overflow and should be quite similar to this (the other is in Timedelta).

the other uses I saw were for array-like input and they don't raise.

The side arg is needed if we want the error message to be specific.

All non-test usages of checked_add_with_arr in datetimelike/datetimes/timedeltas. We may be able to consolidate them in datetimelike. (though im pretty sure that numeric classes should have overflow checking implemented at some point)

my point is you are adding a function here, where if there are only 2 uses, its shorter / simpler just to use a try/except directly

... you mean like the original implementation of the PR?

jreback · 2018-02-19T18:00:12Z

this closes #14187 pls add appropriate tests from that issue

jbrockmendel · 2018-02-19T18:29:41Z

this closes #14187 pls add appropriate tests from that issue

I'll add the GH reference to the test already implemented. That test covers the 14187's example almost exactly; only difference is the starting date passed to date_range.

jreback · 2018-02-19T19:15:22Z

pandas/core/indexes/datetimes.py

@@ -2166,6 +2166,44 @@ def _generate_regular_range(start, end, periods, offset):
    return data


+def _reraise_overflow_as_oob(endpoint, periods, stride, side='start'):
+    """


my point is you are adding a function here, where if there are only 2 uses, its shorter / simpler just to use a try/except directly

jreback · 2018-02-19T19:15:22Z

pandas/core/indexes/datetimes.py

@@ -2166,6 +2166,44 @@ def _generate_regular_range(start, end, periods, offset):
    return data


+def _reraise_overflow_as_oob(endpoint, periods, stride, side='start'):
+    """


my point is you are adding a function here, where if there are only 2 uses, its shorter / simpler just to use a try/except directly

jreback

... you mean like the original implementation of the PR?

well the original impl you had a badly written version of checked_add_with_arr

for 2 cases don't use a function, for more than 2 that would be ok.

jreback · 2018-02-19T19:16:17Z

doc/source/whatsnew/v0.23.0.txt

@@ -716,7 +716,7 @@ Datetimelike
 - Bug in :class:`Timestamp` and :func:`to_datetime` where a string representing a barely out-of-bounds timestamp would be incorrectly rounded down instead of raising ``OutOfBoundsDatetime`` (:issue:`19382`)
 - Bug in :func:`Timestamp.floor` :func:`DatetimeIndex.floor` where time stamps far in the future and past were not rounded correctly (:issue:`19206`)
 - Bug in :func:`to_datetime` where passing an out-of-bounds datetime with ``errors='coerce'`` and ``utc=True`` would raise ``OutOfBoundsDatetime`` instead of parsing to ``NaT`` (:issue:`19612`)
-
+- Bug in :func:`date_range` with frequency of ``Day`` or higher where dates sufficiently far in the future could wrap around to the past instead of raising ``OutOfBoundsDatetime`` (:issue:`19740`)


add the issue number here as well

jbrockmendel · 2018-02-19T21:49:45Z

well the original impl you had a badly written version of checked_add_with_arr

I call BS. You're welcome to be demanding but I won't accept aspersions given that it was literally just wrapping the one line that is already there. Closing.

jbrockmendel added 3 commits February 17, 2018 10:52

Fix wraparound/overflow in date_range

44c3a10

edit whatsnew

db37391

add GH reference

b88aff0

jreback requested changes Feb 17, 2018

View reviewed changes

wrap checked_add_with_arr

54bd775

jreback requested changes Feb 19, 2018

View reviewed changes

jreback added Datetime Datetime data dtype Error Reporting Incorrect or improved errors from pandas labels Feb 19, 2018

jreback requested changes Feb 19, 2018

View reviewed changes

jbrockmendel closed this Feb 19, 2018

jbrockmendel mentioned this pull request Oct 25, 2018

BUG: Fix date_range overflow #23345

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix wraparound/overflow in date_range #19740

Fix wraparound/overflow in date_range #19740

jbrockmendel commented Feb 17, 2018 •

edited

Loading

jreback Feb 17, 2018

jreback Feb 17, 2018

jbrockmendel Feb 17, 2018

jreback Feb 18, 2018

codecov bot commented Feb 17, 2018 •

edited

Loading

jreback commented Feb 18, 2018

jbrockmendel commented Feb 18, 2018

jreback commented Feb 18, 2018

jbrockmendel commented Feb 19, 2018

jreback Feb 19, 2018

jbrockmendel Feb 19, 2018

jreback Feb 19, 2018

jreback Feb 19, 2018

jbrockmendel Feb 19, 2018

jreback commented Feb 19, 2018

jbrockmendel commented Feb 19, 2018

jreback Feb 19, 2018

jreback Feb 19, 2018

jreback left a comment

jreback Feb 19, 2018

jbrockmendel commented Feb 19, 2018

Fix wraparound/overflow in date_range #19740

Fix wraparound/overflow in date_range #19740

Conversation

jbrockmendel commented Feb 17, 2018 • edited Loading

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

codecov bot commented Feb 17, 2018 • edited Loading

Codecov Report

jreback commented Feb 18, 2018

jbrockmendel commented Feb 18, 2018

jreback commented Feb 18, 2018

jbrockmendel commented Feb 19, 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 Feb 19, 2018

jbrockmendel commented Feb 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Feb 19, 2018

jbrockmendel commented Feb 17, 2018 •

edited

Loading

codecov bot commented Feb 17, 2018 •

edited

Loading