classification
Title: OverflowError from timedelta * float in datetime.py
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.5, Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: belopolsky Nosy List: bdkearns, belopolsky, benjamin.peterson, mark.dickinson, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2015-02-25 17:05 by belopolsky, last changed 2015-02-28 15:48 by belopolsky. This issue is now closed.

Files
File name Uploaded Description Edit
issue23521.patch belopolsky, 2015-02-25 17:22 review
issue23521-2.patch belopolsky, 2015-02-25 19:30 review
issue23521-3.patch belopolsky, 2015-02-25 22:52 review
issue23521-4.patch belopolsky, 2015-02-26 00:00 review
issue23521-5.patch belopolsky, 2015-02-26 16:16 review
issue23521-6.patch belopolsky, 2015-02-26 22:34 review
Messages (20)
msg236602 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2015-02-25 17:05
>>> import sys
>>> sys.modules['_datetime'] = None
>>> from datetime import timedelta
>>> timedelta(seconds=1)*0.6112295
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/a/Work/cpython/Lib/datetime.py", line 519, in __mul__
    return self * a / b
  File "/Users/a/Work/cpython/Lib/datetime.py", line 516, in __mul__
    self._microseconds * other)
  File "/Users/a/Work/cpython/Lib/datetime.py", line 411, in __new__
    raise OverflowError("timedelta # of days is too large: %d" % d)
OverflowError: timedelta # of days is too large: 63720670102


C implementation is unaffected:

>>> from datetime import timedelta
>>> timedelta(seconds=1)*0.6112295
datetime.timedelta(0, 0, 611229)
msg236605 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2015-02-25 17:22
Attached patch fixes the issue, but produces a slightly different result:

>>> timedelta(seconds=1)*0.6112295
datetime.timedelta(0, 0, 611230)

Note that C implementation is probably buggy:

>>> from datetime import *
>>> timedelta(seconds=1)*0.6112295
datetime.timedelta(0, 0, 611229)
>>> timedelta(seconds=0.6112295)
datetime.timedelta(0, 0, 611230)

See msg194311  in #8860.
msg236613 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-25 18:43
You can use self._to_microseconds().
msg236614 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2015-02-25 19:30
> You can use self._to_microseconds().

Right.  Did that and added a simple test.
msg236615 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-25 19:53
LGTM. But the bug in C implementation should be fixed.
msg236623 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2015-02-25 21:45
> the bug in C implementation should be fixed.

