msg236602 - (view) |
Author: Alexander Belopolsky (belopolsky) * |
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) * |
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) * |
Date: 2015-02-25 18:43 |
You can use self._to_microseconds().
|
msg236614 - (view) |
Author: Alexander Belopolsky (belopolsky) * |
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) * |
Date: 2015-02-25 19:53 |
LGTM. But the bug in C implementation should be fixed.
|
msg236623 - (view) |
Author: Alexander Belopolsky (belopolsky) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2015-02-26 00:00 |
Fixed truediv in issue23521-4.patch.
|
msg236651 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-02-26 07:20 |
Added comments on Rietveld.
|
msg236681 - (view) |
Author: Alexander Belopolsky (belopolsky) * |
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) * |
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) * |
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) * |
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
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:13 | admin | set | github: 67709 |
2015-02-28 15:48:52 | belopolsky | set | status: open -> closed resolution: fixed stage: commit review -> resolved |
2015-02-28 15:46:11 | python-dev | set | nosy:
+ python-dev messages:
+ msg236891
|
2015-02-28 15:02:16 | belopolsky | set | stage: needs patch -> commit review versions:
+ Python 3.4 |
2015-02-28 10:08:19 | serhiy.storchaka | set | messages:
+ msg236872 |
2015-02-26 23:27:06 | belopolsky | set | messages:
+ msg236717 |
2015-02-26 22:34:50 | belopolsky | set | files:
+ issue23521-6.patch
messages:
+ msg236712 |
2015-02-26 18:38:29 | bdkearns | set | messages:
+ msg236697 |
2015-02-26 16:16:54 | belopolsky | set | files:
+ issue23521-5.patch
messages:
+ msg236681 |
2015-02-26 07:20:18 | serhiy.storchaka | set | messages:
+ msg236651 |
2015-02-26 00:00:20 | belopolsky | set | files:
+ issue23521-4.patch
messages:
+ msg236633 |
2015-02-25 23:56:48 | belopolsky | set | messages:
+ msg236632 |
2015-02-25 23:55:07 | belopolsky | set | messages:
+ msg236631 |
2015-02-25 23:34:36 | bdkearns | set | messages:
+ msg236630 |
2015-02-25 23:29:55 | belopolsky | set | messages:
+ msg236629 |
2015-02-25 23:18:53 | bdkearns | set | messages:
+ msg236628 |
2015-02-25 22:52:04 | belopolsky | set | files:
+ issue23521-3.patch
messages:
+ msg236627 |
2015-02-25 21:45:52 | belopolsky | set | nosy:
+ mark.dickinson messages:
+ msg236623
|
2015-02-25 19:53:49 | serhiy.storchaka | set | messages:
+ msg236615 |
2015-02-25 19:30:45 | belopolsky | set | files:
+ issue23521-2.patch
messages:
+ msg236614 |
2015-02-25 18:43:06 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka messages:
+ msg236613
|
2015-02-25 17:22:28 | belopolsky | set | files:
+ issue23521.patch keywords:
+ patch messages:
+ msg236605
|
2015-02-25 17:07:20 | belopolsky | set | nosy:
+ benjamin.peterson, bdkearns
|
2015-02-25 17:05:47 | belopolsky | create | |