In the past (see #8860) we were not able to reach a consensus on which behavior is correct and which has a bug:

>>> timedelta(seconds=1)*0.6112295
datetime.timedelta(0, 0, 611229)
>>> timedelta(seconds=0.6112295)
datetime.timedelta(0, 0, 611230)

It all boils down to the the fact that

>>> round(0.6112295*10**6)
611230

but

>>> round(0.6112295, 6) * 10**6
611229.0


In my view, timedelta(seconds=1) is 10**6 microseconds and timedelta(seconds=1) * 0.6112295 is 611229.5 microseconds which is correctly rounded to 611230 in  timedelta(seconds=0.6112295), but timedelta(seconds=1)*0.6112295 returning 611229 is incorrect.

If I understand Mark's argument correctly (see msg107097), he views timedeltas as fractional number of seconds rather than integer number of microseconds and in his view timedelta(seconds=0.6112295) is timedelta(seconds=0.6112295) is 0.611229499999999981 seconds because

>>> Decimal(0.6112295)
Decimal('0.61122949999999998116351207499974407255649566650390625')

Thus timedelta(seconds=0.6112295), 0.6112295 should be rounded down to 6 decimal places and result in 611229 microseconds.

Neither of us thought resolving this was important enough to hold other fixes.  In the same spirit, I suggest that we apply my current patch that fixes a crash and pass the rounding curiosity down to the future generations.
msg236627 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2015-02-25 22:52
I eliminated floating point arithmetics from the calculation completely and it now matches C.
msg236628 - (view) Author: Brian Kearns (bdkearns) * Date: 2015-02-25 23:18
Maybe we should also use divide_and_round for __truediv__ to match the C implementation calling divide_nearest there as well?
msg236629 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2015-02-25 23:29
> Maybe we should also use divide_and_round for __truediv__ 

You mean in timedelta / float case, right?  You are probably right, but can you provide a test case in which it matters?
msg236630 - (view) Author: Brian Kearns (bdkearns) * Date: 2015-02-25 23:34
td = timedelta(seconds=1)
print(td / (1/0.6112295))
msg236631 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2015-02-25 23:55
>>> td = timedelta(seconds=1)
>>> print(td / (1/0.6112295))
0:00:00.611229

What is wrong with this answer? It is the same as in

>>> print(td * 0.6112295)
0:00:00.611229
msg236632 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2015-02-25 23:56
I've got it:

>>> import sys
>>> sys.modules['_datetime'] = None
>>> from datetime import *
>>> td = timedelta(seconds=1)
>>> print(td / (1/0.6112295))
0:00:00.611230
msg236633 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2015-02-26 00:00
Fixed truediv in issue23521-4.patch.
msg236651 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-26 07:20
Added comments on Rietveld.
msg236681 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2015-02-26 16:16
I addressed review comments and fixed the case of negative divisor.  There  may be a better solution than calling the function recursively, but I wanted an easy to understand code.
msg236697 - (view) Author: Brian Kearns (bdkearns) * Date: 2015-02-26 18:38
You saw the demo python implementation of divmod_near in Objects/longobject.c? Maybe it's worth using a common implementation?
msg236712 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2015-02-26 22:34
> You saw the demo python implementation of divmod_near in Objects/longobject.c?

I was looking for it, but could not find.  Thanks for the pointer.  (See also #8817.)

In issue23521-6.patch, I've used a slightly optimized variant of divmod_near code and beefed up the tests some more because the code
is somewhat non-obvious.
msg236717 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2015-02-26 23:27
We can further optimize _divide_and_round by changing r*=2 to r<<=1 and q % 2 == 1 to (q & 1), but this will obfuscate the algorithm and limit arguments to ints.  (As written, _divide_and_round will work with any numeric types.)
msg236872 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-28 10:08
LGTM.
msg236891 - (view) Author: Roundup Robot (python-dev) Date: 2015-02-28 15:46
New changeset 8fe15bf68522 by Alexander Belopolsky in branch '3.4':
Fixes #23521: Corrected pure python implementation of timedelta division.
https://hg.python.org/cpython/rev/8fe15bf68522

New changeset d783132d72bc by Alexander Belopolsky in branch 'default':
Fixes #23521: Corrected pure python implementation of timedelta division.
https://hg.python.org/cpython/rev/d783132d72bc
History
Date User Action Args
2015-02-28 15:48:52belopolskysetstatus: open -> closed
resolution: fixed
stage: commit review -> resolved
2015-02-28 15:46:11python-devsetnosy: + python-dev
messages: + msg236891
2015-02-28 15:02:16belopolskysetstage: needs patch -> commit review
versions: + Python 3.4
2015-02-28 10:08:19serhiy.storchakasetmessages: + msg236872
2015-02-26 23:27:06belopolskysetmessages: + msg236717
2015-02-26 22:34:50belopolskysetfiles: + issue23521-6.patch

messages: + msg236712
2015-02-26 18:38:29bdkearnssetmessages: + msg236697
2015-02-26 16:16:54belopolskysetfiles: + issue23521-5.patch

messages: + msg236681
2015-02-26 07:20:18serhiy.storchakasetmessages: + msg236651
2015-02-26 00:00:20belopolskysetfiles: + issue23521-4.patch

messages: + msg236633
2015-02-25 23:56:48belopolskysetmessages: + msg236632
2015-02-25 23:55:07belopolskysetmessages: + msg236631
2015-02-25 23:34:36bdkearnssetmessages: + msg236630
2015-02-25 23:29:55belopolskysetmessages: + msg236629
2015-02-25 23:18:53bdkearnssetmessages: + msg236628
2015-02-25 22:52:04belopolskysetfiles: + issue23521-3.patch

messages: + msg236627
2015-02-25 21:45:52belopolskysetnosy: + mark.dickinson
messages: + msg236623
2015-02-25 19:53:49serhiy.storchakasetmessages: + msg236615
2015-02-25 19:30:45belopolskysetfiles: + issue23521-2.patch

messages: + msg236614
2015-02-25 18:43:06serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg236613
2015-02-25 17:22:28belopolskysetfiles: + issue23521.patch
keywords: + patch
messages: + msg236605
2015-02-25 17:07:20belopolskysetnosy: + benjamin.peterson, bdkearns
2015-02-25 17:05:47belopolskycreate